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

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

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 [off-list ref] 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 <redacted>

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 01/12/2016 04:19 PM, Daniel Borkmann wrote:

On 01/12/2016 12:56 PM, Lubomir Rintel wrote:

quoted

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!

quoted

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---

[...]

quoted

+config IFB_COUNT
+        int "Number of ifb devices to pre-create at init time"
+        depends on IFB
+        default 3

Should that not be 2?

quoted

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

[...]

quoted

-static int numifbs = 2;

               ^^^^^^^^^^^^

quoted

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

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

On Tue, 2016-01-12 at 16:19 +0100, Daniel Borkmann wrote:

On 01/12/2016 12:56 PM, Lubomir Rintel wrote:

quoted

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!

quoted

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---

[...]

quoted

+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 Tue, 2016-01-12 at 16:35 +0100, Daniel Borkmann wrote:

On 01/12/2016 04:19 PM, Daniel Borkmann wrote:

quoted

On 01/12/2016 12:56 PM, Lubomir Rintel wrote:

...

quoted

quoted

-static int numifbs = 2;

               ^^^^^^^^^^^^

quoted

+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:

quoted

On 01/12/2016 04:19 PM, Daniel Borkmann wrote:

quoted

On 01/12/2016 12:56 PM, Lubomir Rintel wrote:

...

quoted

quoted

quoted

-static int numifbs = 2;

                ^^^^^^^^^^^^

quoted

+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 [off-list ref] wrote:

On 16-01-12 06:56 AM, Lubomir Rintel wrote:

quoted

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.

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.

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 [off-list ref] wrote:

quoted

On 16-01-12 06:56 AM, Lubomir Rintel wrote:

quoted

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

quoted

On Tue, 12 Jan 2016 07:55:22 -0500
Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-01-12 06:56 AM, Lubomir Rintel wrote:

quoted

The devices can be created at run-time for quite some time

already and the

quoted

quoted

quoted

load-time device creation collides with attempts to create the

device of

quoted

quoted

quoted

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

quoted

quoted

quoted

which was solved by adding a build-time configuration that the
distributions could use as they deem fit while keeping the

default for

quoted

quoted

quoted

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.

quoted

Only developers build their own configured kernels. Having the

value set later

quoted

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