LGTM. I don't get why we don't send PC and the watch stop reason in the '?' case but it probably doesn't matter and it looks like a faithful refactor.

I don't have an objection to changing the LOG_WARNING to LOG_ERR, but I'm confused about what this actually changes. I looked at man 3 syslog and vsyslog1() to see if we printed to stderr by default for LOG_ERR but I'm not seeing that. So, what does this actually do?

A NULL check seems like a clear improvement over a crash.

In D28762#697585, @arichardson wrote:

Seems like this is needed to build stable/13 with GCC. Are you going to MFC this? If not I can cherry-pick it for you.

Will work on a providing a better semaphore first.

In D29542#662307, @mjg wrote:

I disagree with the patch.

The current mechanism is crap and should probably be retired, anyone interested in ramping up kernel memory use has plenty of other ways to do it.

LGTM, and I checked $work's tree for other references.

Change looks fine once the i386 hack is restored.

This can be done without introducing another object pointer (nobj) by resetting lobj and following the pattern elsewhere, but I found the approach with nobj to be more readable.

I guess this case is intended for when a single-ish writer is outpacing the buf daemon?

In D28821#645325, @markj wrote:

In D28821#645324, @mjg wrote:

While the patch at hand may indeed be good enough for a quick fixup, trying to review the entire ordeal suggests the mechanism is very error prone (at least to my taste). For example rm_tracker_remove may still be executing against IPIs and it contains multiple unordered stores.

I think that is ok, the ordering doesn't matter with respect to the IPI, which only does forward traversal and does not update the queue.

I have a couple questions, but the change itself looks fine.

In D28769#645024, @jhb wrote:

In D28769#644380, @rlibby wrote:

In D28769#644259, @jhb wrote:

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

Hmm, I may be misunderstanding something here, but I think I see methods where that isn't true, that return void or pointers? For example, sys/kern/bus_if.m, child_deleted and get_dma_tag, then sys/dev/pci/pci.c, pci_child_deleted and pci_get_dma_tag (void and bus_dma_tag_t).

Yes, those functions do abuse the interface a bit, and it might be ok for the non-void case as long as the function has an explicit default method, but for methods without a default, kobj_error_method() is what gets used. I guess though that the generated BUS_CHILD_DELETED() does actually return void though, so perhaps it isn't really inherent to kobj aside from the default default.

In D28769#644259, @jhb wrote:

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

In D28768#644214, @mm wrote:

I have merged lastest OpenZFS to main including the new "compatibility" property, you can close this now. Thanks for getting the change upstream!

Dropping, alternate solution committed as 0a4f851074a3ca74cd4859c20e7d9807b2aeca65.

In D28761#644041, @rrs wrote:

question

If we put the wrap
#ifdef TCPHPTS

#endif

Around the function do we still need __unused for gcc?

