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.

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.

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.

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?

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.