Page MenuHomeFreeBSD

Introduce EVENTHANDLER_LIST_* and some users.
ClosedPublic

Authored by mjoras on Oct 28 2017, 3:40 AM.
Tags
None
Referenced Files
F105977788: D12814.diff
Mon, Dec 23, 9:28 AM
Unknown Object (File)
Fri, Dec 13, 1:02 AM
Unknown Object (File)
Mon, Dec 9, 6:17 AM
Unknown Object (File)
Sat, Nov 30, 9:29 PM
Unknown Object (File)
Nov 20 2024, 9:02 AM
Unknown Object (File)
Nov 2 2024, 8:38 AM
Unknown Object (File)
Oct 20 2024, 12:37 PM
Unknown Object (File)
Oct 19 2024, 11:43 PM

Details

Summary

This introduces a facility to EVENTHANDLER(9) for explicitly defining a reference to an event handler list. This is useful since previously all invokers of events had to do a locked traversal of the global list of event handler lists in order to find the appropriate event handler list. By keeping a pointer to the appropriate list an invoker can avoid this traversal completely. The pointer is initialized with SYSINIT(9) during the eventhandler stage. Users registering interest in events do not need to know if the event is backed by such a list, since the list is added to the global list of lists as normal. As with lists that are not pre-defined it is safe to register for the events before the list has been created.

This converts the process_* and thread_* events to using the new facility, as these are events whose traversals end up showing up significantly in ports build workflows. It may be advantageous to convert other events to using the new facility as well.

Test Plan

buildworlds, bulk port builds by mjg with -j80, kernel module that
registers/deregisters and invokes handlers concurrently

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Oct 28 2017, 6:10 AM

Could you please update the EVENTHANDLER(9) man page too?

bdrewery added a subscriber: bdrewery.

Looks good, thanks for doing this. Yes do please update EVENTHANDLER.9

This revision now requires changes to proceed.Oct 31 2017, 1:38 AM

Seems ok to me. Do you plan to update this further in light of Ian's similar patch?

+1 to the man page update.

sys/kern/subr_eventhandler.c
144 ↗(On Diff #34413)

Style: missing newline after definition, and initialization should be done in a separate statement.

Seems ok to me. Do you plan to update this further in light of Ian's similar patch?

+1 to the man page update.

Yeah I've discussed it with Ian. I'll be appropriating the parts of his revision that allow these handlers to be defined from modules (as opposed to my version which only work in the core kernel by design). I'll update the revision shortly with that and the manpage.

  • use a pointer as the global reference to the pre-defined list, allowing for these lists to be created on-register (e.g. from a module) before they are defined. This was the approach suggested and taken by ian in his revision.
  • refactor API usage to reflect this change. LISTS are now defined and declared separately from the eventhandlers. The corresponding invoke is EVENTHANDLER_DIRECT_INVOKE since I think EVENTHANDLER_LIST_INVOKE is too confusing (whereas DIRECT can reasonably imply direct disaptch-ish semantics).
  • Now you must use EVENTHANDLER_LIST_DEFINE to create the reference to the list. EVENTHANDLER_LIST_DECLARE is only needed to declare the list pointer extern, so for callers of EVENTHANDLER_DIRECT_INVOKE that are not in the same compilation unit as the DEFINE.
  • small change to make the eventhandler list name stored at the end of the struct, since the storage is always allocated at the end of the struct anyway.
  • remove EHL_INITTED since it is no longer a relevant state (the lists are initialized either by the SYSINIT stage for eventhandler or when the first handler is registered).
  • update EVENTHANDLER(9) to reflect the API addition, with the recommendation to use the EVENTHANDLER_LIST/DIRECT macros.
mjoras retitled this revision from Introduce EVENTHANDLER_STATIC_* and some users. to Introduce EVENTHANDLER_LIST_* and some users..Nov 1 2017, 5:57 PM
mjoras edited the summary of this revision. (Show Details)
share/man/man9/EVENTHANDLER.9
89 ↗(On Diff #34610)

"even relatively" sounds a bit weird. Relative to what? I'd just remove that qualifier.

I'm also confused by "additionally." Either we're using the dynamic eventhandler interface, or we aren't, right?

93 ↗(On Diff #34610)

New sentences should start on new lines.

177 ↗(On Diff #34610)

New sentence, new line.

sys/kern/subr_eventhandler.c
80 ↗(On Diff #34610)

Missing spaces around "|".

111 ↗(On Diff #34610)

Extra newline.

sys/sys/eventhandler.h
125 ↗(On Diff #34610)

What's the point of this flag? The test is racy anyway, so it seems like you could directly check TAILQ_EMPTY(&el->el_entries).

194 ↗(On Diff #34610)

I think it's ok to defer the corresponding updates for VIMAGE until they're actually needed, but do you foresee any problems with adding "fast" eventhandlers for VIMAGE?

share/man/man9/EVENTHANDLER.9
89 ↗(On Diff #34610)

It does sound weird. I meant relative to other events but you're right, it's not really a useful addition.

There is no non-dynamic interface in this newer revision. I said "additionally" to try to convey that the EVENTHANDLER_LIST macros are completely extra, they don't actually replace any of the other macros (unlike my previous version). The EVENTHANDLER_LIST_DEFINE is just required if you choose to use EVENTHANDLER_DIRECT_INVOKE. There's nothing stopping someone from using EVENTHANDLER_INVOKE with a pre-defined event handler list; it would work but it would also be a mistake.

sys/sys/eventhandler.h
125 ↗(On Diff #34610)

Not really any point. I originally made it as a flag since the original implementation still had a use for EHL_INITTED, so it seemed reasonable to have another flag. I can go ahead and just axe the flags entirely.

mjoras marked 2 inline comments as done.
  • manpage style and extraneous use of "relative"
  • remove the flags field as it is not needed.
sys/sys/eventhandler.h
194 ↗(On Diff #34610)

Just looked at how it is implemented. There's still no users of the code interestingly so I'm curious why bz added it.

It seems like it would be compatible without any work. It essentially just overrides the callback function for the underlying handler with one that iterates over all the vnets. It leverages eventhandler_register_internal to register that handler, which is compatible with pre-defined event handler lists. Since vnets don't need to define their own lists or anything it seems like it should "just work", but there's no users to test :/.

markj added inline comments.
share/man/man9/EVENTHANDLER.9
89 ↗(On Diff #34610)

I see.

This revision is now accepted and ready to land.Nov 2 2017, 8:17 PM
  • in order to be MFCable we can't change the eventhandler_list layout.
This revision now requires review to proceed.Nov 3 2017, 4:47 PM
This revision is now accepted and ready to land.Nov 3 2017, 7:44 PM
This revision was automatically updated to reflect the committed changes.