(it won't be unused of course if TCPHPTS is defined)

@freqlabs, @mm, sure thing.

I'm obviously not the maintainer but:

Patch itself looks fine.

markj feedback: man style

markj feedback: ANSI-fy while here

markj feedback:

• Rename the sysctl and make it tunable
• output a warning when input is dropped
• man page blurb

In D28676#642760, @markj wrote:

In D28676#642677, @rlibby wrote:

So, yeah, I don't want to break this kind of thing. On the other hand I want it to be possible to break out of a situation where a tool has executed some kind of command that would take forever to complete and could result in loss of the debugger. What's in the log you linked should fit in the 512 byte buffer, but the point stands if someone had a bigger script like that. One possibility could be just to default to the prioritize_control_input=0 behavior, so the break ability is only lost for large scripts. Another could be just a big bump in the buffer size (2k? 4k?). Thoughts?

So ddb does support a scripting facility, whereby you can run something like

db> script test= show pcpu; ps
db> run test
-- "show pcpu" output, "ps" output --

So there is at least a workaround for this kind of problem, though there you are limited by TOK_STRING_SIZE, I believe.

In D28676#642635, @markj wrote:

So if one were to programatically send a sequence of commands, separated by newlines, to ddb, does this change mean that after the first command begins executing, the echoed characters from subsequent commands will be interleaved with output from the first command?

In D28676#641723, @imp wrote:

Couple of comments but I don't know a lot about the character pipeline so I'm not sure how much good they will do.

imp feedback: provide a chicken switch

In D28375#635347, @bdrewery wrote:

To be clear, everyone is ok with this being committed for now and improvements being worked on and tested for later?

In D28375#634884, @markj wrote:

In D28375#634881, @rlibby wrote:

In D28375#634875, @markj wrote:

In D28375#634860, @rlibby wrote:

In D28375#634851, @markj wrote:

...

One possibility to unify the two tries might be to augment each node with a bitmap, with one bit per slot, and maintain the invariant that a bit is set iff the subtree rooted at that slot contains at least one dirty buf, or the slot points directly to a dirty buf. Then we can still quickly look up the dirty buf with smallest LBN that's larger than a given LBN. That's still not ideal for implementing lookups of clean bufs since all bufs in a subtree may be dirty. We could use a second bitmap to handle that case or maybe there's some more clever way.

Right, if you did it with two bits or two bitmaps, you can track each of dirty and clean per subtree that way, but either way you still have to do the log(n) scan.

Which scan do you mean exactly?

If I understand the pctrie bitmap proposal correctly:

Any walk of just the dirty or clean list becomes O(log(n + m) + n) where n is the number of entries in the list of interest and n + m is the sum of the number of entries from both lists, or in other words, finding the next element becomes O(log(n + m)) instead of O(1). The situation where you'd notice this would be when m >> n. In code, this is git grep "TAILQ_FOREACH.*bo_\(dirty\|clean\)".

Oh, I was envisioning keeping the separate lists and just merging the pctries. The bits let you quickly find the next dirty or clean buf after a given LBN, so buf_vlist_add() would stay more or less the same, and you'd keep the lists for iterating over successive bufs.

In D28375#634875, @markj wrote:

In D28375#634860, @rlibby wrote:

In D28375#634851, @markj wrote:

In D28375#634849, @rlibby wrote:

In D28375#634847, @cem wrote:

I think I mentioned this talking to Bryan but I’m not sure we actually need separate clean and dirty buf tries— we might be better off with a single trie, and two linked lists for enumeration. That’s a big refactor and there may be reasons it isn’t feasible.

Yeah, we need the clean/dirty tailq linkage, but if we could get away with just one pctrie that would be great. That would also get rid of the free/alloc entirely during reassignbuf(). From grepping, only vfs_subr.c actually accesses bv_root, so that's encouraging, and struct bufv doesn't look too bad either. It's looking feasible to me...

I think we use the tries to enable fast insertion into the sorted lists?

Ah, yeah, I see. (And we actually use the order?)

There are some operations on ranges of bufs which rely on this. v_inval_buf_range_locked() for instance. They can drop the bufobj lock between iterations so it doesn't seem very easy to maintain a cursor for the pctrie.

In D28375#634875, @markj wrote:

In D28375#634860, @rlibby wrote:

In D28375#634851, @markj wrote:

...

One possibility to unify the two tries might be to augment each node with a bitmap, with one bit per slot, and maintain the invariant that a bit is set iff the subtree rooted at that slot contains at least one dirty buf, or the slot points directly to a dirty buf. Then we can still quickly look up the dirty buf with smallest LBN that's larger than a given LBN. That's still not ideal for implementing lookups of clean bufs since all bufs in a subtree may be dirty. We could use a second bitmap to handle that case or maybe there's some more clever way.

Right, if you did it with two bits or two bitmaps, you can track each of dirty and clean per subtree that way, but either way you still have to do the log(n) scan.

Which scan do you mean exactly?

In D28375#634851, @markj wrote:

In D28375#634849, @rlibby wrote:

In D28375#634847, @cem wrote:

I think I mentioned this talking to Bryan but I’m not sure we actually need separate clean and dirty buf tries— we might be better off with a single trie, and two linked lists for enumeration. That’s a big refactor and there may be reasons it isn’t feasible.

Yeah, we need the clean/dirty tailq linkage, but if we could get away with just one pctrie that would be great. That would also get rid of the free/alloc entirely during reassignbuf(). From grepping, only vfs_subr.c actually accesses bv_root, so that's encouraging, and struct bufv doesn't look too bad either. It's looking feasible to me...

I think we use the tries to enable fast insertion into the sorted lists?

In D28375#634847, @cem wrote:

I think I mentioned this talking to Bryan but I’m not sure we actually need separate clean and dirty buf tries— we might be better off with a single trie, and two linked lists for enumeration. That’s a big refactor and there may be reasons it isn’t feasible.

This approach seems safe but obviously makes the lookup twice as slow when the buffer doesn't exist. I'm not sure we could do better easily...

imp feedback: keep the limit

Feedback: use default instead of listing cases

The advantage of listing cases instead of using default would be that if another enum value ever gets added, then the compiler's warning about unhandled cases won't have been suppressed. But I agree it's uglier. I'll change it to default.