[ovs-dev] [PATCH v4 2/2] Avoid packet drop on LACP bond after link up

[Nitin Katiyar]

Hi,

> -----Original Message-----
> From: Ben Pfaff [mailto:blp at ovn.org]
> Sent: Saturday, March 02, 2019 12:15 AM
> To: Nitin Katiyar <nitin.katiyar at ericsson.com>
> Cc: ovs-dev at openvswitch.org; Manohar Krishnappa Chidambaraswamy
> <manukc at gmail.com>
> Subject: Re: [ovs-dev] [PATCH v4 2/2] Avoid packet drop on LACP bond after
> link up
> 
> I don't entirely understand the problem.  It seems like a driver bug.
> Why isn't the bug being fixed?
I agree it is driver/firmware bug but OVS implementation also doesn't account for link state which causes traffic drop in case of bond.
> 
> Patches 1 and 2 have the same title.  It would be better if they were different.
Both issues had surfaced in same scenario so we decided to have 2 patches. But we can send it as 2 different patches next time.
> 
> [more below]
> 
> On Thu, Feb 28, 2019 at 01:56:20PM +0000, Nitin Katiyar wrote:
> > +    /*
> > +     * On some NICs L1 state reporting is slow. In case LACP packets are
> > +     * received while carrier (L1) state is still down, drop the LACPDU and
> > +     * trigger re-checking of L1 state.
> > +     */
> > +    carrier_up = bond_slave_get_carrier(bond, slave->aux);
> > +    if (!carrier_up) {
> > +        seq_change(connectivity_seq_get());
> > +
> > +        VLOG_INFO_RL(&rl, "carrier still DOWN - conn seq changed for %s, "
> > +                  "dropping packet\n", slave->name);
> 
> It's going to be hard for a user to understand what this means, especially the
> "conn seq changed" part.  The user also doesn't know the context, so it's not
> clear what "carrier still DOWN" means--still down after what?  Can you please
> rephrase the log message so that it's clear what happened and why it's
> unusual?
Sure, I will rephrase it.
> 
> Please put the name of the lacp object at the beginning of the message.
> 
> Please don't add \n at the end of a log message; the logger does that itself.
> 
> In bond_slave_get_carrier(), please use CONST_CAST for casting away const.
I will update all these three in next patch.

Thanks,
Nitin