devd: move all devd notification logic to a separate file

[melifaro]

Summary

Move all devd notification business logic to a separate file.

Currently, subr_bus.c shares logic for (a) maintaining all HW devices (e.g. discovery/attach/detach logic) and (b) generic devctl notification layer for devices/PMU/GEOM/interfaces/etc).
These two domains share very limited interface, composed of just 3 notification functions. With that in mind, move devctl layer to a separate file, establishing a clear notification interface between the two.

The primary driver of this change is netlink implementation (D36002). The idea is to propagate device-level events to netlink as well, so all netlink customers can subscribe to these changes.
The long-term is to deprecate devctl and to use netlink as kernel<> userland transport provided netlink gets enough traction.

[melifaro]

update includes.

[markj]

Seems sensible to me overall.

sys/kern/kern_devctl.c

124

This can be static const.

200

Extra newline.

562

Let's keep it together with the definition of devd_init()?

[imp]

Can you talk more about other notifiers? And how this separates the 'buisiness logic'? I don't even know what you mean by that since that term has been so overused in the last 30 years that it's lost all real meaning...

Also, if there are other notification methods, then why does this help. They'd be independent anyway.

Also, link status changes are already propagated, so I don't understand the need for this, though 'netlink' likely is some new work that I don't have sufficient context on to understand why it would need a special/different mechanism.

So the actual move isn't bad (modulo the new-exposure of private interfaces), but the justification for it is so light that I can't fill in enough of the gaps to even get the 'gist' of what's planned. Please share more your ideas, in arch@ if necessary and they won't fit here.

sys/sys/devctl.h

18 ↗

