hwpmc: use hwpmc unlocked hooks for fork and exit

[kib]

Summary

I am not sure if hwpmc kld unload deserves the drain code.

[mjg]

I'm not convinced by this one. If hwpmc is to remain as a module, imo the eventhandler hooks are an ok use here. If putting effort into I think the code should become an integral part of the kernel, but that would require cleaning up some pessimizations.

[kib]

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

[markj]

In D27345#610900, @kib wrote:

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

As I understood, the complaint is that we use EVENTHANDLER in consumers compiled into GENERIC, i.e., some consumers are never unregistered so they pay an unnecessary price.

How do you determine whether an eventhook is necessary or not? Any consumer can add a custom hook instead of using EVENTHANDLER.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

This seems like a reasonable justification for the patch, though IMO it should go in the other direction: the kernel should provide hook points and hwpmc would be a consumer.

sys/dev/hwpmc/hwpmc_mod.c

5801

Isn't it quite likely that this will fail to make progress?

5811

Style

[kib]

In D27345#610923, @markj wrote:

In D27345#610900, @kib wrote:

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

As I understood, the complaint is that we use EVENTHANDLER in consumers compiled into GENERIC, i.e., some consumers are never unregistered so they pay an unnecessary price.

How do you determine whether an eventhook is necessary or not? Any consumer can add a custom hook instead of using EVENTHANDLER.

In base systems, for core component, basically yes, there is no reason to use eventhandlers. They were clearly designed for loose low-bandwidth notifications, with consumers of notification not known by notifier in advance. The lookup of the list by name at runtime, and general flexibility of the KPI point to this.

For instance, use of eventhandlers for power off to notify interested drivers is fine. I was somewhat surprised when I saw that eventhandlers are used to notify ether drivers about vlan reconfig, but then I realized that not all drivers want this notification. Draining and destructing itimers on exec or exit is definitely not the right use of this facility.

Similarly, I believe that very specific consumer of hooks which hwpmc is, should use specific interface.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

This seems like a reasonable justification for the patch, though IMO it should go in the other direction: the kernel should provide hook points and hwpmc would be a consumer.

Isn't this what the patch does, or rather, finishes ? It converts all hwpmc hook points into hwpmc hooks instead of mixing.

Ideally, somewhere at the end of this series of patches, process and thread lifetime hooks can be removed. I still did not decided about consumers like sysvshm and mqueue.

[markj]

In D27345#611028, @kib wrote:

In D27345#610923, @markj wrote:

In D27345#610900, @kib wrote:

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

As I understood, the complaint is that we use EVENTHANDLER in consumers compiled into GENERIC, i.e., some consumers are never unregistered so they pay an unnecessary price.

How do you determine whether an eventhook is necessary or not? Any consumer can add a custom hook instead of using EVENTHANDLER.

In base systems, for core component, basically yes, there is no reason to use eventhandlers. They were clearly designed for loose low-bandwidth notifications, with consumers of notification not known by notifier in advance. The lookup of the list by name at runtime, and general flexibility of the KPI point to this.

I agree. (Note that EVENTHANDLER_DIRECT_* does not do the lookup by name, it uses a static list.)

For instance, use of eventhandlers for power off to notify interested drivers is fine. I was somewhat surprised when I saw that eventhandlers are used to notify ether drivers about vlan reconfig, but then I realized that not all drivers want this notification. Draining and destructing itimers on exec or exit is definitely not the right use of this facility.

Similarly, I believe that very specific consumer of hooks which hwpmc is, should use specific interface.

Ok.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

This seems like a reasonable justification for the patch, though IMO it should go in the other direction: the kernel should provide hook points and hwpmc would be a consumer.

Isn't this what the patch does, or rather, finishes ? It converts all hwpmc hook points into hwpmc hooks instead of mixing.

I mean, some hwpmc hooks could be useful to other consumers. Look at how both hwpmc and dtrace have hooks in sched_switch(), there are other similar points. It is worth trying to minimize EVENTHANDLER usage, but I'm sure new consumers will arise over time.

[mjg]

In D27345#610900, @kib wrote:

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

This does not change the fact scalable eventhandlers will have to be implemented and the patch in the current form adds work, unless it can be demonstrated both fork and exit eventhandlers can be removed altogether. procfs is an example of a module which uses the exit handler, converting that to another test adds even more work to kernels which don't even load the module. In comparison if both remain as eventhandlers they are both sorted out in one go if not present.

Ultimately I don't have a strong opinion here, but I don't think this a win.

[kib]

In D27345#611486, @mjg wrote:

In D27345#610900, @kib wrote:

Argument there is that eventhandler hook is causing locking in fork and exit pathes just by the loading of the module (which was your complain). In other words, it is not much an optimization for hwpmc, but rather a removal of eventhook which is not necessary.

Also it unifies hwpmc interface, by using the same type of hooks for all events watched.

This does not change the fact scalable eventhandlers will have to be implemented and the patch in the current form adds work, unless it can be demonstrated both fork and exit eventhandlers can be removed altogether. procfs is an example of a module which uses the exit handler, converting that to another test adds even more work to kernels which don't even load the module. In comparison if both remain as eventhandlers they are both sorted out in one go if not present.

Ultimately I don't have a strong opinion here, but I don't think this a win.

How does this patch add any work for the eventhandler rewrite ?

And yes, the (eventual) goal is to remove all eventhandlers providing process and thread lifecycle events.