Page MenuHomeFreeBSD

Add new EVENTHANDLER types: PREEMPTIBLE and SLEEPABLE
AbandonedPublic

Authored by mjg on Aug 23 2022, 7:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 21 2024, 12:11 AM
Unknown Object (File)
Oct 6 2024, 12:38 PM
Unknown Object (File)
Sep 28 2024, 12:05 AM
Unknown Object (File)
Sep 27 2024, 9:53 PM
Unknown Object (File)
Sep 26 2024, 8:44 PM
Unknown Object (File)
Sep 23 2024, 7:50 PM
Unknown Object (File)
Sep 22 2024, 6:45 PM
Unknown Object (File)
Sep 8 2024, 11:46 PM
Subscribers

Details

Reviewers
kib
markj
Summary

The new code uses rm and rms locks respectively to fix scalability issues of the original API.

On top of atomic ops removal it is faster single-threaded by replacing linked lists in favor of an array of callbacks.

Lost features which should not matter:

  • arbitrary argument to be stored at registration and passed to the callback -- all but one cases that I found pass NULL
  • ability to reference lists which don't exist
  • ability to have the callback unregister itself

The original API effectively allowed unbounded sleep as it was executing callbacks without any locks held, but this is not an option with the new approach of keeping locks held the entire time. Relocking would only add overhead. The preemptible case can be used while within NET_EPOCH and to my understanding netlink wants it.

In https://people.freebsd.org/~mjg/eventhandler/ there is a patchset where I converted every single user of EVENTHANDLER_DIRECT_INVOKE and finally retired that bit, with the intent of having any new handler points use the new API.

I tried converting all current users, but there is just too much, so I think the above is a fair compromise.

TODO: document the new API and denote the current one as deprecated ; ask someone with powerpc to test the patch to their code

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mjg edited the summary of this revision. (Show Details)
rpokala added inline comments.
sys/kern/subr_eventhandler.c
374

If this is the default case, why specify EVENTHANDLER_PRIORITY_INVALID as well? Also, default should come at the end of the list, so if you keep the INVALID case, it should fall-thru to default.

sys/sys/_eventhandler.h
102

Would it make more sense to have common be the first field, so the sleepable/preemptable structures would have the same starting layout and could be cast to the common structure?

sys/kern/subr_eventhandler.c
609

Is this missing that __unreachable() builtin? Shouldn't eventhandler_common_validate() cover this?

sys/sys/_eventhandler.h
102

That would make sense to me, too.

  • consistently use __builtin_unreachable
  • validate before and after register/unregister
  • style tidy ups
sys/kern/subr_eventhandler.c
609

it is coverd by the validate func, but clang warns anyway

sys/sys/_eventhandler.h
102

i don't see any point. if there is any need to pass them in, you can pass the common part already and whatever code needs the full thing can just unwrap it with container_of, in fact this is already done in this file

mjg planned changes to this revision.Aug 25 2022, 5:06 PM

I found some bugs in array manipulation, will fix later.