Message ID | 1452599804-27284-1-git-send-email-lkundrak@v3.sk |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 16-01-12 06:56 AM, Lubomir Rintel wrote: > The devices can be created at run-time for quite some time already and the > load-time device creation collides with attempts to create the device of > the same name: > > # rmmod ifb > # ip link add ifb0 type ifb > RTNETLINK answers: File exists > > This is pretty much the same situation as was with the block loop devices > which was solved by adding a build-time configuration that the > distributions could use as they deem fit while keeping the default for > compatibility. > > Let's do that here as well. > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> I guess module options are frowned upon. so: Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
On Tue, Jan 12, 2016 at 12:56 PM, Lubomir Rintel <lkundrak@v3.sk> wrote: > The devices can be created at run-time for quite some time already and the > load-time device creation collides with attempts to create the device of > the same name: > > # rmmod ifb > # ip link add ifb0 type ifb > RTNETLINK answers: File exists > > This is pretty much the same situation as was with the block loop devices > which was solved by adding a build-time configuration that the > distributions could use as they deem fit while keeping the default for > compatibility. > > Let's do that here as well. > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> This has been a nuisance, so would be great to see it fixed upstream. Acked-by: Tom Gundersen <teg@jklm.no> > --- > drivers/net/Kconfig | 21 +++++++++++++++++---- > drivers/net/ifb.c | 2 +- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index f184fb5..63535b4 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -110,12 +110,25 @@ config IFB > This is an intermediate driver that allows sharing of > resources. > To compile this driver as a module, choose M here: the module > - will be called ifb. If you want to use more than one ifb > - device at a time, you need to compile this driver as a module. > - Instead of 'ifb', the devices will then be called 'ifb0', > - 'ifb1' etc. > + will be called ifb. > + > Look at the iproute2 documentation directory for usage etc > > +config IFB_COUNT > + int "Number of ifb devices to pre-create at init time" > + depends on IFB > + default 3 > + help > + Static number of ifb devices to be unconditionally pre-created > + at init time. > + > + This default value can be overwritten on the kernel command > + line or with module-parameter ifb.numifbs. > + > + The historic default is 3. If a mid-2007 version of iproute2 > + is used (v2.6.23 or later), it can be set to 0, since needed > + ifb devices can be dynamically allocated via rtnetlink. > + > source "drivers/net/team/Kconfig" > > config MACVLAN > diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c > index cc56fac..911af2c 100644 > --- a/drivers/net/ifb.c > +++ b/drivers/net/ifb.c > @@ -298,7 +298,7 @@ static struct rtnl_link_ops ifb_link_ops __read_mostly = { > * Note that these legacy devices have one queue. > * Prefer something like : ip link add ifb10 numtxqueues 8 type ifb > */ > -static int numifbs = 2; > +static int numifbs = CONFIG_IFB_COUNT; > module_param(numifbs, int, 0); > MODULE_PARM_DESC(numifbs, "Number of ifb devices"); > > -- > 2.5.0 >
On 01/12/2016 12:56 PM, Lubomir Rintel wrote: > The devices can be created at run-time for quite some time already and the > load-time device creation collides with attempts to create the device of > the same name: > > # rmmod ifb > # ip link add ifb0 type ifb > RTNETLINK answers: File exists > > This is pretty much the same situation as was with the block loop devices > which was solved by adding a build-time configuration that the > distributions could use as they deem fit while keeping the default for > compatibility. > > Let's do that here as well. Thanks for the set in general, good to see it fixed! > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > --- [...] > +config IFB_COUNT > + int "Number of ifb devices to pre-create at init time" > + depends on IFB > + default 3 Should that not be 2? > + help > + Static number of ifb devices to be unconditionally pre-created > + at init time. > + > + This default value can be overwritten on the kernel command > + line or with module-parameter ifb.numifbs. > + > + The historic default is 3. If a mid-2007 version of iproute2 > + is used (v2.6.23 or later), it can be set to 0, since needed > + ifb devices can be dynamically allocated via rtnetlink. [...] > -static int numifbs = 2; ^^^^^^^^^^^^ > +static int numifbs = CONFIG_IFB_COUNT; > module_param(numifbs, int, 0); > MODULE_PARM_DESC(numifbs, "Number of ifb devices");
On Tue, 2016-01-12 at 16:19 +0100, Daniel Borkmann wrote: > On 01/12/2016 12:56 PM, Lubomir Rintel wrote: > > The devices can be created at run-time for quite some time already > > and the > > load-time device creation collides with attempts to create the > > device of > > the same name: > > > > # rmmod ifb > > # ip link add ifb0 type ifb > > RTNETLINK answers: File exists > > > > This is pretty much the same situation as was with the block loop > > devices > > which was solved by adding a build-time configuration that the > > distributions could use as they deem fit while keeping the default > > for > > compatibility. > > > > Let's do that here as well. > > Thanks for the set in general, good to see it fixed! > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > --- > [...] > > +config IFB_COUNT > > + int "Number of ifb devices to pre-create at init time" > > + depends on IFB > > + default 3 > > Should that not be 2? Yes, it should be. Good catch, thanks. Will follow-up with a fixed patch. Lubo
On 01/12/2016 04:19 PM, Daniel Borkmann wrote: > On 01/12/2016 12:56 PM, Lubomir Rintel wrote: >> The devices can be created at run-time for quite some time already and the >> load-time device creation collides with attempts to create the device of >> the same name: >> >> # rmmod ifb >> # ip link add ifb0 type ifb >> RTNETLINK answers: File exists >> >> This is pretty much the same situation as was with the block loop devices >> which was solved by adding a build-time configuration that the >> distributions could use as they deem fit while keeping the default for >> compatibility. >> >> Let's do that here as well. > > Thanks for the set in general, good to see it fixed! > >> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> >> --- > [...] >> +config IFB_COUNT >> + int "Number of ifb devices to pre-create at init time" >> + depends on IFB >> + default 3 > > Should that not be 2? > >> + help >> + Static number of ifb devices to be unconditionally pre-created >> + at init time. >> + >> + This default value can be overwritten on the kernel command >> + line or with module-parameter ifb.numifbs. >> + >> + The historic default is 3. If a mid-2007 version of iproute2 >> + is used (v2.6.23 or later), it can be set to 0, since needed >> + ifb devices can be dynamically allocated via rtnetlink. > > [...] >> -static int numifbs = 2; > ^^^^^^^^^^^^ >> +static int numifbs = CONFIG_IFB_COUNT; >> module_param(numifbs, int, 0); >> MODULE_PARM_DESC(numifbs, "Number of ifb devices"); Btw, perhaps there should also be a pr_info() or the like to warn the user that relying on modprobe creating these devs is deprecated and scheduled for removal in future, so they should switch to ip link instead ...
On Tue, 2016-01-12 at 16:35 +0100, Daniel Borkmann wrote: > On 01/12/2016 04:19 PM, Daniel Borkmann wrote: > > On 01/12/2016 12:56 PM, Lubomir Rintel wrote: ... > > > -static int numifbs = 2; > > ^^^^^^^^^^^^ > > > +static int numifbs = CONFIG_IFB_COUNT; > > > module_param(numifbs, int, 0); > > > MODULE_PARM_DESC(numifbs, "Number of ifb devices"); > > Btw, perhaps there should also be a pr_info() or the like to warn the user > that relying on modprobe creating these devs is deprecated and scheduled > for removal in future, so they should switch to ip link instead ... I don't know, is it actually is scheduled for removal? I was under impression that changes in default behavior users can rely on are generally near impossible to do since the potential problems largely outweigh the benefits. The distributions will do the changes in their kernels anyway and changing behavior on kernel upgrades in random old systems might be risky. That said, it's just an impression. I don't follow the development closely enough to be sure about anything. Lubo
On 01/12/2016 04:43 PM, Lubomir Rintel wrote: > On Tue, 2016-01-12 at 16:35 +0100, Daniel Borkmann wrote: >> On 01/12/2016 04:19 PM, Daniel Borkmann wrote: >>> On 01/12/2016 12:56 PM, Lubomir Rintel wrote: > ... >>>> -static int numifbs = 2; >>> ^^^^^^^^^^^^ >>>> +static int numifbs = CONFIG_IFB_COUNT; >>>> module_param(numifbs, int, 0); >>>> MODULE_PARM_DESC(numifbs, "Number of ifb devices"); >> >> Btw, perhaps there should also be a pr_info() or the like to warn the user >> that relying on modprobe creating these devs is deprecated and scheduled >> for removal in future, so they should switch to ip link instead ... > > I don't know, is it actually is scheduled for removal? > > I was under impression that changes in default behavior users can rely > on are generally near impossible to do since the potential problems > largely outweigh the benefits. The distributions will do the changes in > their kernels anyway and changing behavior on kernel upgrades in random > old systems might be risky. BLK_DEV_LOOP_MIN_COUNT is 8 by default on upstream today, 0 in Fedora kernels since Fedora 19 (2013) onwards [1] (F18 to F19 made the switch apparently). If the same happens one day to these options here and all major distros play along, then perhaps after still long enough time it might be an option (and if not, people at least switched to use ip link ;)). > That said, it's just an impression. I don't follow the development > closely enough to be sure about anything. [1] http://pkgs.fedoraproject.org/cgit/rpms/kernel.git/tree/config-generic?h=f19
On Tue, 12 Jan 2016 07:55:22 -0500 Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 16-01-12 06:56 AM, Lubomir Rintel wrote: > > The devices can be created at run-time for quite some time already and the > > load-time device creation collides with attempts to create the device of > > the same name: > > > > # rmmod ifb > > # ip link add ifb0 type ifb > > RTNETLINK answers: File exists > > > > This is pretty much the same situation as was with the block loop devices > > which was solved by adding a build-time configuration that the > > distributions could use as they deem fit while keeping the default for > > compatibility. > > > > Let's do that here as well. > > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > I guess module options are frowned upon. so: I would prefer that this were done with a module parameter, the same as dummy. Only developers build their own configured kernels. Having the value set later at module load time is preferable.
From: Stephen Hemminger <stephen@networkplumber.org> Date: Tue, 12 Jan 2016 10:44:37 -0800 > On Tue, 12 Jan 2016 07:55:22 -0500 > Jamal Hadi Salim <jhs@mojatatu.com> wrote: > >> On 16-01-12 06:56 AM, Lubomir Rintel wrote: >> > The devices can be created at run-time for quite some time already and the >> > load-time device creation collides with attempts to create the device of >> > the same name: >> > >> > # rmmod ifb >> > # ip link add ifb0 type ifb >> > RTNETLINK answers: File exists >> > >> > This is pretty much the same situation as was with the block loop devices >> > which was solved by adding a build-time configuration that the >> > distributions could use as they deem fit while keeping the default for >> > compatibility. >> > >> > Let's do that here as well. >> > >> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> >> >> I guess module options are frowned upon. so: > > I would prefer that this were done with a module parameter, the same as dummy. > Only developers build their own configured kernels. Having the value set later > at module load time is preferable. I like this even less, it means tools behave significantly differently based upon what module options were passed to the kernel. Module options really should not change kernel behavior like this..
On Tue, 2016-01-12 at 15:54 -0500, David Miller wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Tue, 12 Jan 2016 10:44:37 -0800 > > > On Tue, 12 Jan 2016 07:55:22 -0500 > > Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > >> On 16-01-12 06:56 AM, Lubomir Rintel wrote: > >> > The devices can be created at run-time for quite some time > already and the > >> > load-time device creation collides with attempts to create the > device of > >> > the same name: > >> > > >> > # rmmod ifb > >> > # ip link add ifb0 type ifb > >> > RTNETLINK answers: File exists > >> > > >> > This is pretty much the same situation as was with the block > loop devices > >> > which was solved by adding a build-time configuration that the > >> > distributions could use as they deem fit while keeping the > default for > >> > compatibility. > >> > > >> > Let's do that here as well. > >> > > >> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > >> > >> I guess module options are frowned upon. so: > > > > I would prefer that this were done with a module parameter, the > same as dummy. > > Only developers build their own configured kernels. Having the > value set later > > at module load time is preferable. > > I like this even less, it means tools behave significantly > differently > based upon what module options were passed to the kernel. > > Module options really should not change kernel behavior like this.. The module option is already there. It's defaults (creating the devices noone asked for and that potentially collide with what the user tried to create) are what we find bothersome. Lubo
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index f184fb5..63535b4 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -110,12 +110,25 @@ config IFB This is an intermediate driver that allows sharing of resources. To compile this driver as a module, choose M here: the module - will be called ifb. If you want to use more than one ifb - device at a time, you need to compile this driver as a module. - Instead of 'ifb', the devices will then be called 'ifb0', - 'ifb1' etc. + will be called ifb. + Look at the iproute2 documentation directory for usage etc +config IFB_COUNT + int "Number of ifb devices to pre-create at init time" + depends on IFB + default 3 + help + Static number of ifb devices to be unconditionally pre-created + at init time. + + This default value can be overwritten on the kernel command + line or with module-parameter ifb.numifbs. + + The historic default is 3. If a mid-2007 version of iproute2 + is used (v2.6.23 or later), it can be set to 0, since needed + ifb devices can be dynamically allocated via rtnetlink. + source "drivers/net/team/Kconfig" config MACVLAN diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index cc56fac..911af2c 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -298,7 +298,7 @@ static struct rtnl_link_ops ifb_link_ops __read_mostly = { * Note that these legacy devices have one queue. * Prefer something like : ip link add ifb10 numtxqueues 8 type ifb */ -static int numifbs = 2; +static int numifbs = CONFIG_IFB_COUNT; module_param(numifbs, int, 0); MODULE_PARM_DESC(numifbs, "Number of ifb devices");
The devices can be created at run-time for quite some time already and the load-time device creation collides with attempts to create the device of the same name: # rmmod ifb # ip link add ifb0 type ifb RTNETLINK answers: File exists This is pretty much the same situation as was with the block loop devices which was solved by adding a build-time configuration that the distributions could use as they deem fit while keeping the default for compatibility. Let's do that here as well. Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> --- drivers/net/Kconfig | 21 +++++++++++++++++---- drivers/net/ifb.c | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-)