From nobody Fri Dec 09 15:25:47 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 4NTFGj5WhSz4jY8m for ; Fri, 9 Dec 2022 15:26:05 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from forward500p.mail.yandex.net (forward500p.mail.yandex.net [77.88.28.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4NTFGg74xBz4Hpx; Fri, 9 Dec 2022 15:26:03 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=ipfw.ru header.s=mail header.b=uTPf6XPa; spf=pass (mx1.freebsd.org: domain of melifaro@ipfw.ru designates 77.88.28.110 as permitted sender) smtp.mailfrom=melifaro@ipfw.ru; dmarc=none Received: from vla5-91e5293da019.qloud-c.yandex.net (vla5-91e5293da019.qloud-c.yandex.net [IPv6:2a02:6b8:c18:3e1f:0:640:91e5:293d]) by forward500p.mail.yandex.net (Yandex) with ESMTP id 16E39F01550; Fri, 9 Dec 2022 18:26:00 +0300 (MSK) Received: by vla5-91e5293da019.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id wPcD1p1ZA8c1-7htvkttE; Fri, 09 Dec 2022 18:25:59 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfw.ru; s=mail; t=1670599559; bh=YqNTFgv7SelG/Re2Dms4R8TM25cZ5pLEp0PYkMqVsr4=; h=Message-Id:To:Date:References:Cc:In-Reply-To:From:Subject; b=uTPf6XPacDLYqWlhWJDiGHJvcDmdlgdVEXkUij42TyEjZsumSqgZAcJ8bQuH9Eju6 jb6zc61T3sRh68MHhj7xw3bydAnz2a5W1MeI8kU0iN9rHRT2FxbufMUF/lsbQg15T0 VOvw6HaKbYYVFCLBcMt+VafmriRvsu2ooXmmKjWA= Content-Type: text/plain; charset=utf-8 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 (Mac OS X Mail 16.0 \(3731.200.110.1.12\)) Subject: Re: devctl_notify system is inconsistent From: "Alexander V. Chernikov" In-Reply-To: <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu> Date: Fri, 9 Dec 2022 15:25:47 +0000 Cc: Warner Losh , hackers@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu> <20221201174043.5yqtfbs6p63bdgqp@aniel.nours.eu> To: Baptiste Daroussin X-Mailer: Apple Mail (2.3731.200.110.1.12) 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]; MV_CASE(0.50)[]; R_SPF_ALLOW(-0.20)[+ip4:77.88.28.0/24]; R_DKIM_ALLOW(-0.20)[ipfw.ru:s=mail]; MIME_GOOD(-0.10)[text/plain]; MLMMJ_DEST(0.00)[hackers@freebsd.org]; RCVD_VIA_SMTP_AUTH(0.00)[]; MIME_TRACE(0.00)[0:+]; FROM_EQ_ENVFROM(0.00)[]; ASN(0.00)[asn:13238, ipnet:77.88.0.0/18, country:RU]; RCVD_TLS_LAST(0.00)[]; DKIM_TRACE(0.00)[ipfw.ru:+]; TO_DN_SOME(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; FREEFALL_USER(0.00)[melifaro]; ARC_NA(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; DMARC_NA(0.00)[ipfw.ru]; RCPT_COUNT_THREE(0.00)[3]; MID_RHS_MATCH_FROM(0.00)[]; RWL_MAILSPIKE_POSSIBLE(0.00)[77.88.28.110:from] X-Rspamd-Queue-Id: 4NTFGg74xBz4Hpx X-Spamd-Bar: -- X-ThisMailContainsUnwantedMimeParts: N > On 1 Dec 2022, at 17:40, Baptiste Daroussin wrote: >=20 > 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: >>=20 >>> 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: >>>>=20 >>>>> Hello, >>>>>=20 >>>>> 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. >>>>>=20 >>>>> The goal is to be able to have multiple consumers without the need = of >>> devd >>>>> to be >>>>> running. >>>>>=20 >>>>> The goal is also to be able subscribe to the events the consumer = is >>>>> willing to >>>>> receive. >>>>>=20 >>>>> https://reviews.freebsd.org/D37574 >>>>>=20 >>>>> I also added a hook to devctl_notify to make sure all its event = got >>> sent >>>>> via >>>>> nlsysevent. (https://reviews.freebsd.org/D37573) >>>>>=20 >>>>=20 >>>> 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. >>>=20 >>> Where should I hook to get the device events? >>>=20 >>=20 >> 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). >>=20 >>=20 >>>>=20 >>>>=20 >>>>> 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" >>>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>>=20 >>>>> I intent to fix the following way: >>>>> Create a new function similar to devctl_notify but with the first >>> argument >>>>> being >>>>> an enum. >>>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> 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. >>>>=20 >>>> 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. >>>=20 >>> 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. >>>=20 >>> But I need a way to list them all. >>>=20 >>=20 >> 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. >>=20 >> 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). >>=20 >>>=20 >>>>=20 >>>>> 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) >>>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> Then fix the inconsistencies: all upper case as it seems the most = wildly >>> use >>>>> case >>>>> s/kern/kernel/g >>>>>=20 >>>>> WDYT? >>>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> Warner >>>=20 >>>=20 >>> Note that if you think nlsysevent is a bad idea, I will just forget = about >>> it. >>>=20 >>=20 >> 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. >>=20 >> I just have some specific issues with your proposal. In hindsight, I = should >> have led with that in my first message :). >>=20 >> Warner >=20 > Here is my new proposal: >=20 > What about: >=20 > 1. I add a static list of systems in sys/devctl.h something like >=20 > enum { > DEVCTL_SYSTEM_ACPI =3D 0, > DEVCTL_SYSTEM_AEON =3D 1, > ... > DEVCTL_SYSTEM_ZFS =3D 19 > }; >=20 > static const char *devctl_systems[] =3D { > "ACPI", > "AEON", > ... > "ZFS", > }; >=20 > this way we have a list of official freebsd's systems. > We don't change the devctl_notify interface >=20 > and in the kernel we change the devctl_notify("ZFS" into > devctl_notify(devctl_systems[DEVCTL_SYSTEM_ZFS],... >=20 > This is not too intrusive, and breaks none of the existing code >=20 > 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 >=20 > The type is changed into: > + -> ATTACH > - -> DETACH > anythingelse -> NOMATCH > and the reste of the current line becomes the data >=20 > so from nlsysvent point of view this is exactly the same kind of = events as the > one hooked in devctl_notify. >=20 > 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" >=20 > 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. >=20 > 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. >=20 > Sounds easy enough without the requirement of complexifying = kern_devctl.c with a > registration of extra systems. >=20 > What do you think? My 2 cents: I=E2=80=99d look at the groups from the customer (e.g. userland API = POV). IMHO, fine-grained groups serves 2 purposes. The first is limiting the amount of notifications the app has to deal = with. The second is moving filtering from the application to the kernel, so the app doesn=E2=80=99t need to check the = event=E2=80=99s subsystem. Note that the second part loses its value if = the app subscribes for more than one group. It is also worth noting that _some_ notifications, like ifnet event, may = be high-volume, so it can be desirable to have a way to filter them out. For example, it can be implemented as a fixed number of broad group = categories (=E2=80=9Cnet=E2=80=9D, =E2=80=9Cdevice=E2=80=9D, =E2=80=9Cfs=E2= =80=9D, =E2=80=9Cpower=E2=80=9D, =E2=80=9Cvendor=E2=80=9D) and each = subsystem picks one it belongs to. It can be potentially extended with an ability to register custom groups = if so desired. I wouldn=E2=80=99t say the existing KPI consisting of a single = devctl_notify() is set in stone. I=E2=80=99d actually vote to have a new = function/set of functions, as the notifications are no longer limited to = the devices, as devctl suggests. For example (just to illustrate the point): group_id =3D unotify_get_group(=E2=80=9Cpower=E2=80=9D) # gets existing = or registers new group in the netlink subsystem unotify_broadcast(group_id, =E2=80=9CACPI=E2=80=9D, =E2=80=A6) unotify_free_group(group_id) >=20 > Best regards, > Bapt >=20