Re: [PATCH v4] net/bonding: send arp in interval if no active slave

[Jarod Wilson]

Nikolay Aleksandrov wrote:
> On 10/06/2015 09:53 PM, Jarod Wilson wrote:
> > From: Uwe Koziolek<uwe.koziolek@xxxxxxxxxxx>
> >
> > With some very finicky switch hardware, active backup bonding can get into
> > a situation where we play ping-pong between interfaces, trying to get one
> > to come up as the active slave. There seems to be an issue with the
> > switch's arp replies either taking too long, or simply getting lost, so we
> > wind up unable to get any interface up and active. Sometimes, the issue
> > sorts itself out after a while, sometimes it doesn't.
> >
> > Testing with num_grat_arp has proven fruitless, but sending an additional
> > arp on curr_arp_slave if we're still in the arp_interval timeslice in
> > bond_ab_arp_probe(), has shown to produce 100% reliability in testing with
> > this hardware combination.
> >
> > [jarod: manufacturing of changelog, addition of modparam gating]
> > CC: Jay Vosburgh<jay.vosburgh@xxxxxxxxxxxxx>
> > CC: Andy Gospodarek<gospo@xxxxxxxxxxxxxxxxxxx>
> > CC: Veaceslav Falico<vfalico@xxxxxxxxx>
> > CC: netdev@xxxxxxxxxxxxxxx
> > Signed-off-by: Uwe Koziolek<uwe.koziolek@xxxxxxxxxxx>
> > Signed-off-by: Jarod Wilson<jarod@xxxxxxxxxx>
> > ---
> > v2: add code comment as to why change is needed
> > v3: fix wrapping of comments
> > v4: [jarod] add module parameter gating of code addition
> Hi all,
> As Andy already stated I'm not a fan of such workarounds either but it's
> necessary sometimes so if this is going to be actually considered then a
> few things need to be fixed. Please make this a proper bonding option
> which can be changed at runtime and not only via a module parameter.

Okay, I can give that a shot, however...

> Now, I saw that you've only tested with 500 ms, can't this be fixed by using
> a different interval ? This seems like a very specific problem to have a
> whole new option for.

...I'll wait until we've heard confirmation from Uwe that intervals 
other than 500ms don't fix things.

> I really want to say fix the switch but I know that's not an option. :-)

Yeah, unfortunately not!

> A few minor nits below,
>
> >   drivers/net/bonding/bond_main.c | 24 ++++++++++++++++++++++++
> >   include/net/bonding.h           |  1 +
> >   2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 90f2615..72ab512 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -95,6 +95,7 @@ static int miimon;
> >   static int updelay;
> >   static int downdelay;
> >   static int use_carrier	= 1;
> > +static int arp_slow_switch;
> >   static char *mode;
> >   static char *primary;
> >   static char *primary_reselect;
> > @@ -133,6 +134,10 @@ MODULE_PARM_DESC(downdelay, "Delay before considering link down, "
> >   module_param(use_carrier, int, 0);
> >   MODULE_PARM_DESC(use_carrier, "Use netif_carrier_ok (vs MII ioctls) in miimon; "
> >   			      "0 for off, 1 for on (default)");
> > +module_param(arp_slow_switch, int, 0);
> > +MODULE_PARM_DESC(arp_slow_switch, "Do extra arp checks for switches with arp "
> > +				  "caches that are slow to update; "
> > +				  "0 for off (default), 1 for on");
> >   module_param(mode, charp, 0);
> >   MODULE_PARM_DESC(mode, "Mode of operation; 0 for balance-rr, "
> >   		       "1 for active-backup, 2 for balance-xor, "
> > @@ -2793,6 +2798,18 @@ static bool bond_ab_arp_probe(struct bonding *bond)
> >   			return should_notify_rtnl;
> >   	}
> >
> > +	/* Sometimes the forwarding tables of the switches are not update
> ^ s/update/updated/

D'oh. Fixed locally.

> > @@ -4280,6 +4297,12 @@ static int bond_check_params(struct bond_params *params)
> >   		use_carrier = 1;
> >   	}
> >
> > +	if ((arp_slow_switch != 0) &&  (arp_slow_switch != 1)) {
> ^^ no need for the extra ()

Copy-pasta from use_carrier checks right above it. Never quite sure if I 
should stick with the same possibly sub-optimal formatting conventions 
already in the file, try to fix them while also fixing bugs, or just mix 
styles...


> > +		pr_warn("Warning: arp_slow_switch module parameter (%d), not of valid value (0/1), so it was set to 1\n",
> > +			arp_slow_switch);
> > +		arp_slow_switch = 1;
> ^^ please default to old behaviour in this case (0)

Will do.



--
Jarod Wilson
jarod@xxxxxxxxxx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/