[PATCH] e100: expose broadcast_disabled as a module option

[PATCH] e100: expose broadcast_disabled as a module option

[Erwan Velu]

Hi folks,

I've been facing a very noisy network where hundreds broadcast packets
were generated every second.
When this traffic can't be controlled at the source, there is a side
effect on some systems.
I was having some idle systems that will never be targeted by this
broadcast traffic that got loaded just by receiving that "flood".
I mean by loaded that this light hardware was generating 300
context/switches per second.

I was looking for many options to avoid this traffic to disturb this
hosts and I discovered that the e100 driver was featuring a
"broadcast_disabled" configure option.
I realize that this option is not controllable, so I wrote this simple
patch that expose this option as a module option.
This allow me to tell this hosts not to listen anymore this traffic.

The result is clearly good as my systems are now running at 21
context/switches while being idle.
Hope this patch isn't too bad and could help others that faces the same problem.

Patch can be downloaded here :
http://konilope.linuxeries.org/e100_broadcast_disabled.patch

Even if gmail is eating the inlined, patch, at least that make it
easier to read it for humans.
If the patch is acked, the downloaded one will be more clean ;)

This patch was generated on top of the latest 2.6 torvald's git.
Cheers,
Erwan

Signed-off-by: Erwan Velu <erwanaliasr1@gmail.com>

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index b997e57..2ba582f 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -194,12 +194,15 @@ MODULE_FIRMWARE(FIRMWARE_D102E);
 static int debug = 3;
 static int eeprom_bad_csum_allow = 0;
 static int use_io = 0;
+static int broadcast_disabled = 0;
 module_param(debug, int, 0);
 module_param(eeprom_bad_csum_allow, int, 0);
 module_param(use_io, int, 0);
