| Message ID | 20190525130755.25678-1-peter@korsgaard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | linux: enable UEVENT_HELPER when mdev is used | expand |
On Sat, May 25, 2019 at 03:07:55PM +0200, Peter Korsgaard wrote: > S10mdev uses /proc/sys/kernel/hotplug, which is only available if > CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. > Thanks! > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > linux/linux.mk | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/linux/linux.mk b/linux/linux.mk > index dd182d06b2..3a5eee63df 100644 > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -333,6 +333,8 @@ define LINUX_KCONFIG_FIXUP_CMDS > $(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS_MOUNT,$(@D)/.config)) > $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV), > $(call KCONFIG_ENABLE_OPT,CONFIG_INOTIFY_USER,$(@D)/.config)) > + $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV), > + $(call KCONFIG_ENABLE_OPT,CONFIG_UEVENT_HELPER,$(@D)/.config)) > $(if $(BR2_PACKAGE_AUDIT), > $(call KCONFIG_ENABLE_OPT,CONFIG_NET,$(@D)/.config) > $(call KCONFIG_ENABLE_OPT,CONFIG_AUDIT,$(@D)/.config)) > -- > 2.11.0 >
On 25/05/2019 15:07, Peter Korsgaard wrote: > S10mdev uses /proc/sys/kernel/hotplug, which is only available if > CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. So, I take a look at it, and it's a little bit more complicated... Busybox *does* have support for the "new" netlink-based uevent interface, but that interface needs a daemon that keeps running and they separated that daemon into a separate process, called uevent. So, in the "new" way, you should do something like (not tried, just based on source code): start-stop-daemon -S -b -x /sbin/uevent -- mdev But of course, that means that we have to be sure that the uevent executable is in fact built for busybox. So we'd need to extend BUSYBOX_SET_MDEV to enable uevent. Also, we can only use the uevent approach if netlink uevents are available. They exist since 2.6.12, so I guess on that side we're safe. However, they also depend on CONFIG_NET (because netlink does). So, these are our options: 1. Bloat the kernel with legacy uevent helper support (this patch). 2. Bloat the kernel with networking even if it is not used for anything else, and bloat busybox with uevent (3.1 kb), and always use uevent. 3. Use uevent in the init script but don't enforce anything (with potential silent failures), and explain stuff in the mdev help text. 4. Try out all possibilities in the init script - which still fails silently in case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. In my opinion, we definitely should *not* go for option 1. I don't want to be the one that forces the kernel to maintain legacy stuff. I would definitely use uevent. On the other hand, I really don't like it when we force changes into busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled in the kernel, and since our default busybox config (but not the minimal one) does enable uevent, I think that option 3 is the way to go. By the way, with netlink, the event are properly serialized, and uevent makes sure that only one mdev subprocess is created so it maintains the serialisation. So, it is no longer needed to create /dev/mdev.seq (which, for some reason, we currently don't do...). Regards, Arnout > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > linux/linux.mk | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/linux/linux.mk b/linux/linux.mk > index dd182d06b2..3a5eee63df 100644 > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -333,6 +333,8 @@ define LINUX_KCONFIG_FIXUP_CMDS > $(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS_MOUNT,$(@D)/.config)) > $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV), > $(call KCONFIG_ENABLE_OPT,CONFIG_INOTIFY_USER,$(@D)/.config)) > + $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV), > + $(call KCONFIG_ENABLE_OPT,CONFIG_UEVENT_HELPER,$(@D)/.config)) > $(if $(BR2_PACKAGE_AUDIT), > $(call KCONFIG_ENABLE_OPT,CONFIG_NET,$(@D)/.config) > $(call KCONFIG_ENABLE_OPT,CONFIG_AUDIT,$(@D)/.config)) >
On Sat, May 25, 2019 at 05:24:15PM +0200, Arnout Vandecappelle wrote: > On 25/05/2019 15:07, Peter Korsgaard wrote: > > S10mdev uses /proc/sys/kernel/hotplug, which is only available if > > CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. > > So, I take a look at it, and it's a little bit more complicated... > > Busybox *does* have support for the "new" netlink-based uevent interface, but > that interface needs a daemon that keeps running and they separated that daemon > into a separate process, called uevent. So, in the "new" way, you should do > something like (not tried, just based on source code): > > start-stop-daemon -S -b -x /sbin/uevent -- mdev > > But of course, that means that we have to be sure that the uevent executable is > in fact built for busybox. So we'd need to extend BUSYBOX_SET_MDEV to enable uevent. > > Also, we can only use the uevent approach if netlink uevents are available. > They exist since 2.6.12, so I guess on that side we're safe. However, they also > depend on CONFIG_NET (because netlink does). > > So, these are our options: > > 1. Bloat the kernel with legacy uevent helper support (this patch). > > 2. Bloat the kernel with networking even if it is not used for anything else, > and bloat busybox with uevent (3.1 kb), and always use uevent. > > 3. Use uevent in the init script but don't enforce anything (with potential > silent failures), and explain stuff in the mdev help text. > > 4. Try out all possibilities in the init script - which still fails silently in > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. Since the UEVENT_HELPER had been made optional back in 2014 and I reported bug for mdev script in 2019 I would *speculate* that everybody nowadays are using udev, so, netlink is quite probably already enabled in the kernel. > In my opinion, we definitely should *not* go for option 1. I don't want to be > the one that forces the kernel to maintain legacy stuff. I second this. > I would definitely use > uevent. On the other hand, I really don't like it when we force changes into > busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled > in the kernel, and since our default busybox config (but not the minimal one) > does enable uevent, I think that option 3 is the way to go. Agree. > By the way, with netlink, the event are properly serialized, and uevent makes > sure that only one mdev subprocess is created so it maintains the serialisation. > So, it is no longer needed to create /dev/mdev.seq (which, for some reason, we > currently don't do...).
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > On 25/05/2019 15:07, Peter Korsgaard wrote: >> S10mdev uses /proc/sys/kernel/hotplug, which is only available if >> CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. > So, I take a look at it, and it's a little bit more complicated... > Busybox *does* have support for the "new" netlink-based uevent interface, but > that interface needs a daemon that keeps running and they separated that daemon > into a separate process, called uevent. So, in the "new" way, you should do > something like (not tried, just based on source code): > start-stop-daemon -S -b -x /sbin/uevent -- mdev Ahh, so busybox now has a clone of s6-uevent-listener / mdevd-netlink? I wasn't aware of that. There is also patches floating for a daemon mode for mdev: https://www.mail-archive.com/busybox@busybox.net/msg25938.html and separately, mdevd exists: https://skarnet.org/software/mdevd/ > So, these are our options: > 1. Bloat the kernel with legacy uevent helper support (this patch). This is not really bloat, as it just makes the existing implicit dependency explicit. > 2. Bloat the kernel with networking even if it is not used for anything else, > and bloat busybox with uevent (3.1 kb), and always use uevent. Networking support adds significant bloat, the other options afaik not. > 3. Use uevent in the init script but don't enforce anything (with potential > silent failures), and explain stuff in the mdev help text. > 4. Try out all possibilities in the init script - which still fails silently in > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. > In my opinion, we definitely should *not* go for option 1. I don't want to be > the one that forces the kernel to maintain legacy stuff. I would definitely use > uevent. What advantage does uevent have over the legacy uevent helper? Just serialization or anything else? > On the other hand, I really don't like it when we force changes into > busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled > in the kernel, and since our default busybox config (but not the minimal one) > does enable uevent, I think that option 3 is the way to go. I don't use mdev myself, so I don't really care a lot about what option we go for longer term, but given how close 2019.05 is, the only valid options for the release are this patch or nothing. I have no problems with patches (for next) moving our mdev logic to netdev instead if people are interested. > By the way, with netlink, the event are properly serialized, and uevent makes > sure that only one mdev subprocess is created so it maintains the serialisation. > So, it is no longer needed to create /dev/mdev.seq (which, for some reason, we > currently don't do...). Ahh yes, a patch adding an 'echo >/dev/mdev.seq' to S10mdev would be nice.
On 25/05/2019 21:56, Peter Korsgaard wrote: >>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > > > On 25/05/2019 15:07, Peter Korsgaard wrote: > >> S10mdev uses /proc/sys/kernel/hotplug, which is only available if > >> CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. > > > So, I take a look at it, and it's a little bit more complicated... > > > Busybox *does* have support for the "new" netlink-based uevent interface, but > > that interface needs a daemon that keeps running and they separated that daemon > > into a separate process, called uevent. So, in the "new" way, you should do > > something like (not tried, just based on source code): > > > start-stop-daemon -S -b -x /sbin/uevent -- mdev > > Ahh, so busybox now has a clone of s6-uevent-listener / mdevd-netlink? I > wasn't aware of that. It has been added in 2015 :-) > > There is also patches floating for a daemon mode for mdev: > https://www.mail-archive.com/busybox@busybox.net/msg25938.html > and separately, mdevd exists: https://skarnet.org/software/mdevd/ > > > So, these are our options: > > > 1. Bloat the kernel with legacy uevent helper support (this patch). > > This is not really bloat, as it just makes the existing implicit > dependency explicit. It is "bloat" because there is a way to avoid it, but the user can't disable it (even if the user modifies the init script to use uevent so it becomes unneeded, Buildroot forces the kernel option on). > > 2. Bloat the kernel with networking even if it is not used for anything else, > > and bloat busybox with uevent (3.1 kb), and always use uevent. > > Networking support adds significant bloat, the other options afaik not. Note that it's just CONFIG_NET, not CONFIG_INET. Without CONFIG_NET, you don't have e.g. the socket() syscall. 90% of the packages will not work (at runtime). So we're kind of in the same domain as with a custom uClibc config that disables stuff that most packages depend on. > > 3. Use uevent in the init script but don't enforce anything (with potential > > silent failures), and explain stuff in the mdev help text. > > > 4. Try out all possibilities in the init script - which still fails silently in > > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. > > > In my opinion, we definitely should *not* go for option 1. I don't want to be > > the one that forces the kernel to maintain legacy stuff. I would definitely use > > uevent. > > What advantage does uevent have over the legacy uevent helper? Just > serialization or anything else? It's also more efficient IIUC, but I don't know much of the details. Admittedly, that efficiency probably goes away with uevent, because it stems from avoiding a fork, but uevent does exactly that. > > On the other hand, I really don't like it when we force changes into > > busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled > > in the kernel, and since our default busybox config (but not the minimal one) > > does enable uevent, I think that option 3 is the way to go. > > I don't use mdev myself, so I don't really care a lot about what option > we go for longer term, but given how close 2019.05 is, the only valid > options for the release are this patch or nothing. As Andy indicated, this bug has existed for several years. I really don't see the need to fix it for 2019.05. > I have no problems with patches (for next) moving our mdev logic to > netdev instead if people are interested. > > > > By the way, with netlink, the event are properly serialized, and uevent makes > > sure that only one mdev subprocess is created so it maintains the serialisation. > > So, it is no longer needed to create /dev/mdev.seq (which, for some reason, we > > currently don't do...). > > Ahh yes, a patch adding an 'echo >/dev/mdev.seq' to S10mdev would be > nice. But not needed if we use uevent :-) Regards, Arnout
Hello Arnout, Peter, Andy and all, On 5/25/19 5:24 PM, Arnout Vandecappelle wrote: > > > On 25/05/2019 15:07, Peter Korsgaard wrote: >> S10mdev uses /proc/sys/kernel/hotplug, which is only available if >> CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. > > So, I take a look at it, and it's a little bit more complicated... > > Busybox *does* have support for the "new" netlink-based uevent interface, but > that interface needs a daemon that keeps running and they separated that daemon > into a separate process, called uevent. So, in the "new" way, you should do > something like (not tried, just based on source code): > > start-stop-daemon -S -b -x /sbin/uevent -- mdev > > But of course, that means that we have to be sure that the uevent executable is > in fact built for busybox. So we'd need to extend BUSYBOX_SET_MDEV to enable uevent. > > Also, we can only use the uevent approach if netlink uevents are available. > They exist since 2.6.12, so I guess on that side we're safe. However, they also > depend on CONFIG_NET (because netlink does). > > So, these are our options: > > 1. Bloat the kernel with legacy uevent helper support (this patch). > > 2. Bloat the kernel with networking even if it is not used for anything else, > and bloat busybox with uevent (3.1 kb), and always use uevent. > > 3. Use uevent in the init script but don't enforce anything (with potential > silent failures), and explain stuff in the mdev help text. > > 4. Try out all possibilities in the init script - which still fails silently in > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. > > > In my opinion, we definitely should *not* go for option 1. I don't want to be > the one that forces the kernel to maintain legacy stuff. I would definitely use > uevent. On the other hand, I really don't like it when we force changes into > busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled > in the kernel, and since our default busybox config (but not the minimal one) > does enable uevent, I think that option 3 is the way to go. Couldn't we implement the following in S10mdev ? - If both /proc/net/netlink and /sbin/uevent are present -> uevent daemon + mdev - If /proc/sys/kernel/hotplug is present -> go for mdev as hotplug (and init mdev.seq) - Otherwise -> print an error message. I have a patch for this, would I submit ? Best regards, Titouan > > > By the way, with netlink, the event are properly serialized, and uevent makes > sure that only one mdev subprocess is created so it maintains the serialisation. > So, it is no longer needed to create /dev/mdev.seq (which, for some reason, we > currently don't do...). > > Regards, > Arnout > > >> >> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> >> --- >> linux/linux.mk | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/linux/linux.mk b/linux/linux.mk >> index dd182d06b2..3a5eee63df 100644 >> --- a/linux/linux.mk >> +++ b/linux/linux.mk >> @@ -333,6 +333,8 @@ define LINUX_KCONFIG_FIXUP_CMDS >> $(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS_MOUNT,$(@D)/.config)) >> $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV), >> $(call KCONFIG_ENABLE_OPT,CONFIG_INOTIFY_USER,$(@D)/.config)) >> + $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV), >> + $(call KCONFIG_ENABLE_OPT,CONFIG_UEVENT_HELPER,$(@D)/.config)) >> $(if $(BR2_PACKAGE_AUDIT), >> $(call KCONFIG_ENABLE_OPT,CONFIG_NET,$(@D)/.config) >> $(call KCONFIG_ENABLE_OPT,CONFIG_AUDIT,$(@D)/.config)) >> > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
On 29/05/2019 15:13, Titouan Christophe wrote: > Hello Arnout, Peter, Andy and all, > > On 5/25/19 5:24 PM, Arnout Vandecappelle wrote: >> >> >> On 25/05/2019 15:07, Peter Korsgaard wrote: >>> S10mdev uses /proc/sys/kernel/hotplug, which is only available if >>> CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. >> >> So, I take a look at it, and it's a little bit more complicated... >> >> Busybox *does* have support for the "new" netlink-based uevent interface, but >> that interface needs a daemon that keeps running and they separated that daemon >> into a separate process, called uevent. So, in the "new" way, you should do >> something like (not tried, just based on source code): >> >> start-stop-daemon -S -b -x /sbin/uevent -- mdev >> >> But of course, that means that we have to be sure that the uevent executable is >> in fact built for busybox. So we'd need to extend BUSYBOX_SET_MDEV to enable >> uevent. >> >> Also, we can only use the uevent approach if netlink uevents are available. >> They exist since 2.6.12, so I guess on that side we're safe. However, they also >> depend on CONFIG_NET (because netlink does). >> >> So, these are our options: >> >> 1. Bloat the kernel with legacy uevent helper support (this patch). >> >> 2. Bloat the kernel with networking even if it is not used for anything else, >> and bloat busybox with uevent (3.1 kb), and always use uevent. >> >> 3. Use uevent in the init script but don't enforce anything (with potential >> silent failures), and explain stuff in the mdev help text. >> >> 4. Try out all possibilities in the init script - which still fails silently in >> case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. >> >> >> In my opinion, we definitely should *not* go for option 1. I don't want to be >> the one that forces the kernel to maintain legacy stuff. I would definitely use >> uevent. On the other hand, I really don't like it when we force changes into >> busybox or kernel configs. Since it's really likely that CONFIG_NET is enabled >> in the kernel, and since our default busybox config (but not the minimal one) >> does enable uevent, I think that option 3 is the way to go. > > Couldn't we implement the following in S10mdev ? > > - If both /proc/net/netlink and /sbin/uevent are present -> uevent daemon + mdev > - If /proc/sys/kernel/hotplug is present -> go for mdev as hotplug (and init > mdev.seq) > - Otherwise -> print an error message. > > > I have a patch for this, would I submit ? If you anyway have it, submit it, and we can discuss based on that. It would be nice if you could also do a preparatory patch that converts the file to the "canonical init script format" (cfr. e.g. package/rsyslog/S01rsyslogd). mdev doesn't have arguments or a default, so that simplifies things a little. Regards, Arnout
Hello, On Wed, 29 May 2019 15:57:22 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > If you anyway have it, submit it, and we can discuss based on that. Titouan has submitted his patch: http://patchwork.ozlabs.org/patch/1107175/ The commit title is probably not the best, but other than that, the idea looks good to me. > It would be nice if you could also do a preparatory patch that converts the > file to the "canonical init script format" (cfr. e.g. > package/rsyslog/S01rsyslogd). mdev doesn't have arguments or a default, so that > simplifies things a little. Since the change to S10mdev from Titouan is kind of a fix, I would actually not make the conversion to the "canonical format" a requirement that should be done as a preparatory patch, but that's admittedly a minor detail. Thomas
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >> > start-stop-daemon -S -b -x /sbin/uevent -- mdev >> >> Ahh, so busybox now has a clone of s6-uevent-listener / mdevd-netlink? I >> wasn't aware of that. > It has been added in 2015 :-) As I said, I don't use mdev ;) >> > 1. Bloat the kernel with legacy uevent helper support (this patch). >> >> This is not really bloat, as it just makes the existing implicit >> dependency explicit. > It is "bloat" because there is a way to avoid it, but the user can't disable it > (even if the user modifies the init script to use uevent so it becomes unneeded, > Buildroot forces the kernel option on). Correct. >> > 2. Bloat the kernel with networking even if it is not used for anything else, >> > and bloat busybox with uevent (3.1 kb), and always use uevent. >> >> Networking support adds significant bloat, the other options afaik not. > Note that it's just CONFIG_NET, not CONFIG_INET. Without CONFIG_NET, you don't > have e.g. the socket() syscall. 90% of the packages will not work (at runtime). > So we're kind of in the same domain as with a custom uClibc config that disables > stuff that most packages depend on. How will the uevent daemon work without socket()? >> > 3. Use uevent in the init script but don't enforce anything (with potential >> > silent failures), and explain stuff in the mdev help text. >> >> > 4. Try out all possibilities in the init script - which still fails silently in >> > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. >> >> > In my opinion, we definitely should *not* go for option 1. I don't want to be >> > the one that forces the kernel to maintain legacy stuff. I would definitely use >> > uevent. >> >> What advantage does uevent have over the legacy uevent helper? Just >> serialization or anything else? > It's also more efficient IIUC, but I don't know much of the details. > Admittedly, that efficiency probably goes away with uevent, because it stems > from avoiding a fork, but uevent does exactly that. Indeed. Notice that Denys has just merged the daemon mode (-d) support for mdev, so from a performance PoV, that is probably what we should support rather than uevent + mdev: http://lists.busybox.net/pipermail/busybox/2019-June/087305.html >> I don't use mdev myself, so I don't really care a lot about what option >> we go for longer term, but given how close 2019.05 is, the only valid >> options for the release are this patch or nothing. > As Andy indicated, this bug has existed for several years. I really don't see > the need to fix it for 2019.05. .. so I didn't. >> Ahh yes, a patch adding an 'echo >/dev/mdev.seq' to S10mdev would be >> nice. > But not needed if we use uevent :-) Not if we unconditionally do it, no.
Hi all, On 6/3/19 4:56 PM, Peter Korsgaard wrote: >>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > > Hi, > > >> > start-stop-daemon -S -b -x /sbin/uevent -- mdev > >> > >> Ahh, so busybox now has a clone of s6-uevent-listener / mdevd-netlink? I > >> wasn't aware of that. > > > It has been added in 2015 :-) > > As I said, I don't use mdev ;) > > >> > 1. Bloat the kernel with legacy uevent helper support (this patch). > >> > >> This is not really bloat, as it just makes the existing implicit > >> dependency explicit. > > > It is "bloat" because there is a way to avoid it, but the user can't disable it > > (even if the user modifies the init script to use uevent so it becomes unneeded, > > Buildroot forces the kernel option on). > > Correct. > > > >> > 2. Bloat the kernel with networking even if it is not used for anything else, > >> > and bloat busybox with uevent (3.1 kb), and always use uevent. > >> > >> Networking support adds significant bloat, the other options afaik not. > > > Note that it's just CONFIG_NET, not CONFIG_INET. Without CONFIG_NET, you don't > > have e.g. the socket() syscall. 90% of the packages will not work (at runtime). > > So we're kind of in the same domain as with a custom uClibc config that disables > > stuff that most packages depend on. > > How will the uevent daemon work without socket()? > > >> > 3. Use uevent in the init script but don't enforce anything (with potential > >> > silent failures), and explain stuff in the mdev help text. > >> > >> > 4. Try out all possibilities in the init script - which still fails silently in > >> > case neither CONFIG_UEVENT_HELPER nor CONFIG_NET is enabled in the kernel. > >> > >> > In my opinion, we definitely should *not* go for option 1. I don't want to be > >> > the one that forces the kernel to maintain legacy stuff. I would definitely use > >> > uevent. > >> > >> What advantage does uevent have over the legacy uevent helper? Just > >> serialization or anything else? > > > It's also more efficient IIUC, but I don't know much of the details. > > Admittedly, that efficiency probably goes away with uevent, because it stems > > from avoiding a fork, but uevent does exactly that. > > Indeed. Notice that Denys has just merged the daemon mode (-d) support > for mdev, so from a performance PoV, that is probably what we should > support rather than uevent + mdev: > > http://lists.busybox.net/pipermail/busybox/2019-June/087305.html This is great news ! Even then, the S10mdev init script has to be modified to start mdev in daemon mode. I could add this to http://patchwork.ozlabs.org/patch/1107175/. However, I don't see a good way to check if FEATURE_MDEV_DAEMON=y. I could try to search for the daemon option in the help text (`mdev 2>&1 | grep "runs mdev as daemon"`), but this look a bit hackish to me. > > > >> I don't use mdev myself, so I don't really care a lot about what option > >> we go for longer term, but given how close 2019.05 is, the only valid > >> options for the release are this patch or nothing. > > > As Andy indicated, this bug has existed for several years. I really don't see > > the need to fix it for 2019.05. > > .. so I didn't. > > > >> Ahh yes, a patch adding an 'echo >/dev/mdev.seq' to S10mdev would be > >> nice. > > > But not needed if we use uevent :-) > > Not if we unconditionally do it, no. > Regards, Titouan
>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: Hi, >> Indeed. Notice that Denys has just merged the daemon mode (-d) support >> for mdev, so from a performance PoV, that is probably what we should >> support rather than uevent + mdev: >> >> http://lists.busybox.net/pipermail/busybox/2019-June/087305.html > This is great news ! > Even then, the S10mdev init script has to be modified to start mdev in > daemon mode. I could add this to > http://patchwork.ozlabs.org/patch/1107175/. However, I don't see a > good way to check if FEATURE_MDEV_DAEMON=y. We would just forcibly enable that option when building busybox when BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV is enabled, similar to the other mdev options.
Hello, On 6/18/19 9:32 PM, Peter Korsgaard wrote: >>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: > > Hi, > > >> Indeed. Notice that Denys has just merged the daemon mode (-d) support > >> for mdev, so from a performance PoV, that is probably what we should > >> support rather than uevent + mdev: > >> > >> http://lists.busybox.net/pipermail/busybox/2019-June/087305.html > > > This is great news ! > > > Even then, the S10mdev init script has to be modified to start mdev in > > daemon mode. I could add this to > > http://patchwork.ozlabs.org/patch/1107175/. However, I don't see a > > good way to check if FEATURE_MDEV_DAEMON=y. > > We would just forcibly enable that option when building busybox when > BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV is enabled, similar to the other > mdev options. > IMHO this patch can now be archived, since this series has been applied: http://patchwork.ozlabs.org/project/buildroot/list/?series=114874 . Also, this last patch http://patchwork.ozlabs.org/patch/1119590/ would be needed to make sure mdev daemon mode can always be used. Regards, Titouan
>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: Hi, > IMHO this patch can now be archived, since this series has been > applied: > http://patchwork.ozlabs.org/project/buildroot/list/?series=114874 . Agreed, marked as rejected. > Also, this last patch http://patchwork.ozlabs.org/patch/1119590/ would > be needed to make sure mdev daemon mode can always be used. And I have applied this, thanks!
diff --git a/linux/linux.mk b/linux/linux.mk index dd182d06b2..3a5eee63df 100644 --- a/linux/linux.mk +++ b/linux/linux.mk @@ -333,6 +333,8 @@ define LINUX_KCONFIG_FIXUP_CMDS $(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS_MOUNT,$(@D)/.config)) $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV), $(call KCONFIG_ENABLE_OPT,CONFIG_INOTIFY_USER,$(@D)/.config)) + $(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV), + $(call KCONFIG_ENABLE_OPT,CONFIG_UEVENT_HELPER,$(@D)/.config)) $(if $(BR2_PACKAGE_AUDIT), $(call KCONFIG_ENABLE_OPT,CONFIG_NET,$(@D)/.config) $(call KCONFIG_ENABLE_OPT,CONFIG_AUDIT,$(@D)/.config))
S10mdev uses /proc/sys/kernel/hotplug, which is only available if CONFIG_UEVENT_HELPER is enabled in the kernel, so ensure it is. Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> --- linux/linux.mk | 2 ++ 1 file changed, 2 insertions(+)