Date: Fri, 9 Dec 2022 22:09:49 -0700 From: Warner Losh <imp@bsdimp.com> To: Baptiste Daroussin <bapt@freebsd.org> Cc: hackers@freebsd.org Subject: Re: devctl_notify system is inconsistent Message-ID: <CANCZdfp6YsxmRNN%2BvGnc8wBjUmOaASvOMXCQYSb8wnK9pBjeiQ@mail.gmail.com> In-Reply-To: <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu> References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> <CANCZdfpdkxGaan6fvTYxm4YO17-t=V4yuzPWYJnGd7Ao73NQgw@mail.gmail.com> <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu> <CANCZdfoxKs9FJMyRh=gVSo9JnX-Lb7-ybXmq==NvzjdKVpckBw@mail.gmail.com> <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--0000000000004012d805ef7248a6 Content-Type: text/plain; charset="UTF-8" On Thu, Dec 1, 2022 at 10:40 AM Baptiste Daroussin <bapt@freebsd.org> wrote: > On Thu, Dec 01, 2022 at 08:38:37AM -0700, Warner Losh wrote: > > On Thu, Dec 1, 2022 at 7:59 AM Baptiste Daroussin <bapt@freebsd.org> > wrote: > > > > > On Thu, Dec 01, 2022 at 07:45:32AM -0700, Warner Losh wrote: > > > > On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin <bapt@freebsd.org> > > > wrote: > > > > > > > > > Hello, > > > > > > > > > > After the addition of netlink(4) by melifaro@, I started working > on a > > > new > > > > > genetlink(4) module, to send kernel notification to the userland > via > > > > > netlink. > > > > > > > > > > The goal is to be able to have multiple consumers without the need > of > > > devd > > > > > to be > > > > > running. > > > > > > > > > > The goal is also to be able subscribe to the events the consumer is > > > > > willing to > > > > > receive. > > > > > > > > > > https://reviews.freebsd.org/D37574 > > > > > > > > > > I also added a hook to devctl_notify to make sure all its event got > > > sent > > > > > via > > > > > nlsysevent. (https://reviews.freebsd.org/D37573) > > > > > > > > > > > > > You're connecting it at the wrong level: it will miss all device > events. > > > > devctl_notify > > > > is used by everything except newbus's device attach, detach and > nomatch > > > > events, so none of those events will make it through. > > > > > > Where should I hook to get the device events? > > > > > > > Either you need to drop down a level to where the formated events are > > queued, > > or you'll need to add something in devaddq() to deal with the events. > These > > are > > a slightly different format than the devctl_notify() events because the > > latter was > > added later and I didn't want to cope with transitioning the old > formatted > > messages > > to the new at that time (silly me). > > > > > > > > > > > > > > > > > It works great and so far I am happy with it. on thing I figured > out > > > it is: > > > > > the "system" argment of devctl_notify is inconsistent: > > > > > Upper case vs lower case > > > > > "kern" vs "kernel" > > > > > > > > > > > > > They are consistent. With one exception that I deprecated in 13.x to > be > > > > removed in 14.x. I documented all of them at the time in devd.conf. I > > > think > > > > I'll go ahead and complete the deprecation. > > > > > > > > > > > > > I intent to fix the following way: > > > > > Create a new function similar to devctl_notify but with the first > > > argument > > > > > being > > > > > an enum. > > > > > > > > > > > > > I'm a pretty hard no on the enum. I rejected doing it that way when I > > > wrote > > > > devd > > > > years ago. These have always been strings, and need to continue to > > > always be > > > > strings, or we're forever modifying devd(8) when we add a new one and > > > > introducing > > > > yet another opportunity for mismatch. I don't think this is a good > idea > > > at > > > > all. > > > > > > > > There's been users of devd in the past that are loadable modules that > > > pass > > > > their > > > > own system name in that devd.conf files would then process. Having an > > > enum > > > > with > > > > enforcement would just get in the way of this. > > > > > > > > I have toyed with the idea of having a centralized list in bus.h > that's a > > > > bunch of > > > > #defines, ala old-school X11 resources (which this is very very > loosely > > > > based on), > > > > but it didn't seem worth the effort. > > > > > > That is fine with me and actually I do need the string name to > register as > > > group > > > name, that I didn't want to trash the strings away. > > > > > > But I need a way to list them all. > > > > > > > We have no current mechanism to do that. We could add something that > would > > register / deregister them with a sysinit + call to something in > > kern_devctl.c which > > could do the trick (and also deal with the ordering issues), though > having > > netlink(4) > > as a loadable modules might be an interesting case to get right. > > > > If we did that, we could return a 'token' that you'd use to call a new > > version of > > devctl_notify(), perhaps with some glue for legacy users (or perhaps not: > > we change > > kernel interfaces all the time, and could just rename it and convert > > everything in the > > tree). > > > > > > > > > > > > > > Make the current devctl_notify convert its first argument into that > > > enum > > > > > and > > > > > yell if an unkwown "system" is passed. (and probably declare > > > devctl_notify > > > > > deprecated) > > > > > > > > > > > > > I don't think this buys us anything. devctl_notify cannot possibly > know > > > > about all > > > > the possible subsystems, so this is likely doomed to high maintenance > > > that > > > > would > > > > be largely ineffective. > > > > > > > > Then fix the inconsistencies: all upper case as it seems the most > wildly > > > use > > > > > case > > > > > s/kern/kernel/g > > > > > > > > > > WDYT? > > > > > > > > > > > > > I wouldn't worry about the upper vs lower case. All the documented > ones > > > are > > > > upper case, except kernel. There's no harm it being one exception > since > > > > changing > > > > it will break user's devd.conf setups. I got enough feedback when I > > > changed > > > > "kern" to "kernel" last year for power events. And as far as I know, > I've > > > > documented > > > > all the events that are generated today. > > > > > > > > Warner > > > > > > > > > Note that if you think nlsysevent is a bad idea, I will just forget > about > > > it. > > > > > > > I love the idea. I got over any resistance I had when melifaro@ moved > > things into kern_devctl.c and we talked through the issues at that time. > > I've been hoping that someone would replace the hacky thing I did with > > a 'routing socket'-like interface (I never could figure out hose to do > it so > > many years ago w/o gross hacks). netlink(4) has finally provided a way > > to do it that doesn't get the routing code all jumbled up for this. > > > > I just have some specific issues with your proposal. In hindsight, I > should > > have led with that in my first message :). > > > > Warner > > Here is my new proposal: > > What about: > > 1. I add a static list of systems in sys/devctl.h something like > > enum { > DEVCTL_SYSTEM_ACPI = 0, > DEVCTL_SYSTEM_AEON = 1, > ... > DEVCTL_SYSTEM_ZFS = 19 > }; > > static const char *devctl_systems[] = { > "ACPI", > "AEON", > ... > "ZFS", > }; > > this way we have a list of official freebsd's systems. > We don't change the devctl_notify interface > > and in the kernel we change the devctl_notify("ZFS" into > devctl_notify(devctl_systems[DEVCTL_SYSTEM_ZFS],... > > This is not too intrusive, and breaks none of the existing code > So what happens if you see one not in the list? > 2. I also hook devadq using the same interface as I already have done, but > all the attach,detach,nomatch become an event (only in nlsysevent) in the > "DEVICE" system, > the "SUBSYSTEM" is the current what of devaddq > > The type is changed into: > + -> ATTACH > - -> DETACH > anythingelse -> NOMATCH > and the rest of the current line becomes the data > This is fine. I kinda think it might not be terrible to expose this to devd and have it cope, but that's a zero sum for not a lot of gain. so from nlsysvent point of view this is exactly the same kind of events as > the > one hooked in devctl_notify. > Sure. > 3. in nlsysevent we have one category one can subscribe per known systems. > All other "systems" falls into a new category named "extra" "vendor" or > "other" > So all events that match elements of the array are 'system' and the others are something else? > from the consumer point of view the name of the system is anyway contained > in > the message itself, so the category it is subscribed to can differ. > Right. No data is lost... > This way, I should be able to get ALL the events in a consistent way. > I should be compatible with people who uses devctl_notify in their custom > kernel > modules. > Yea. > Sounds easy enough without the requirement of complexifying kern_devctl.c > with a > registration of extra systems. > Yea, I kinda prefer that... Unless we add too many new systems. It's still extra work to add one, but likely not enough extra to be worth the automation. > What do you think? > Not bad. Warner --0000000000004012d805ef7248a6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Thu, Dec 1, 2022 at 10:40 AM Bapti= ste Daroussin <<a href=3D"mailto:bapt@freebsd.org">bapt@freebsd.org</a>&= gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0= px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Th= u, Dec 01, 2022 at 08:38:37AM -0700, Warner Losh wrote:<br> > On Thu, Dec 1, 2022 at 7:59 AM Baptiste Daroussin <<a href=3D"mailt= o:bapt@freebsd.org" target=3D"_blank">bapt@freebsd.org</a>> wrote:<br> > <br> > > On Thu, Dec 01, 2022 at 07:45:32AM -0700, Warner Losh wrote:<br> > > > On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin <<a hre= f=3D"mailto:bapt@freebsd.org" target=3D"_blank">bapt@freebsd.org</a>><br= > > > wrote:<br> > > ><br> > > > > Hello,<br> > > > ><br> > > > > After the addition of netlink(4) by melifaro@, I starte= d working on a<br> > > new<br> > > > > genetlink(4) module, to send kernel notification to the= userland via<br> > > > > netlink.<br> > > > ><br> > > > > The goal is to be able to have multiple consumers witho= ut the need of<br> > > devd<br> > > > > to be<br> > > > > running.<br> > > > ><br> > > > > The goal is also to be able subscribe to the events the= consumer is<br> > > > > willing to<br> > > > > receive.<br> > > > ><br> > > > > <a href=3D"https://reviews.freebsd.org/D37574" rel=3D"n= oreferrer" target=3D"_blank">https://reviews.freebsd.org/D37574</a><br>; > > > ><br> > > > > I also added a hook to devctl_notify to make sure all i= ts event got<br> > > sent<br> > > > > via<br> > > > > nlsysevent. (<a href=3D"https://reviews.freebsd.org/D37= 573" rel=3D"noreferrer" target=3D"_blank">https://reviews.freebsd.org/D3757= 3</a>)<br> > > > ><br> > > ><br> > > > You're connecting it at the wrong level: it will miss al= l device events.<br> > > > devctl_notify<br> > > > is used by everything except newbus's device attach, det= ach and nomatch<br> > > > events, so none of those events will make it through.<br> > ><br> > > Where should I hook to get the device events?<br> > ><br> > <br> > Either you need to drop down a level to where the formated events are<= br> > queued,<br> > or you'll need to add something in devaddq() to deal with the even= ts. These<br> > are<br> > a slightly different format than the devctl_notify() events because th= e<br> > latter was<br> > added later and I didn't want to cope with transitioning the old f= ormatted<br> > messages<br> > to the new at that time (silly me).<br> > <br> > <br> > > ><br> > > ><br> > > > > It works great and so far I am happy with it. on thing = I figured out<br> > > it is:<br> > > > > the "system" argment of devctl_notify is inco= nsistent:<br> > > > > Upper case vs lower case<br> > > > > "kern" vs "kernel"<br> > > > ><br> > > ><br> > > > They are consistent. With one exception that I deprecated in= 13.x to be<br> > > > removed in 14.x. I documented all of them at the time in dev= d.conf. I<br> > > think<br> > > > I'll go ahead and complete the deprecation.<br> > > ><br> > > ><br> > > > > I intent to fix the following way:<br> > > > > Create a new function similar to devctl_notify but with= the first<br> > > argument<br> > > > > being<br> > > > > an enum.<br> > > > ><br> > > ><br> > > > I'm a pretty hard no on the enum. I rejected doing it th= at way when I<br> > > wrote<br> > > > devd<br> > > > years ago. These have always been strings, and need to conti= nue to<br> > > always be<br> > > > strings, or we're forever modifying devd(8) when we add = a new one and<br> > > > introducing<br> > > > yet another opportunity for mismatch. I don't think this= is a good idea<br> > > at<br> > > > all.<br> > > ><br> > > > There's been users of devd in the past that are loadable= modules that<br> > > pass<br> > > > their<br> > > > own system name in that devd.conf files would then process. = Having an<br> > > enum<br> > > > with<br> > > > enforcement would just get in the way of this.<br> > > ><br> > > > I have toyed with the idea of having a centralized list in b= us.h that's a<br> > > > bunch of<br> > > > #defines, ala old-school X11 resources (which this is very v= ery loosely<br> > > > based on),<br> > > > but it didn't seem worth the effort.<br> > ><br> > > That is fine with me and actually I do need the string name to re= gister as<br> > > group<br> > > name, that I didn't want to trash the strings away.<br> > ><br> > > But I need a way to list them all.<br> > ><br> > <br> > We have no current mechanism to do that. We could add something that w= ould<br> > register / deregister them with a sysinit + call to something in<br> > kern_devctl.c which<br> > could do the trick (and also deal with the ordering issues), though ha= ving<br> > netlink(4)<br> > as a loadable modules might be an interesting case to get right.<br> > <br> > If we did that, we could return a 'token' that you'd use t= o call a new<br> > version of<br> > devctl_notify(), perhaps with some glue for legacy users (or perhaps n= ot:<br> > we change<br> > kernel interfaces all the time, and could just rename it and convert<b= r> > everything in the<br> > tree).<br> > <br> > ><br> > > ><br> > > > > Make the current devctl_notify convert its first argume= nt into that<br> > > enum<br> > > > > and<br> > > > > yell if an unkwown "system" is passed. (and p= robably declare<br> > > devctl_notify<br> > > > > deprecated)<br> > > > ><br> > > ><br> > > > I don't think this buys us anything. devctl_notify canno= t possibly know<br> > > > about all<br> > > > the possible subsystems, so this is likely doomed to high ma= intenance<br> > > that<br> > > > would<br> > > > be largely ineffective.<br> > > ><br> > > > Then fix the inconsistencies: all upper case as it seems the= most wildly<br> > > use<br> > > > > case<br> > > > > s/kern/kernel/g<br> > > > ><br> > > > > WDYT?<br> > > > ><br> > > ><br> > > > I wouldn't worry about the upper vs lower case. All the = documented ones<br> > > are<br> > > > upper case, except kernel. There's no harm it being one = exception since<br> > > > changing<br> > > > it will break user's devd.conf setups. I got enough feed= back when I<br> > > changed<br> > > > "kern" to "kernel" last year for power e= vents. And as far as I know, I've<br> > > > documented<br> > > > all the events that are generated today.<br> > > ><br> > > > Warner<br> > ><br> > ><br> > > Note that if you think nlsysevent is a bad idea, I will just forg= et about<br> > > it.<br> > ><br> > <br> > I love the idea. I got over any resistance I had when melifaro@ moved<= br> > things into kern_devctl.c and we talked through the issues at that tim= e.<br> > I've been hoping that someone would replace the hacky thing I did = with<br> > a 'routing socket'-like interface (I never could figure out ho= se to do it so<br> > many years ago w/o gross hacks). netlink(4) has finally provided a way= <br> > to do it that doesn't get the routing code all jumbled up for this= .<br> > <br> > I just have some specific issues with your proposal. In hindsight, I s= hould<br> > have led with that in my first message :).<br> > <br> > Warner<br> <br> Here is my new proposal:<br> <br> What about:<br> <br> 1. I add a static list of systems in sys/devctl.h something like<br> <br> enum {<br> =C2=A0DEVCTL_SYSTEM_ACPI =3D 0,<br> =C2=A0DEVCTL_SYSTEM_AEON =3D 1,<br> =C2=A0...<br> =C2=A0DEVCTL_SYSTEM_ZFS =3D 19<br> };<br> <br> static const char *devctl_systems[] =3D {<br> =C2=A0 "ACPI",<br> =C2=A0 "AEON",<br> =C2=A0 ...<br> =C2=A0 "ZFS",<br> };<br> <br> this way we have a list of official freebsd's systems.<br> We don't change the devctl_notify interface<br> <br> and in the kernel we change the devctl_notify("ZFS" into<br> devctl_notify(devctl_systems[DEVCTL_SYSTEM_ZFS],...<br> <br> This is not too intrusive, and breaks none of the existing code<br></blockq= uote><div><br></div><div>So what happens if you see one not in the list?</d= iv><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0= px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 2. I also hook devadq using the same interface as I already have done, but<= br> all the attach,detach,nomatch become an event (only in nlsysevent) in the<b= r> "DEVICE" system,<br> the "SUBSYSTEM" is the current what of devaddq<br> <br> The type is changed into:<br> + -> ATTACH<br> - -> DETACH<br> anythingelse -> NOMATCH<br> and the rest of the current line becomes the data<br></blockquote><div><br>= </div><div>This is fine. I kinda think it might not be terrible to expose t= his to devd and have it cope, but that's a zero sum for not a lot of ga= in.</div><div><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0= px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">s= o from nlsysvent point of view this is exactly the same kind of events as t= he<br> one hooked in devctl_notify.<br></blockquote><div><br></div><div>Sure.</div= ><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px= 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 3. in nlsysevent we have one category one can subscribe per known systems.<= br> All other "systems" falls into a new category named "extra&q= uot; "vendor" or "other"<br></blockquote><div><br></div= ><div>So all events that match elements of the array are 'system' a= nd the others are something else?</div><div>=C2=A0</div><blockquote class= =3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg= b(204,204,204);padding-left:1ex"> from the consumer point of view the name of the system is anyway contained = in<br> the message itself, so the category it is subscribed to can differ.<br></bl= ockquote><div><br></div><div>Right. No data is lost...</div><div>=C2=A0</di= v><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;borde= r-left:1px solid rgb(204,204,204);padding-left:1ex"> This way, I should be able to get ALL the events in a consistent way.<br> I should be compatible with people who uses devctl_notify in their custom k= ernel<br> modules.<br></blockquote><div><br></div><div>Yea.=C2=A0</div><div>=C2=A0</d= iv><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bord= er-left:1px solid rgb(204,204,204);padding-left:1ex"> Sounds easy enough without the requirement of complexifying kern_devctl.c w= ith a<br> registration of extra systems.<br></blockquote><div><br></div><div>Yea, I k= inda prefer that... Unless we add too many new systems. It's still extr= a work to add one, but likely not enough extra to be worth the automation.<= /div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px= 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> What do you think?<br></blockquote><div><br></div><div>Not bad.</div><div><= br></div><div>Warner=C2=A0</div></div></div> --0000000000004012d805ef7248a6--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfp6YsxmRNN%2BvGnc8wBjUmOaASvOMXCQYSb8wnK9pBjeiQ>