[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Peter Korsgaard]

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(+)

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

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Andy Shevchenko]

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
> 

-- 
With Best Regards,
Andy Shevchenko

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Arnout Vandecappelle]

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))
> 

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Andy Shevchenko]

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...).

-- 
With Best Regards,
Andy Shevchenko

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Peter Korsgaard]

>>>>> "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 at 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.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Arnout Vandecappelle]

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 at 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

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Titouan Christophe]

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 at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
> 

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Arnout Vandecappelle]

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

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Peter Korsgaard]

>>>>> "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.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Titouan Christophe]

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

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Peter Korsgaard]

>>>>> "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.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Titouan Christophe]

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

[Buildroot] [PATCH] linux: enable UEVENT_HELPER when mdev is used

[Peter Korsgaard]

>>>>> "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!

-- 
Bye, Peter Korsgaard