(On Diff #109045)

I'd strongly prefer that these be called out as 'private' and 'not for use outside of newbus' in comments and likely in a separate section. These are internal interfaces, and were static before because they are hard to use outside of newbus and there's non-obvious connections between these events and what devd does, so users outside of newbus can cause big problems for devd.

[melifaro]

In D36091#819539, @imp wrote:

First of all, thank you for the valuable feedback! Not all reviews get feedback, so sometimes it's worth starting with light description :-)

Can you talk more about other notifiers? And how this separates the 'buisiness logic'? I don't even know what you mean by that since that term has been so overused in the last 30 years that it's lost all real meaning...

Sure! I'm talking about netlink implementation ( D36002 ). Re business logic - indeed it's pretty much overused in many different aspects. What I meant was that subr_bus.c contained device management logic (discovery/addition/removal) and generic userland notification mechanism used for everything not-network-related. 'Separation of business logic' meant exactly providing basic interface for the former to propagate device-oriented events, I'll update the review to make devctl to subscribe for device_attach and device_detach. Guess I need to introduce device_nomatch eventhandler hook as well.

Also, if there are other notification methods, then why does this help. They'd be independent anyway.

Some of the events such as nomatch are currently not exposed.

Also, link status changes are already propagated, so I don't understand the need for this, though 'netlink' likely is some new work that I don't have sufficient context on to understand why it would need a special/different mechanism.

Yep, netlink (or any other notification mechanism) doesn't need anything specific for link events and can just subscribe to the existing eventhandlers.

So the actual move isn't bad (modulo the new-exposure of private interfaces), but the justification for it is so light that I can't fill in enough of the gaps to even get the 'gist' of what's planned. Please share more your ideas, in arch@ if necessary and they won't fit here.

[imp]

In D36091#819557, @melifaro wrote:

In D36091#819539, @imp wrote:

First of all, thank you for the valuable feedback! Not all reviews get feedback, so sometimes it's worth starting with light description :-)

Can you talk more about other notifiers? And how this separates the 'buisiness logic'? I don't even know what you mean by that since that term has been so overused in the last 30 years that it's lost all real meaning...

Sure! I'm talking about netlink implementation ( D36002 ). Re business logic - indeed it's pretty much overused in many different aspects. What I meant was that subr_bus.c contained device management logic (discovery/addition/removal) and generic userland notification mechanism used for everything not-network-related. 'Separation of business logic' meant exactly providing basic interface for the former to propagate device-oriented events, I'll update the review to make devctl to subscribe for device_attach and device_detach. Guess I need to introduce device_nomatch eventhandler hook as well.

Likely. One of the issues I foresee is that these events can be called when the kernel can't wait for memory, which is why we have our own memory allocator to allow us to generate the events always (with a drop oldest policy should devd get ridiculously far behind events generated in this context). The data isn't stable beyond the call to the notifier, which would make using 'eventhandler' as its current constituted tricky since these context also can't sleep and it uses default locks...

Also, in case it wasn't clear, please use a different term than 'business logic' here in your final commit message.

Also, while I'm happy to review these changes in bulk, I'd like to express a very strong preference to a series of small commits to help bisect-ability should something break and we need to track it down after the fact. This is doubly true here as a lot of different people use it and what breaks is likely not necessarily apparent to you or I (otherwise we'd fix it) and will likely show up in some strange context.

Also, if there are other notification methods, then why does this help. They'd be independent anyway.

Some of the events such as nomatch are currently not exposed.

Yea, the devctl_notify_* routines is very much intended to be a very private interface. And I think I'll have to quibble on your names, because it isn't that the device is added or remove, but that it's attached or detached (which are semantically different in newbus: devices may be unattached and present because there's no driver loaded for them, for example).

Also, link status changes are already propagated, so I don't understand the need for this, though 'netlink' likely is some new work that I don't have sufficient context on to understand why it would need a special/different mechanism.

Yep, netlink (or any other notification mechanism) doesn't need anything specific for link events and can just subscribe to the existing eventhandlers.

So the actual move isn't bad (modulo the new-exposure of private interfaces), but the justification for it is so light that I can't fill in enough of the gaps to even get the 'gist' of what's planned. Please share more your ideas, in arch@ if necessary and they won't fit here.

So in general, I like this now that I understand things better. There's almost no down-stream consumers of devd events, except zfsd which will also need to be transitioned as well. AF_NETLINK sounds like a better match to what it really needs. X11 did listen for a while, but I think it's all moved to evdev which is a better fit for its needs.

[melifaro]

Address comments:

• do not expose devctl_notify_dev_* functions, subscribe to the event handlers instead
• move SYSINIT closer to the actual function
• make devctl_rfiltops static

[melifaro]

Likely. One of the issues I foresee is that these events can be called when the kernel can't wait for memory, which is why we have our own memory allocator to allow us to generate the events always (with a drop oldest policy should devd get ridiculously far behind events generated in this context). The data isn't stable beyond the call to the notifier, which would make using 'eventhandler' as its current constituted tricky since these context also can't sleep and it uses default locks...

Got it. That's a valuable detail. So, the provider should treat these events as critical w.r.t delivery and should have some pre-allocated memory to allow propagation of these events to userland?

Also, in case it wasn't clear, please use a different term than 'business logic' here in your final commit message.

Will do.

Also, while I'm happy to review these changes in bulk, I'd like to express a very strong preference to a series of small commits to help bisect-ability should something break and we need to track it down after the fact. This is doubly true here as a lot of different people use it and what breaks is likely not necessarily apparent to you or I (otherwise we'd fix it) and will likely show up in some strange context.

I'd probably stick with the variation of this review for now - it establishes full eventhandler-based KPI for device-level notifications and clearly separates devctl code. The next changes will be in the direction of hooking devctl_notify() calls to generate structured output for userland, but these will go one-by one, as separate reviews/commits.

Also, if there are other notification methods, then why does this help. They'd be independent anyway.

Some of the events such as nomatch are currently not exposed.

Yea, the devctl_notify_* routines is very much intended to be a very private interface. And I think I'll have to quibble on your names, because it isn't that the device is added or remove, but that it's attached or detached (which are semantically different in newbus: devices may be unattached and present because there's no driver loaded for them, for example).

Does the new version look better to you?

[imp]

I think I like moving to event handler, assuming that the locking that it does doesn't introduce problems with ordering or context. I didn't see ordering issues, but I'm still unsure on context.

sys/kern/kern_devctl.c

197

You don't need the cast here. It really doesn't add any value and we've typically been omitting it.

199

So there's a smalls semantic difference between this code and the old code. The old code would only call devremove() when we successfully detached. Moving this to the event handler, though, I think means that we'll get callbacks for both successful and unsuccessful events.

[melifaro]

• react only on EVHDEV_DETACH_COMPLETE

[melifaro]

Remove (device_t) typecasts.

[imp]

sys/kern/kern_devctl.c

5

This should likely have my copyright on it.

Copyright (c) 2002-2020 M. Warner Losh <[e-mail]>

And the SPDX-License-Identifier: should be just 'BSD-2-Clause' It's wrong in a lot of places in the tree today (long story) and these licenses don't match what the SPDX people have as BSD-2-Clause-FreeBSD for reasons I've never been completely clear on and haven't asked about (again) since I got connected with that group.

I wrote this code originally in 2002 and updated it over the years, most recent major update was in 2020.

198

Thanks!

[melifaro]

Update copyright & author.

[imp]

sys/kern/kern_devctl.c

6

Thanks!

[melifaro]

Fix eventhandler callbacks: the first argument is always user-provided.