I do not quite like the patch. May I suggest to have a list of nmi handlers, registered dynamically?

I think registration for single-linked list can be done without sync with nmi processing, and de-registration can simply NULL the function pointer in the list element. This way you would avoid a need to sync with (AKA disable) NMI for list manipulation. But then we remove the need to patch trap.c for each NMI consumer.

In D46421#1058638, @kib wrote:

I do not quite like the patch. May I suggest to have a list of nmi handlers, registered dynamically?

I think registration for single-linked list can be done without sync with nmi processing, and de-registration can simply NULL the function pointer in the list element. This way you would avoid a need to sync with (AKA disable) NMI for list manipulation. But then we remove the need to patch trap.c for each NMI consumer.

I like the approach, but after implementing it and thinking about the matter some more I'm not sure how to proceed.
I'd like to hear what you think before updating the diff.

You can find the dynamic registration patch here. It implements the approach you described above, with the addition of using a UMA_ZONE_NOFREE zone for allocating handler entries and masking the PCINT LVT before invoking the handlers.
However, I'm not sure that adding this additional complexity is entirely justified.
Only the PCINT LVT is currently configured to use an NMI, and according to the AMD and Intel manuals the only PCINT consumers (so far) are performance counters and Intel PT.
Adding dynamic kernel-mode NMI handler registration seems a bit overkill when we're dealing with a maximum of two handlers.

I intended this approach to be applied to all nmi handlers, including the nmi_handle_intr(). [This in particular means that toggling of pcint should be inside the handlers]. Also, with some work, parts of the asm code for nmi from exception.S can be moved to C.

Why do you need zone for the handlers list elements? Due to NOFREE? I do not think it is justifiable.

Why do you check for existing handler registration when registering? If anything, the action should be a panic IMO.

With these changes, it is quite small addition, and most important, it makes code much more modular, which I wanted: remove the knowledge of hwpmc and now pt from trap.c.

I've updated the patch and tested this approach by having pmcstat and the PT driver run at the same time. I have not encountered any crashes while testing.

There is, however, a peculiar issue I've encountered while working on this.
If I dynamically registers the ipi_nmi_handler, I get dropped into ddb when running the test described above.
However, as you can see in the patch, everything works fine if ipi_nmi_handler is invoked before the dynamic handlers.
I'm not sure what's causing this issue.

In D46421#1059782, @kib wrote:

I intended this approach to be applied to all nmi handlers, including the nmi_handle_intr(). [This in particular means that toggling of pcint should be inside the handlers]. Also, with some work, parts of the asm code for nmi from exception.S can be moved to C.

With these changes, it is quite small addition, and most important, it makes code much more modular, which I wanted: remove the knowledge of hwpmc and now pt from trap.c.

That makes sense, thanks for clearing things up!

Why do you check for existing handler registration when registering? If anything, the action should be a panic IMO.

I was on the fence about that one, I've changed it to a panic.

Why do you need zone for the handlers list elements? Due to NOFREE? I do not think it is justifiable.

Right, the handler list elements are never deallocated which is why I've created a NOFREE zone.

Please upload diffs with full context, otherwise the phab feature to suggest diff does not work.

I still suggest to not use zone, NOFREE for several elements is hardly a justification for the whole zone. Plain malloc would be fine there.

In D46421#1060536, @kib wrote:

Please upload diffs with full context, otherwise the phab feature to suggest diff does not work.

Sorry, don't know how that slipped through. The updated diff has full context.

I still suggest to not use zone, NOFREE for several elements is hardly a justification for the whole zone. Plain malloc would be fine there.

No problem, I've switched to plain malloc.

Since the patch is almost finished, I looked at the overall structure and I do not see a reason to use atomics for accesses to ->func. Instead, note that CAS is used against ->next, and there we do need atomics, and perhaps store release on assignment, as well as load acquire on traversing.

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).

In D46421#1064708, @avg wrote:

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.

Thank you, I hadn't thought about this type of situation, the code should properly handle this now.

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.

Yeah, please disregard this.
CK lists / queues are many readers / single writer.
So, unless you'd want to add a lock serializing registrations and deregitrations, those CK containers are not suitable.

At the same time, it might not be such a bad idea :-)

Since registration already uses malloc, it is not wait-free and it does not need to be wait-free in general.
Only nmi_handle_intr has to be wait-free.
So, adding the lock and reducing atomic requirements to handle interactions between a single writer (a single thread (de-)registering a handler) and a single reader (or maybe multiple if we can have concurrent NMI-s) may simplify atomic operations a little bit.
And CK could be used too.

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

Please feel free to ignore it or to pick any ideas, pieces of code, etc.