[PATCH 1/3] ifb: make device count build-time configurable

[PATCH 1/3] ifb: make device count build-time configurable

[Lubomir Rintel]

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

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Jamal Hadi Salim]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Stephen Hemminger]

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.

Re: [PATCH 1/3] ifb: make device count build-time configurable

[David Miller]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Lubomir Rintel]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Tom Gundersen]

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
>

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Daniel Borkmann]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Lubomir Rintel]

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

[PATCH v2] ifb: make device count build-time configurable

[Lubomir Rintel]

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>
---
Changes since v1:
    - The default number of devices is 2 not 3.

 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..bb40d30 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 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.
+
 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

Re: [PATCH v2] ifb: make device count build-time configurable

[David Miller]

Please, when you are asked to make changes to a patch within
a series, always resubmit the entire series freshly, including
the cover letter.

Also, please be mindful of:

	http://marc.info/?l=linux-netdev&m=145248145925834&w=2

Thanks.

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Daniel Borkmann]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Lubomir Rintel]

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

Re: [PATCH 1/3] ifb: make device count build-time configurable

[Daniel Borkmann]

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