From nobody Thu Dec 01 15:38:37 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 4NNKx64vxDz4jWCx for ; Thu, 1 Dec 2022 15:38:50 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) (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 4NNKx64MN4z4B74 for ; Thu, 1 Dec 2022 15:38:50 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x536.google.com with SMTP id e13so2854220edj.7 for ; Thu, 01 Dec 2022 07:38:50 -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=4towKsbf7wvyXPsUuhe9LSfTWI9K2qaDmggDI+pF5LE=; b=lo9Om4zymhom4h6s4c19ecTdXHTSRdEY8TeWjDE1m9HTfPCML1t9IlC9jBsL2G04iy sE5LsbevqkDZ0dCoeHlk3AIkp5Pyt37AcxMcnPF/KyQx0M3IYY92ND6W1pzAcYBbXE3q Q6W+YpvMvP8LV2anxWJ++DBx3nkiQ96tDiYgdGv7A5yaks22tDVnwY3+8Zch9zhVKH0G dBaWFzjxvdphMFZjCCv14xKRhayMERteFt+2Zhgrqm0uZqf7F5LhOMO9shfDnXvfODQl W0fnoSoAokwvOYBczN/OpUITHq8M7ahfWBVQbrbwSKC/lYXBIpv94gMLRP6RaBM4i2Nw bgcg== 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=4towKsbf7wvyXPsUuhe9LSfTWI9K2qaDmggDI+pF5LE=; b=RzBviDNa6TTPgdZ7jKoUBUsBGJ09KB/HeghAW4pUew/GOYBxrBj0ABgLuwhkpFqUlO A/LfiSDorqFxdoL5Movig+oHX3hiVz2WWy+COAqKaNHrtlSBfc4cg5vi4FqVc8uk6aYC mdNJLjBLqAiHSlcF2hnRVmVPh2XahefqVfrZEsiZnwG3jPDqWzsz+C11VhbwqOP0KgZ7 /Jm7FO5PCOcYcYXVXfJW6TwNjdGaj/QOfeatkSH05MedqpL2KXuWD6/QMWffqaD4hUGo 4ZrwjKL+rzNJzuzPmsFrGso6h5XhnrMjRnq7RRwSnGvnZwgv9fGq5/4wjDewEhyE3AaA xHIQ== X-Gm-Message-State: ANoB5pkUB9TBNauUaRUnxw2K8On0mjB0lwjNg9Qngo6dnqmCRqPzSZqd WxeI5smm0vE+lb3Gh5+D/M5RyGhFBl2mj1gunGDFJuwEZy5Gig== X-Google-Smtp-Source: AA0mqf5qAllMaL5gQER2VWpmIKdKVJCkd4od2iPEIXKJo8OhCE2roH9ZozJ32hmvexArQ7WT/TFovamnLYgacuMlMzI= X-Received: by 2002:a05:6402:48d:b0:461:c3d9:c6a3 with SMTP id k13-20020a056402048d00b00461c3d9c6a3mr42946609edv.154.1669909129068; Thu, 01 Dec 2022 07:38:49 -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> In-Reply-To: <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu> From: Warner Losh Date: Thu, 1 Dec 2022 08:38:37 -0700 Message-ID: Subject: Re: devctl_notify system is inconsistent To: Baptiste Daroussin Cc: hackers@freebsd.org Content-Type: multipart/alternative; boundary="000000000000721fab05eec604c2" X-Rspamd-Queue-Id: 4NNKx64MN4z4B74 X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --000000000000721fab05eec604c2 Content-Type: text/plain; charset="UTF-8" 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 --000000000000721fab05eec604c2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Dec 1, 2022 at 7:59 AM Baptis= te Daroussin <bapt@freebsd.org&g= t; 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 nee= d 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 g= ot sent
> > via
> > nlsysevent. (https://reviews.freebsd.org/D37573) > >
>
> You're connecting it at the wrong level: it will miss all device e= vents.
> devctl_notify
> is used by everything except newbus's device attach, detach and no= match
> 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 ar= e queued,
or you'll need to add something in devaddq() to dea= l 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).
=C2=A0
>
>
> > 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:<= br> > > Upper case vs lower case
> > "kern" vs "kernel"
> >
>
> They are consistent. With one exception that I deprecated in 13.x to b= e
> 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 whe= n I wrote
> devd
> years ago. These have always been strings, and need to continue to alw= ays 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 t= hat 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&= #39;s a
> bunch of
> #defines, ala old-school X11 resources (which this is very very loosel= y
> 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 h= ave no current mechanism to do that. We could add something that would
register / deregister them with a sysinit=C2=A0+ call to something in= kern_devctl.c which
could do the trick (and also deal with the o= rdering issues), though having netlink(4)
as a loadable modules m= ight be an interesting case to get right.

If we di= d that, we could return a 'token' that you'd use to call a new = version of
devctl_notify(), perhaps with some glue for legacy use= rs (or perhaps not: we change
kernel interfaces all the time, and= could just rename it and convert everything in the
tree).
<= div>
>
>
> > Make the current devctl_notify convert its first argument into th= at enum
> > and
> > yell if an unkwown "system" is passed. (and probably de= clare 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 wild= ly 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 i= t.

I=C2=A0love 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 hopi= ng that someone would replace the hacky thing I did with
a 'r= outing socket'-like interface (I never could figure out hose to do it s= o
many years ago w/o gross hacks). netlink(4) has finally provide= d a way
to do it that doesn't get the routing code all jumble= d up for this.

I just have some specific issues wi= th your proposal. In hindsight, I should
have led with that in my= first message :).

Warner
--000000000000721fab05eec604c2--