No. There were two comments but neither one received a response: https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348943.html https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348944.html
On Mon, Aug 13, 2018 at 11:22:54AM +0000, Nitin Katiyar wrote: > Ben, > I am following up on the patch which Manohar had sent earlier. Can it be > merged? > > Regards, > Nitin > > -----Original Message----- > From: Manohar Krishnappa Chidambaraswamy > [mailto:manohar.krishnappa.chidambarasw...@ericsson.com] > Sent: Monday, June 25, 2018 2:49 PM > To: Ben Pfaff <b...@ovn.org> > Cc: d...@openvswitch.org; Nitin Katiyar <nitin.kati...@ericsson.com> > Subject: Re: [ovs-dev] [PATCH v2 1/2] Fix packet drops on LACP bond after > link up > > Hi Ben, > > Does this patch apply without issues? > > Would you be able to look at 2/2 of this series as well? > > Thanx > Manu > > On 18/06/18, 2:05 PM, "ovs-dev-boun...@openvswitch.org on behalf of Manohar > Krishnappa Chidambaraswamy" <ovs-dev-boun...@openvswitch.org on behalf of > manohar.krishnappa.chidambarasw...@ericsson.com> wrote: > > Ben, > > Here are the v2 diffs. Hope this applies without any issue. > > Thanx > Manu > > Signed-off-by: Manohar K C > <manohar.krishnappa.chidambarasw...@ericsson.com> > CC: Jan Scheurich <jan.scheur...@ericsson.com> > CC: Nitin Katiyar <nitin.kati...@ericsson.com> > --- > v1 1/2: https://patchwork.ozlabs.org/patch/915285/ > v2 1/2: Rebased to master > > lib/lacp.c | 14 ++++++++++++-- > lib/lacp.h | 3 ++- > ofproto/bond.c | 18 +++++++++++++++--- > ofproto/ofproto-dpif-xlate.c | 13 ++++++++++++- > 4 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/lib/lacp.c b/lib/lacp.c > index d6b36aa..9e43e06 100644 > --- a/lib/lacp.c > +++ b/lib/lacp.c > @@ -154,6 +154,7 @@ static struct slave *slave_lookup(const struct lacp > *, const void *slave) > OVS_REQUIRES(mutex); > static bool info_tx_equal(struct lacp_info *, struct lacp_info *) > OVS_REQUIRES(mutex); > +static bool slave_may_enable__(struct slave *slave) OVS_REQUIRES(mutex); > > static unixctl_cb_func lacp_unixctl_show; > static unixctl_cb_func lacp_unixctl_show_stats; > @@ -324,8 +325,8 @@ lacp_is_active(const struct lacp *lacp) > OVS_EXCLUDED(mutex) > /* Processes 'packet' which was received on 'slave_'. This function > should be > * called on all packets received on 'slave_' with Ethernet Type > ETH_TYPE_LACP. > */ > -void > -lacp_process_packet(struct lacp *lacp, const void *slave_, > +bool > +lacp_process_packet(struct lacp *lacp, const void *bond, const void > *slave_, > const struct dp_packet *packet) > OVS_EXCLUDED(mutex) > { > @@ -333,6 +334,7 @@ lacp_process_packet(struct lacp *lacp, const void > *slave_, > const struct lacp_pdu *pdu; > long long int tx_rate; > struct slave *slave; > + bool lacp_may_enable = false; > > lacp_lock(); > slave = slave_lookup(lacp, slave_); > @@ -362,8 +364,16 @@ lacp_process_packet(struct lacp *lacp, const void > *slave_, > slave->partner = pdu->actor; > } > > + /* > + * Evaluate may_enable here to avoid dropping of packets till main > thread > + * sets may_enable to true. > + */ > + lacp_may_enable = slave_may_enable__(slave); > + > out: > lacp_unlock(); > + > + return lacp_may_enable; > } > > /* Returns the lacp_status of the given 'lacp' object (which may be > NULL). */ > diff --git a/lib/lacp.h b/lib/lacp.h > index f35cff5..1505c2c 100644 > --- a/lib/lacp.h > +++ b/lib/lacp.h > @@ -46,7 +46,8 @@ struct lacp *lacp_ref(const struct lacp *); > void lacp_configure(struct lacp *, const struct lacp_settings *); > bool lacp_is_active(const struct lacp *); > > -void lacp_process_packet(struct lacp *, const void *slave, > +bool lacp_process_packet(struct lacp *, const void *bond, > + const void *slave, > const struct dp_packet *packet); > enum lacp_status lacp_status(const struct lacp *); > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index f87cdba..5fc1e0e 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -777,6 +777,7 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > { > enum bond_verdict verdict = BV_DROP; > struct bond_slave *slave; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > ovs_rwlock_rdlock(&rwlock); > slave = bond_slave_lookup(bond, slave_); > @@ -794,7 +795,13 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > * drop all incoming traffic except if lacp_fallback_ab is enabled. > */ > switch (bond->lacp_status) { > case LACP_NEGOTIATED: > - verdict = slave->enabled ? BV_ACCEPT : BV_DROP; > + /* > + * To reduce packet-drops due to delay in enabling of slave (post > + * LACP-SYNC), from main thread, check for may_enable as well. > + * When may_enable is TRUE, it means LACP is UP and waiting for > + * the main thread to run LACP state machine and enable the > slave. > + */ > + verdict = (slave->enabled || slave->may_enable) ? BV_ACCEPT : > BV_DROP; > goto out; > case LACP_CONFIGURED: > if (!bond->lacp_fallback_ab) { > @@ -830,8 +837,6 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > /* Drop all packets which arrive on backup slaves. This is > similar to > * how Linux bonding handles active-backup bonds. */ > if (bond->active_slave != slave) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > - > VLOG_DBG_RL(&rl, "active-backup bond received packet on > backup" > " slave (%s) destined for " ETH_ADDR_FMT, > slave->name, ETH_ADDR_ARGS(eth_dst)); > @@ -853,6 +858,13 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > > OVS_NOT_REACHED(); > out: > + if (slave && (verdict != BV_ACCEPT)) { > + VLOG_DBG_RL(&rl, "slave= %s actv-slave= %d may_enable %d enable > %d " > + "LACP %d verdict(A/D/M=0/1/2) %d\n", slave->name, > + (bond->active_slave == slave), slave->may_enable, > + slave->enabled, bond->lacp_status, verdict); > + } > + > ovs_rwlock_unlock(&rwlock); > return verdict; > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index c02a032..b49a223 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3189,6 +3189,7 @@ process_special(struct xlate_ctx *ctx, const struct > xport *xport) > const struct xbridge *xbridge = ctx->xbridge; > const struct dp_packet *packet = ctx->xin->packet; > enum slow_path_reason slow; > + bool lacp_may_enable; > > if (!xport) { > slow = 0; > @@ -3209,7 +3210,17 @@ process_special(struct xlate_ctx *ctx, const > struct xport *xport) > } else if (xport->xbundle && xport->xbundle->lacp > && flow->dl_type == htons(ETH_TYPE_LACP)) { > if (packet) { > - lacp_process_packet(xport->xbundle->lacp, xport->ofport, > packet); > + lacp_may_enable = lacp_process_packet(xport->xbundle->lacp, > + xport->xbundle->bond, > + xport->ofport, packet); > + /* > + * Update LACP status in bond-slave to avoid packet-drops > until > + * LACP state machine is run by the main thread. > + */ > + if (xport->xbundle->bond && lacp_may_enable) { > + bond_slave_set_may_enable(xport->xbundle->bond, > xport->ofport, > + lacp_may_enable); > + } > } > slow = SLOW_LACP; > } else if ((xbridge->stp || xbridge->rstp) && > -- > 1.9.1 > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev