From nobody Sat Dec 10 05:09:49 2022 X-Original-To: hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4NTbYR42LTz4kbwj for ; Sat, 10 Dec 2022 05:10:03 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com [IPv6:2a00:1450:4864:20::62d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4NTbYQ3X8Rz3K5y for ; Sat, 10 Dec 2022 05:10:02 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20210112.gappssmtp.com header.s=20210112 header.b=gmQHyZ+T; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2a00:1450:4864:20::62d) smtp.mailfrom=wlosh@bsdimp.com; dmarc=none Received: by mail-ej1-x62d.google.com with SMTP id fc4so15930723ejc.12 for ; Fri, 09 Dec 2022 21:10:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=EPD++tWuKduFjcMLO8Vx9AuJUvycQ2HVvplnWn/c16Q=; b=gmQHyZ+Ti8vxrSJts/KyvSRSsnqNyG7oHVE7qqhNmB+4jvmNOMBMsx6YfdU5FtuahE rqlHi07EdaNizj6YrSOSnR4cRhhKGYWFZPkdGr89nRVIxJMzTrrp5bOVNtkWJM5/5WpQ bdba+sqdemYRuqRjd1XXn1rhXrU81aQNugUNhoMnptOpiUOLFNwa/otQ9wUqMdALm7Q5 gjvWZkxRV0pQOrS5r7cx/DCpXCPAXbDcgmSZwaetneLyU6J+wPHShRtYg+NrUxoil/yp zyUvcLQvkNF5kzWg4/wUSDxutjCwmddOcFbBq+Nv7/n6L6Lcq4kAHsiwJm+mr69FP3VR Qdjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=EPD++tWuKduFjcMLO8Vx9AuJUvycQ2HVvplnWn/c16Q=; b=KGb9c41rVPmWtMSx88ufpanR+avaeYfhHWl4hSyfcWGa4CJ/a5+xFpHp/42gMX8sE7 Fo123FN2fRZFYnBeihLM91czOgTMhDO4GTg8+bau5jw83m70I7XBZcBRPDkENdQZWKtK cEnlPFlYopZRaJ9Xf+Ra+miwzT4CrtdYxSSgfmr806cd2qry1ar+HfCzcmG0XXAgmVvv yM2S8dCFxYldLArv4cVBd6e9ojll0XTL7pWcsmdyNNuFIqxnMYKxUWlWSuZ9cL8vYGzz 7SxV0tGMPRgJhx5TkSDqBkruih7W6zvfMfH+970cXFxrz5he+O+6t2+jcJqmo4lpHuzu 7Rng== X-Gm-Message-State: ANoB5pn0bZVi+nq6CkVJ6HCnZp0nB7uFCKKs8gWN1KAE+0uFZetZ+CKt fmaz0ny2poKopW/fUAhQx9zabpJt7m7VYBi9Fe13mwb049+9fw== X-Google-Smtp-Source: AA0mqf6DCNbS4EMNvqzYMfpA5+qUFBQV1SIkZUUnoX5VaeQLiBekG7yhRyB3+O0RxsyHaoQcxPt0nq2OmcImCPARkLA= X-Received: by 2002:a17:906:f84d:b0:7b9:631b:7dfb with SMTP id ks13-20020a170906f84d00b007b9631b7dfbmr62325151ejb.32.1670649001014; Fri, 09 Dec 2022 21:10:01 -0800 (PST) List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org MIME-Version: 1.0 References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu> <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu> In-Reply-To: <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu> From: Warner Losh Date: Fri, 9 Dec 2022 22:09:49 -0700 Message-ID: Subject: Re: devctl_notify system is inconsistent To: Baptiste Daroussin Cc: hackers@freebsd.org Content-Type: multipart/alternative; boundary="0000000000004012d805ef7248a6" X-Spamd-Result: default: False [-3.00 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20210112.gappssmtp.com:s=20210112]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; MLMMJ_DEST(0.00)[hackers@freebsd.org]; R_SPF_NA(0.00)[no SPF record]; ARC_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::62d:from]; DKIM_TRACE(0.00)[bsdimp-com.20210112.gappssmtp.com:+]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; TO_DN_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_TWO(0.00)[2]; PREVIOUSLY_DELIVERED(0.00)[hackers@freebsd.org]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DMARC_NA(0.00)[bsdimp.com]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Queue-Id: 4NTbYQ3X8Rz3K5y X-Spamd-Bar: -- X-ThisMailContainsUnwantedMimeParts: N --0000000000004012d805ef7248a6 Content-Type: text/plain; charset="UTF-8" On Thu, Dec 1, 2022 at 10:40 AM Baptiste Daroussin 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 > 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 > > > 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


=
On Thu, Dec 1, 2022 at 10:40 AM Bapti= ste Daroussin <bapt@freebsd.org&= gt; wrote:
On Th= u, 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 starte= d 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 witho= ut 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 i= ts event got
> > sent
> > > > via
> > > > nlsysevent. (https://reviews.freebsd.org/D3757= 3)
> > > >
> > >
> > > You're connecting it at the wrong level: it will miss al= l device events.
> > > devctl_notify
> > > is used by everything except newbus's device attach, det= ach 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<= br> > queued,
> or you'll need to add something in devaddq() to deal with the even= ts. These
> are
> a slightly different format than the devctl_notify() events because th= e
> latter was
> added later and I didn't want to cope with transitioning the old f= ormatted
> 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 inco= nsistent:
> > > > 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 dev= d.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 th= at way when I
> > wrote
> > > devd
> > > years ago. These have always been strings, and need to conti= nue 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 b= us.h that's a
> > > bunch of
> > > #defines, ala old-school X11 resources (which this is very v= ery 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 re= gister 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 w= ould
> 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 ha= ving
> 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 t= o call a new
> version of
> devctl_notify(), perhaps with some glue for legacy users (or perhaps n= ot:
> 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 argume= nt into that
> > enum
> > > > and
> > > > yell if an unkwown "system" is passed. (and p= robably declare
> > devctl_notify
> > > > deprecated)
> > > >
> > >
> > > I don't think this buys us anything. devctl_notify canno= t possibly know
> > > about all
> > > the possible subsystems, so this is likely doomed to high ma= intenance
> > 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 feed= back when I
> > changed
> > > "kern" to "kernel" last year for power e= vents. 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 forg= et about
> > it.
> >
>
> 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.
> I've been hoping that someone would replace the hacky thing I did = with
> a 'routing socket'-like interface (I never could figure out ho= se 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 s= hould
> 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 {
=C2=A0DEVCTL_SYSTEM_ACPI =3D 0,
=C2=A0DEVCTL_SYSTEM_AEON =3D 1,
=C2=A0...
=C2=A0DEVCTL_SYSTEM_ZFS =3D 19
};

static const char *devctl_systems[] =3D {
=C2=A0 "ACPI",
=C2=A0 "AEON",
=C2=A0 ...
=C2=A0 "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?
=C2=A0
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 "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 t= his to devd and have it cope, but that's a zero sum for not a lot of ga= in.

s= o from nlsysvent point of view this is exactly the same kind of events as t= he
one hooked in devctl_notify.

Sure.
=C2=A0
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"

So all events that match elements of the array are 'system' a= nd the others are something else?
=C2=A0
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...
=C2=A0
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 k= ernel
modules.

Yea.=C2=A0
=C2=A0
Sounds easy enough without the requirement of complexifying kern_devctl.c w= ith a
registration of extra systems.

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>
=C2=A0
What do you think?

Not bad.
<= br>
Warner=C2=A0
--0000000000004012d805ef7248a6--