| Message ID | 20220317155412.32579-1-cfontain@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto/bond: Add knob "all_slaves_active" | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Bleep bloop. Greetings Christophe Fontaine, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#115 FILE: vswitchd/vswitch.xml:2096:
kernel bonds. This is useful when we need to share 2 physical functions
WARNING: Line is 81 characters long (recommended limit is 79)
#117 FILE: vswitchd/vswitch.xml:2098:
be negotiated over the same physical link. LACP session will be managed
WARNING: Line is 82 characters long (recommended limit is 79)
#119 FILE: vswitchd/vswitch.xml:2100:
In that particular configuration, the physical switch will send a unique
WARNING: Line has trailing whitespace
#121 FILE: vswitchd/vswitch.xml:2102:
the secondary link of the bond.
Lines checked: 131, Warnings: 4, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Thu, Mar 17, 2022 at 11:54 AM Christophe Fontaine <cfontain@redhat.com> wrote: > > This config param allows the delivery of broadcast and multicast packets > to the secondary interface of non-lacp bonds, identical to the same option > for kernel bonds. > > Tested with an ovs-dpdk balance-slb bond with 2 virtual functions, > and a kernel bond with LACP on 2 other virtual functions. > > Scapy sending 10 broadcast packets from 10 different mac addresses: > - with ovs-vsctl set port dpdkbond other_config:all_slaves_active=false > (default unmodified behavior) 5 packets are received > - with other_config:all_slaves_active=true, all 10 packets are received. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1720935 > Signed-off-by: Christophe Fontaine <cfontain@redhat.com> > --- Hello Chris, This feature seems like it would be useful, and looks correctly implemented. However, the nomenclature used throughout the code and documentation is member in place of slave. Cheers, M > NEWS | 1 + > ofproto/bond.c | 5 ++++- > ofproto/bond.h | 2 ++ > vswitchd/bridge.c | 3 +++ > vswitchd/vswitch.xml | 20 ++++++++++++++++++++ > 5 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/NEWS b/NEWS > index df633e8e2..07f72fd5a 100644 > --- a/NEWS > +++ b/NEWS > @@ -16,6 +16,7 @@ v2.17.0 - xx xxx xxxx > * Removed experimental tag for PMD Auto Load Balance. > * New configuration knob 'other_config:n-offload-threads' to change the > number of HW offloading threads. > + * New configuration knob 'all_slaves_active' for non lacp bonds > - DPDK: > * EAL argument --socket-mem is no longer configured by default upon > start-up. If dpdk-socket-mem and dpdk-alloc-mem are not specified, > diff --git a/ofproto/bond.c b/ofproto/bond.c > index cdfdf0b9d..8bf166a41 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -145,6 +145,7 @@ struct bond { > struct eth_addr active_member_mac; /* MAC address of the active member. */ > /* Legacy compatibility. */ > bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */ > + bool all_slaves_active; > > struct ovs_refcount ref_cnt; > }; > @@ -448,6 +449,7 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s) > > bond->updelay = s->up_delay; > bond->downdelay = s->down_delay; > + bond->all_slaves_active = s->all_slaves_active; > > if (bond->lacp_fallback_ab != s->lacp_fallback_ab_cfg) { > bond->lacp_fallback_ab = s->lacp_fallback_ab_cfg; > @@ -893,7 +895,8 @@ bond_check_admissibility(struct bond *bond, const void *member_, > > /* Drop all multicast packets on inactive members. */ > if (eth_addr_is_multicast(eth_dst)) { > - if (bond->active_member != member) { > + if (bond->active_member != member && > + bond->all_slaves_active == false) { > goto out; > } > } > diff --git a/ofproto/bond.h b/ofproto/bond.h > index 1683ec878..4e8c1872a 100644 > --- a/ofproto/bond.h > +++ b/ofproto/bond.h > @@ -62,6 +62,8 @@ struct bond_settings { > ovs run. */ > bool use_lb_output_action; /* Use lb_output action. Only applicable for > bond mode BALANCE TCP. */ > + bool all_slaves_active; /* For non LACP bond, also accept multicast > + packets on secondary interface. */ > }; > > /* Program startup. */ > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 5223aa897..53be7ddf6 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -4615,6 +4615,9 @@ port_configure_bond(struct port *port, struct bond_settings *s) > s->use_lb_output_action = (s->balance == BM_TCP) > && smap_get_bool(&port->cfg->other_config, > "lb-output-action", false); > + /* all_slaves_active is disabled by default */ > + s->all_slaves_active = smap_get_bool(&port->cfg->other_config, > + "all_slaves_active", false); > } > > /* Returns true if 'port' is synthetic, that is, if we constructed it locally > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 0c6632617..d420397a3 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2083,6 +2083,26 @@ > <code>true</code>). > </column> > > + <column name="other_config" key="all_slaves_active" > + type='{"type": "boolean"}'> > + <p> > + Enable/Disable delivery of broadcast/multicast packets on secondary > + interface of a bond. Relevant only when <ref column="lacp"/> is > + <code>off</code>. > + </p> > + > + <p> > + This parameter is identical to <code>all_slaves_active</code> for > + kernel bonds. This is useful when we need to share 2 physical functions > + between an ovs bond and a linux bond, as 2 LACP sessions can't > + be negotiated over the same physical link. LACP session will be managed > + by the kernel bond, while an active-active bond is used for ovs. > + In that particular configuration, the physical switch will send a unique > + copy of a broadcast packet over the 2 physical links, eventually to > + the secondary link of the bond. > + </p> > + </column> > + > <group title="Link Failure Detection"> > <p> > An important part of link bonding is detecting that links are down so > -- > 2.35.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Mar 23, 2022 at 10:12 PM Mike Pattrick <mkp@redhat.com> wrote: > > On Thu, Mar 17, 2022 at 11:54 AM Christophe Fontaine > <cfontain@redhat.com> wrote: > > > > This config param allows the delivery of broadcast and multicast packets > > to the secondary interface of non-lacp bonds, identical to the same option > > for kernel bonds. > > > > Tested with an ovs-dpdk balance-slb bond with 2 virtual functions, > > and a kernel bond with LACP on 2 other virtual functions. > > > > Scapy sending 10 broadcast packets from 10 different mac addresses: > > - with ovs-vsctl set port dpdkbond other_config:all_slaves_active=false > > (default unmodified behavior) 5 packets are received > > - with other_config:all_slaves_active=true, all 10 packets are received. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1720935 > > Signed-off-by: Christophe Fontaine <cfontain@redhat.com> > > --- > > Hello Chris, > > This feature seems like it would be useful, and looks correctly > implemented. However, the nomenclature used throughout the code and > documentation is member in place of slave. > Hi Mike, Sure, I'll fix that in v2. Thanks, Christophe > > Cheers, > M > > > NEWS | 1 + > > ofproto/bond.c | 5 ++++- > > ofproto/bond.h | 2 ++ > > vswitchd/bridge.c | 3 +++ > > vswitchd/vswitch.xml | 20 ++++++++++++++++++++ > > 5 files changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/NEWS b/NEWS > > index df633e8e2..07f72fd5a 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -16,6 +16,7 @@ v2.17.0 - xx xxx xxxx > > * Removed experimental tag for PMD Auto Load Balance. > > * New configuration knob 'other_config:n-offload-threads' to change the > > number of HW offloading threads. > > + * New configuration knob 'all_slaves_active' for non lacp bonds > > - DPDK: > > * EAL argument --socket-mem is no longer configured by default upon > > start-up. If dpdk-socket-mem and dpdk-alloc-mem are not specified, > > diff --git a/ofproto/bond.c b/ofproto/bond.c > > index cdfdf0b9d..8bf166a41 100644 > > --- a/ofproto/bond.c > > +++ b/ofproto/bond.c > > @@ -145,6 +145,7 @@ struct bond { > > struct eth_addr active_member_mac; /* MAC address of the active member. */ > > /* Legacy compatibility. */ > > bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */ > > + bool all_slaves_active; > > > > struct ovs_refcount ref_cnt; > > }; > > @@ -448,6 +449,7 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s) > > > > bond->updelay = s->up_delay; > > bond->downdelay = s->down_delay; > > + bond->all_slaves_active = s->all_slaves_active; > > > > if (bond->lacp_fallback_ab != s->lacp_fallback_ab_cfg) { > > bond->lacp_fallback_ab = s->lacp_fallback_ab_cfg; > > @@ -893,7 +895,8 @@ bond_check_admissibility(struct bond *bond, const void *member_, > > > > /* Drop all multicast packets on inactive members. */ > > if (eth_addr_is_multicast(eth_dst)) { > > - if (bond->active_member != member) { > > + if (bond->active_member != member && > > + bond->all_slaves_active == false) { > > goto out; > > } > > } > > diff --git a/ofproto/bond.h b/ofproto/bond.h > > index 1683ec878..4e8c1872a 100644 > > --- a/ofproto/bond.h > > +++ b/ofproto/bond.h > > @@ -62,6 +62,8 @@ struct bond_settings { > > ovs run. */ > > bool use_lb_output_action; /* Use lb_output action. Only applicable for > > bond mode BALANCE TCP. */ > > + bool all_slaves_active; /* For non LACP bond, also accept multicast > > + packets on secondary interface. */ > > }; > > > > /* Program startup. */ > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 5223aa897..53be7ddf6 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -4615,6 +4615,9 @@ port_configure_bond(struct port *port, struct bond_settings *s) > > s->use_lb_output_action = (s->balance == BM_TCP) > > && smap_get_bool(&port->cfg->other_config, > > "lb-output-action", false); > > + /* all_slaves_active is disabled by default */ > > + s->all_slaves_active = smap_get_bool(&port->cfg->other_config, > > + "all_slaves_active", false); > > } > > > > /* Returns true if 'port' is synthetic, that is, if we constructed it locally > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > > index 0c6632617..d420397a3 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -2083,6 +2083,26 @@ > > <code>true</code>). > > </column> > > > > + <column name="other_config" key="all_slaves_active" > > + type='{"type": "boolean"}'> > > + <p> > > + Enable/Disable delivery of broadcast/multicast packets on secondary > > + interface of a bond. Relevant only when <ref column="lacp"/> is > > + <code>off</code>. > > + </p> > > + > > + <p> > > + This parameter is identical to <code>all_slaves_active</code> for > > + kernel bonds. This is useful when we need to share 2 physical functions > > + between an ovs bond and a linux bond, as 2 LACP sessions can't > > + be negotiated over the same physical link. LACP session will be managed > > + by the kernel bond, while an active-active bond is used for ovs. > > + In that particular configuration, the physical switch will send a unique > > + copy of a broadcast packet over the 2 physical links, eventually to > > + the secondary link of the bond. > > + </p> > > + </column> > > + > > <group title="Link Failure Detection"> > > <p> > > An important part of link bonding is detecting that links are down so > > -- > > 2.35.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
Changes in v2: * rename parameter "all_slaves_active" to "all_members_active" * fix doc warnings Thanks, Christophe
On 11 Apr 2022, at 15:52, Christophe Fontaine wrote: > Changes in v2: > * rename parameter "all_slaves_active" to "all_members_active" > * fix doc warnings > > Thanks, > Christophe I did not review the code, but what is clearly missing are the appropriate test cases. //Eelco
Hi Eelco, Mike, all, Changes in v3 * Added unit test Changes in v2 * rename parameter "all_slaves_active" to "all_members_active" * fix doc warnings Thanks, Christophe
diff --git a/NEWS b/NEWS index df633e8e2..07f72fd5a 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,7 @@ v2.17.0 - xx xxx xxxx * Removed experimental tag for PMD Auto Load Balance. * New configuration knob 'other_config:n-offload-threads' to change the number of HW offloading threads. + * New configuration knob 'all_slaves_active' for non lacp bonds - DPDK: * EAL argument --socket-mem is no longer configured by default upon start-up. If dpdk-socket-mem and dpdk-alloc-mem are not specified, diff --git a/ofproto/bond.c b/ofproto/bond.c index cdfdf0b9d..8bf166a41 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -145,6 +145,7 @@ struct bond { struct eth_addr active_member_mac; /* MAC address of the active member. */ /* Legacy compatibility. */ bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */ + bool all_slaves_active; struct ovs_refcount ref_cnt; }; @@ -448,6 +449,7 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s) bond->updelay = s->up_delay; bond->downdelay = s->down_delay; + bond->all_slaves_active = s->all_slaves_active; if (bond->lacp_fallback_ab != s->lacp_fallback_ab_cfg) { bond->lacp_fallback_ab = s->lacp_fallback_ab_cfg; @@ -893,7 +895,8 @@ bond_check_admissibility(struct bond *bond, const void *member_, /* Drop all multicast packets on inactive members. */ if (eth_addr_is_multicast(eth_dst)) { - if (bond->active_member != member) { + if (bond->active_member != member && + bond->all_slaves_active == false) { goto out; } } diff --git a/ofproto/bond.h b/ofproto/bond.h index 1683ec878..4e8c1872a 100644 --- a/ofproto/bond.h +++ b/ofproto/bond.h @@ -62,6 +62,8 @@ struct bond_settings { ovs run. */ bool use_lb_output_action; /* Use lb_output action. Only applicable for bond mode BALANCE TCP. */ + bool all_slaves_active; /* For non LACP bond, also accept multicast + packets on secondary interface. */ }; /* Program startup. */ diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 5223aa897..53be7ddf6 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -4615,6 +4615,9 @@ port_configure_bond(struct port *port, struct bond_settings *s) s->use_lb_output_action = (s->balance == BM_TCP) && smap_get_bool(&port->cfg->other_config, "lb-output-action", false); + /* all_slaves_active is disabled by default */ + s->all_slaves_active = smap_get_bool(&port->cfg->other_config, + "all_slaves_active", false); } /* Returns true if 'port' is synthetic, that is, if we constructed it locally diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 0c6632617..d420397a3 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2083,6 +2083,26 @@ <code>true</code>). </column> + <column name="other_config" key="all_slaves_active" + type='{"type": "boolean"}'> + <p> + Enable/Disable delivery of broadcast/multicast packets on secondary + interface of a bond. Relevant only when <ref column="lacp"/> is + <code>off</code>. + </p> + + <p> + This parameter is identical to <code>all_slaves_active</code> for + kernel bonds. This is useful when we need to share 2 physical functions + between an ovs bond and a linux bond, as 2 LACP sessions can't + be negotiated over the same physical link. LACP session will be managed + by the kernel bond, while an active-active bond is used for ovs. + In that particular configuration, the physical switch will send a unique + copy of a broadcast packet over the 2 physical links, eventually to + the secondary link of the bond. + </p> + </column> + <group title="Link Failure Detection"> <p> An important part of link bonding is detecting that links are down so
This config param allows the delivery of broadcast and multicast packets to the secondary interface of non-lacp bonds, identical to the same option for kernel bonds. Tested with an ovs-dpdk balance-slb bond with 2 virtual functions, and a kernel bond with LACP on 2 other virtual functions. Scapy sending 10 broadcast packets from 10 different mac addresses: - with ovs-vsctl set port dpdkbond other_config:all_slaves_active=false (default unmodified behavior) 5 packets are received - with other_config:all_slaves_active=true, all 10 packets are received. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1720935 Signed-off-by: Christophe Fontaine <cfontain@redhat.com> --- NEWS | 1 + ofproto/bond.c | 5 ++++- ofproto/bond.h | 2 ++ vswitchd/bridge.c | 3 +++ vswitchd/vswitch.xml | 20 ++++++++++++++++++++ 5 files changed, 30 insertions(+), 1 deletion(-)