In D50825#1204638, @markj wrote:

If this looks ok, I'll extend it to support PCPU and DPCPU variables. And probably counter(9) too.

I never really worked with code in this area, but the change looks good to me.

In D52466#1198185, @kib wrote:

I somewhat doubt that the arguments by @avg were true.

Maybe we can even retire printm...
I never knew it existed.

Thank you!

LGTM

@zldrobit_gmail.com if you cannot submit the change for review from Git (e.g., using git-arc), can you update it with a diff created with -U999 option?

Here is an example of how I made use of this code in iichid when it still was out-of-tree:
https://github.com/avg-I/iichid/commit/d5c33b243c599fd234685a2cd8ee1bd1fb67e1b0

In D50338#1148850, @olce wrote:

In D50338#1148702, @avg wrote:

I pointed out this problem to Kirk and Colin back when the change was made.
I had to do the same kind of fix locally (for Panzura) because of live-locks as well.

I should probably have contacted you when I saw the revert commit, as even if I hadn't suspected there was a problem with it and hadn't experienced deadlocks yet, I was interested in the vnode cache and had analyzed quite a bunch of it at that time already.

That said, it would have been nice that Kirk and Colin put the revert under review (I suspect they didn't in part because they were running out of time) so that your "objection" could be logged. Were there any specific problems preventing you from submitting your local fix, or just lack of time?

Anyway, we are now at risk of shipping 14.3 with this annoying bug (although I don't know its frequency in general; all I can say is that, for my use case, it's becoming super annoying). I hope the fix here will be included in 14.3. Will normally have finished my tests in a couple of hours (now with the reload of numvnodes), and will report here.

Thank you very much!

I pointed out this problem to Kirk and Colin back when the change was made.
I had to do the same kind of fix locally (for Panzura) because of live-locks as well.
But I have one small difference, not sure if important, which I'll describe in an inline comment.

Thank you!

Looks good to me.

Still looks good to me.

The change looks good to me.
Thank you.

I think that vmxnet3_isc_txd_credits_update can be made similar to, e.g., igb_isc_txd_credits_update with respect to the array index (tx_rs_cidx and vxcr_next respectively).
That is, we can stash vxcr_next into a local variable and then use the local variable for iteration.
At the end, we can update vxcr_next from the local variable.
This won't fix other potential issues with correctness / concurrency (data races), but at least it will ensure that vxcr_next will not have an out-of-bounds value.

@jhb my personal / private interest in sleepable ithreads is "natural" support for interrupt controllers on slow buses.
Originally, it was assumed that an interrupt controller has fast access and they had.
But imagine something like an I/O expander on an I2C bus.
Let's assume that it supports detecting changes on its I/O inputs and also has a dedicated line to an upstream interrupt controller.
We can configure interrupt modes using I2C transactions and we can also query / mask / etc interrupt lines using I2C.
This is a real interrupt controller from all points of view and if we want to avoid polling (for potentially long time) we need to support sleeping / waiting while communicating with such controller.
At least, that's how I see it.

Looks goof to me.
Thank you!

In D47779#1089560, @christos wrote:

I guess the same is true for PCM_RELEASE.

In D47779#1089556, @christos wrote:

Alright. There are other places in the sound driver that cv_waiters is still accessed directly, so I will see how I can fix those as well.

In D47779#1089553, @christos wrote:

Is there a reason (except maybe layering violations) that we do not want to do it this way and it's better to implement an additional counter in the driver?

Also, cv_wmesg is kind of semi-private and it's used only for diagnostics (tracing).
So, I don't think that it's precedent for general access to cv implementation details.

As I noted in the email thread, there is only one consumer outside of condvar implementation.

In D47571#1085010, @Matthew.Nygard.Dodd_gmail.com wrote:

In D47571#1084984, @avg wrote:

If you have added this from a diff, could you please re-do the diff with -U 9999 option?
That's to have a lot more context lines which would be useful when reviewing.

Done.

If you have added this from a diff, could you please re-do the diff with -U 9999 option?
That's to have a lot more context lines which would be useful when reviewing.

Oh, thinking about this triggered some old memories and I've dug up my old musings on this topic.
The code was never really tested and I cannot guarantee that it's correct, but it seemed correct to the younger me and it still seems correct the now me.
https://people.freebsd.org/~avg/wfhandler.v8.h

In D46421#1066706, @avg wrote:

Another general suggestion is that using CK_SLIST from ConcurrencyKit (included into FreeBSD) might help to make the code more compact and expressive.
But I haven't thought through details, so please feel free to ignore this suggestion if CK does not provide something that could be readily used.
The proposed code is completely good too.

Another general suggestion is that using CK_SLIST from ConcurrencyKit (included into FreeBSD) might help to make the code more compact and expressive.
But I haven't thought through details, so please feel free to ignore this suggestion if CK does not provide something that could be readily used.
The proposed code is completely good too.

I am not sure that such a OS/ACPI (firmware) mutex is possible.

My only concern with the current deregistration approach is a hypothetical situation where a handler resides in a loadable module and, thus, can be unloaded after deregistration.
I like the clever simplicity of the current code where list elements are never removed and only the head pointer can ever be modified.
But maybe it would be good for nmi_remove_handler to wait for a completion of the handler if it's running.
E.g., we could add an atomic counter to struct nmi_handler which would be incremented when the handler is started and decremented when the handler is done.
Then, nmi_remove_handler could spin-wait on that counter if it's not zero (after having cleared func field to prevent future calls).

LGTM

I like how using t_dtrace_trapframe both eliminates the search for the frame below the trap frame and the assumption on the position of the trap frame.