+module_param(broadcast_disabled, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 MODULE_PARM_DESC(eeprom_bad_csum_allow, "Allow bad eeprom checksums");
 MODULE_PARM_DESC(use_io, "Force use of i/o access mode");
+MODULE_PARM_DESC(broadcast_disabled, "Filter broadcast packets
(0=disabled (default), 1=enabled)");
 #define DPRINTK(nlevel, klevel, fmt, args...) \
 	(void)((NETIF_MSG_##nlevel & nic->msg_enable) && \
 	printk(KERN_##klevel PFX "%s: %s: " fmt, nic->netdev->name, \
@@ -1131,6 +1134,8 @@ static void e100_configure(struct nic *nic,
struct cb *cb, struct sk_buff *skb)
 		config->promiscuous_mode = 0x1;		/* 1=on, 0=off */
 	}

+	config->broadcast_disabled = broadcast_disabled; /* Broadcast filtering */
+
 	if (nic->flags & multicast_all)
 		config->multicast_all = 0x1;		/* 1=accept, 0=no */

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Jeff Kirsher]

On Fri, Apr 23, 2010 at 13:14, Erwan Velu <erwanaliasr1@gmail.com> wrote:
> Hi folks,
>
> I've been facing a very noisy network where hundreds broadcast packets
> were generated every second.
> When this traffic can't be controlled at the source, there is a side
> effect on some systems.
> I was having some idle systems that will never be targeted by this
> broadcast traffic that got loaded just by receiving that "flood".
> I mean by loaded that this light hardware was generating 300
> context/switches per second.
>
> I was looking for many options to avoid this traffic to disturb this
> hosts and I discovered that the e100 driver was featuring a
> "broadcast_disabled" configure option.
> I realize that this option is not controllable, so I wrote this simple
> patch that expose this option as a module option.
> This allow me to tell this hosts not to listen anymore this traffic.
>
> The result is clearly good as my systems are now running at 21
> context/switches while being idle.
> Hope this patch isn't too bad and could help others that faces the same problem.
>
> Patch can be downloaded here :
> http://konilope.linuxeries.org/e100_broadcast_disabled.patch
>
> Even if gmail is eating the inlined, patch, at least that make it
> easier to read it for humans.
> If the patch is acked, the downloaded one will be more clean ;)
>
> This patch was generated on top of the latest 2.6 torvald's git.
> Cheers,
> Erwan
>
> Signed-off-by: Erwan Velu <erwanaliasr1@gmail.com>
>
> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> index b997e57..2ba582f 100644
> --- a/drivers/net/e100.c
> +++ b/drivers/net/e100.c
> @@ -194,12 +194,15 @@ MODULE_FIRMWARE(FIRMWARE_D102E);
>  static int debug = 3;
>  static int eeprom_bad_csum_allow = 0;
>  static int use_io = 0;
> +static int broadcast_disabled = 0;
>  module_param(debug, int, 0);
>  module_param(eeprom_bad_csum_allow, int, 0);
>  module_param(use_io, int, 0);
> +module_param(broadcast_disabled, int, 0);
>  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>  MODULE_PARM_DESC(eeprom_bad_csum_allow, "Allow bad eeprom checksums");
>  MODULE_PARM_DESC(use_io, "Force use of i/o access mode");
> +MODULE_PARM_DESC(broadcast_disabled, "Filter broadcast packets
> (0=disabled (default), 1=enabled)");
>  #define DPRINTK(nlevel, klevel, fmt, args...) \
>        (void)((NETIF_MSG_##nlevel & nic->msg_enable) && \
>        printk(KERN_##klevel PFX "%s: %s: " fmt, nic->netdev->name, \
> @@ -1131,6 +1134,8 @@ static void e100_configure(struct nic *nic,
> struct cb *cb, struct sk_buff *skb)
>                config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>        }
>
> +       config->broadcast_disabled = broadcast_disabled; /* Broadcast filtering */
> +
>        if (nic->flags & multicast_all)
>                config->multicast_all = 0x1;            /* 1=accept, 0=no */
> --

Adding Netdev...

-- 
Cheers,
Jeff

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Stephen Hemminger]

On Fri, 23 Apr 2010 13:22:22 -0700
Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:

> On Fri, Apr 23, 2010 at 13:14, Erwan Velu <erwanaliasr1@gmail.com> wrote:
> > Hi folks,
> >
> > I've been facing a very noisy network where hundreds broadcast packets
> > were generated every second.
> > When this traffic can't be controlled at the source, there is a side
> > effect on some systems.
> > I was having some idle systems that will never be targeted by this
> > broadcast traffic that got loaded just by receiving that "flood".
> > I mean by loaded that this light hardware was generating 300
> > context/switches per second.
> >
> > I was looking for many options to avoid this traffic to disturb this
> > hosts and I discovered that the e100 driver was featuring a
> > "broadcast_disabled" configure option.
> > I realize that this option is not controllable, so I wrote this simple
> > patch that expose this option as a module option.
> > This allow me to tell this hosts not to listen anymore this traffic.
> >
> > The result is clearly good as my systems are now running at 21
> > context/switches while being idle.
> > Hope this patch isn't too bad and could help others that faces the same problem.
> >
> > Patch can be downloaded here :
> > http://konilope.linuxeries.org/e100_broadcast_disabled.patch
> >
> > Even if gmail is eating the inlined, patch, at least that make it
> > easier to read it for humans.
> > If the patch is acked, the downloaded one will be more clean ;)
> >
> > This patch was generated on top of the latest 2.6 torvald's git.
> > Cheers,
> > Erwan
> >
> > Signed-off-by: Erwan Velu <erwanaliasr1@gmail.com>
> >
> > diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> > index b997e57..2ba582f 100644
> > --- a/drivers/net/e100.c
> > +++ b/drivers/net/e100.c
> > @@ -194,12 +194,15 @@ MODULE_FIRMWARE(FIRMWARE_D102E);
> >  static int debug = 3;
> >  static int eeprom_bad_csum_allow = 0;
> >  static int use_io = 0;
> > +static int broadcast_disabled = 0;
> >  module_param(debug, int, 0);
> >  module_param(eeprom_bad_csum_allow, int, 0);
> >  module_param(use_io, int, 0);
> > +module_param(broadcast_disabled, int, 0);
> >  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> >  MODULE_PARM_DESC(eeprom_bad_csum_allow, "Allow bad eeprom checksums");
> >  MODULE_PARM_DESC(use_io, "Force use of i/o access mode");
> > +MODULE_PARM_DESC(broadcast_disabled, "Filter broadcast packets
> > (0=disabled (default), 1=enabled)");
> >  #define DPRINTK(nlevel, klevel, fmt, args...) \
> >        (void)((NETIF_MSG_##nlevel & nic->msg_enable) && \
> >        printk(KERN_##klevel PFX "%s: %s: " fmt, nic->netdev->name, \
> > @@ -1131,6 +1134,8 @@ static void e100_configure(struct nic *nic,
> > struct cb *cb, struct sk_buff *skb)
> >                config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
> >        }
> >
> > +       config->broadcast_disabled = broadcast_disabled; /* Broadcast filtering */
> > +
> >        if (nic->flags & multicast_all)
> >                config->multicast_all = 0x1;            /* 1=accept, 0=no */
> > --
> 
> Adding Netdev...
> 

What is wrong with using existing IFF_BROADCAST flag?


-- 

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Erwan Velu]

I first tried "ifconfig -broadcast" without any success, so I forced
the driver to unset IFF_BROADCAST, the interface didn't showed anymore
the BROADCAST option with ifconfig. But I didn't noticed any reduction
in the amount of context/switches on my host.

I found the broadcast_disabled far more efficient when considering the
cpu impact.


2010/4/23 Stephen Hemminger <shemminger@vyatta.com>:
> On Fri, 23 Apr 2010 13:22:22 -0700
> Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>
>> On Fri, Apr 23, 2010 at 13:14, Erwan Velu <erwanaliasr1@gmail.com> wrote:
>> > Hi folks,
>> >
>> > I've been facing a very noisy network where hundreds broadcast packets
>> > were generated every second.
>> > When this traffic can't be controlled at the source, there is a side
>> > effect on some systems.
>> > I was having some idle systems that will never be targeted by this
>> > broadcast traffic that got loaded just by receiving that "flood".
>> > I mean by loaded that this light hardware was generating 300
>> > context/switches per second.
>> >
>> > I was looking for many options to avoid this traffic to disturb this
>> > hosts and I discovered that the e100 driver was featuring a
>> > "broadcast_disabled" configure option.
>> > I realize that this option is not controllable, so I wrote this simple
>> > patch that expose this option as a module option.
>> > This allow me to tell this hosts not to listen anymore this traffic.
>> >
>> > The result is clearly good as my systems are now running at 21
>> > context/switches while being idle.
>> > Hope this patch isn't too bad and could help others that faces the same problem.
>> >
>> > Patch can be downloaded here :
>> > http://konilope.linuxeries.org/e100_broadcast_disabled.patch
>> >
>> > Even if gmail is eating the inlined, patch, at least that make it
>> > easier to read it for humans.
>> > If the patch is acked, the downloaded one will be more clean ;)
>> >
>> > This patch was generated on top of the latest 2.6 torvald's git.
>> > Cheers,
>> > Erwan
>> >
>> > Signed-off-by: Erwan Velu <erwanaliasr1@gmail.com>
>> >
>> > diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>> > index b997e57..2ba582f 100644
>> > --- a/drivers/net/e100.c
>> > +++ b/drivers/net/e100.c
>> > @@ -194,12 +194,15 @@ MODULE_FIRMWARE(FIRMWARE_D102E);
>> >  static int debug = 3;
>> >  static int eeprom_bad_csum_allow = 0;
>> >  static int use_io = 0;
>> > +static int broadcast_disabled = 0;
>> >  module_param(debug, int, 0);
>> >  module_param(eeprom_bad_csum_allow, int, 0);
>> >  module_param(use_io, int, 0);
>> > +module_param(broadcast_disabled, int, 0);
>> >  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>> >  MODULE_PARM_DESC(eeprom_bad_csum_allow, "Allow bad eeprom checksums");
>> >  MODULE_PARM_DESC(use_io, "Force use of i/o access mode");
>> > +MODULE_PARM_DESC(broadcast_disabled, "Filter broadcast packets
>> > (0=disabled (default), 1=enabled)");
>> >  #define DPRINTK(nlevel, klevel, fmt, args...) \
>> >        (void)((NETIF_MSG_##nlevel & nic->msg_enable) && \
>> >        printk(KERN_##klevel PFX "%s: %s: " fmt, nic->netdev->name, \
>> > @@ -1131,6 +1134,8 @@ static void e100_configure(struct nic *nic,
>> > struct cb *cb, struct sk_buff *skb)
>> >                config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>> >        }
>> >
>> > +       config->broadcast_disabled = broadcast_disabled; /* Broadcast filtering */
>> > +
>> >        if (nic->flags & multicast_all)
>> >                config->multicast_all = 0x1;            /* 1=accept, 0=no */
>> > --
>>
>> Adding Netdev...
>>
>
> What is wrong with using existing IFF_BROADCAST flag?
>
>
> --
>

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Stephen Hemminger]

On Fri, 23 Apr 2010 23:03:59 +0200
Erwan Velu <erwanaliasr1@gmail.com> wrote:

> I first tried "ifconfig -broadcast" without any success, so I forced
> the driver to unset IFF_BROADCAST, the interface didn't showed anymore
> the BROADCAST option with ifconfig. But I didn't noticed any reduction
> in the amount of context/switches on my host.
> 
> I found the broadcast_disabled far more efficient when considering the
> cpu impact.

The point is that the driver can look at IFF_BROADCAST rather than having
module parameter. Module parameters are device driver specific and should
be avoid as much as possible in favor of general mechanism. This is a repeated
problem where users and vendors make special hooks that only work with their
driver, which makes life hard for other users and distribution providers.

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Erwan Velu]

Agreed that will be a better implementation. Changes of IFF_BROADCAST
could play with this "broadcast_disabled" configuration switch to
increase the cpu efficiency.

 I'm not a kernel expert and I don't really figure how the changes of
the IFF_BROADCAST should be forwarded to the interfaces and how it can
be manipulated. I saw that ifconfig have a '-broadcast' option but
looks like none of my drivers are compatible with that.

Does any one you could help me understanding what should be the good way to
1- enabled/disabled IFF_BROADCAST for a given interface
2- populate this changes to the driver

Once I'll be able to have a proper patch using that, I'll post it
again to the list.

Cheers,
Erwan

2010/4/24 Stephen Hemminger <shemminger@vyatta.com>:
> On Fri, 23 Apr 2010 23:03:59 +0200
[...]
> The point is that the driver can look at IFF_BROADCAST rather than having
> module parameter. Module parameters are device driver specific and should
> be avoid as much as possible in favor of general mechanism. This is a repeated
> problem where users and vendors make special hooks that only work with their
> driver, which makes life hard for other users and distribution providers.
>

Re: [PATCH] e100: expose broadcast_disabled as a module option

[Erwan Velu]

2010/4/26 Erwan Velu <erwanaliasr1@gmail.com>:

After some research, here come my status :

> Does any one you could help me understanding what should be the good way to
> 1- enabled/disabled IFF_BROADCAST for a given interface

Looks like the current code isn't taking care of a possible changing
of IFF_BROADCAST. I can find some code in net/core/dev.c to handle
MULTICAST or PROMISC but nothing for BROADCAST.
do I have to implement one ?


> 2- populate this changes to the driver

Looks like I need then to add a .ndo_change_rx_flags function to
manage this changes.

Does someone confirm this is the way to go ?