Hi, Aaron. Thanks for the patch! The code looks good to me in general, but I have a few small comments below.
On 5/26/26 9:01 PM, Aaron Conole wrote: > When STP/RSTP are in a disabled transition on a bridge, the sets will call > bundle_update() for each port as it should transition from FORWARDING to > DISABLED. However, this happens while the states are still non-NULL, > which causes bundle_update() to assume that the STP/RSTP status will > remain as forwarding. After this happens, the bundle->floodable flag > will remain 'false' and NORMAL action flood attempts will skip over > the ports. > > In the case of RSTP, the state processing is a bit more complex, so we > do a final bundle_update pass to reset the floodable flag. We could > choose to do a more simplistic fix in STP case by releasing the > ofproto->stp object early; however to keep it consistent, the fix will > adopt the same approach of doing a final pass for bundle updates. I think, this is not the only case. If somehow the port is configured for STP, but it is already in the disabled state, then its state will not change while deconfiguring STP on this part and the bundle_update() will not be called at all. So, we need to call bundle_update() at the end, after the ofproto->stp is cleared anyway. > > Reported-at: https://redhat.atlassian.net/browse/FDP-3852 > Fixes: 21f7563cef5f ("ovs-vswitchd: Add support for 802.1D STP.") > Fixes: 9efd308e957c ("Rapid Spanning Tree Protocol (IEEE 802.1D).") > Signed-off-by: Aaron Conole <[email protected]> > --- > ofproto/ofproto-dpif.c | 17 +++++++++ > tests/ofproto-dpif.at | 82 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 99 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index a02afe8ef3..26a6da2541 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2813,12 +2813,21 @@ set_rstp(struct ofproto *ofproto_, const struct > ofproto_rstp_settings *s) > rstp_set_bridge_transmit_hold_count(ofproto->rstp, > s->transmit_hold_count); > } else { > + struct ofbundle *bundle; > struct ofport *ofport; > + > HMAP_FOR_EACH (ofport, hmap_node, &ofproto->up.ports) { > set_rstp_port(ofport, NULL); > } > rstp_unref(ofproto->rstp); > ofproto->rstp = NULL; > + > + /* During processing above, RSTP state machine may have incorrectly > + * recalculated the floodable state, due to a transition. > Recalculate > + * the bundle floodable flag now that RSTP states are quiet. */ I'm not sure we should call it "incorrect". The calculation for the bundle is correct at the moment of updating one port. And, IIUC, all the ports are in disabled state at this point, so every bundle is non-foodable here. And given the previous comment as well, maybe a shorter comment would suffice: /* Now that ofproto no longer has RSTP configured, bundles need to * have their 'floodable' states updated. */ WDYT? > + HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) { > + bundle_update(bundle); > + } > } > } > > @@ -2957,6 +2966,7 @@ set_stp(struct ofproto *ofproto_, const struct > ofproto_stp_settings *s) > stp_set_max_age(ofproto->stp, s->max_age); > stp_set_forward_delay(ofproto->stp, s->fwd_delay); > } else { > + struct ofbundle *bundle; > struct ofport *ofport; > > HMAP_FOR_EACH (ofport, hmap_node, &ofproto->up.ports) { > @@ -2965,6 +2975,13 @@ set_stp(struct ofproto *ofproto_, const struct > ofproto_stp_settings *s) > > stp_unref(ofproto->stp); > ofproto->stp = NULL; > + > + /* During processing above, STP state machine may have incorrectly > + * recalculated the floodable state, due to a transition. > Recalculate > + * the bundle floodable flag now that STP states are quiet. */ Same here. > + HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) { > + bundle_update(bundle); > + } > } > > return 0; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index dcab7bba45..def246e877 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -13651,3 +13651,85 @@ AT_CHECK([ovs-appctl coverage/read-counter > revalidate_missing_dp_flow], [0], > > OVS_VSWITCHD_STOP(["/failed to flow_del (No such file or directory)/d"]) > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - bundle floodable flag after STP disabled]) > +AT_KEYWORDS([stp bundle floodable]) > +OVS_VSWITCHD_START > + > +dnl Setup STP Not sure we need this comment. But if we want it, should add a period at the end. > +AT_CHECK([ovs-vsctl set bridge br0 stp_enable=true]) > + > +dnl Add ports for forwarding. Also a questionable comment. > +AT_CHECK([ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy]) > +AT_CHECK([ovs-vsctl add-port br0 p2 -- set interface p2 type=dummy]) Should use add_of_ports instead. > + > +dnl Ensure normal mode. > +AT_CHECK([ovs-ofctl del-flows br0], [0], [ignore]) We should not need that. > +AT_CHECK([ovs-ofctl add-flow br0 action=NORMAL], [0], [ignore]) '[0], [ignore]' not needed here. > + > +dnl Verify bundle is non-floodable. > +AT_CHECK([ovs-appctl dpif/show | grep -A5 br0], [0], [ignore]) I'm not sure this verifies anything. Should not ignore the output, I guess? > + > +dnl Esnure STP reached forwarding state. > +OVS_WAIT_UNTIL([ovs-appctl stp/show br0 | grep "p1.*forwarding"]) > + > +dnl Capture port details in case. > +AT_CHECK([ovs-ofctl dump-ports-desc br0], [0], [ignore]) > + > +dnl Now disable STP. > +AT_CHECK([ovs-vsctl set bridge br0 stp_enable=false]) > +OVS_WAIT_UNTIL([ovs-vsctl get bridge br0 stp_enable | grep false]) This waiting doesn't do anything, OVS doesn't overwrite the value and the database write before this command is synchronous, i.e. it waits for the value to be written. I suppose here should be the same command as 'Verify bundle is non-floodable.', but the opposite, of course, and inside the WAIT, e.g. WAIT_FOR_OUTPUT macro. > + > +dnl Test flood behavior This line seems unnecessary. > +dnl Send a broadcast packet on p2, verify it floods to p1 Period at the end. > +AT_CHECK([ovs-appctl ofproto/trace br0 dnl > + 'in_port=p2,eth_src=00:00:00:00:00:01,eth_dst=ff:ff:ff:ff:ff:ff' > dnl Better to use standard '\' here, they are easier to look at in the logs. > + -generate], > + [0], [stdout]) > + > +dnl Check that output includes p1 (proves it's floodable) > +AT_CHECK([grep 'Datapath actions: 100,1' stdout], [0], [ignore]) May be better to use tail -1 and match on the output, in this case we'll actually see what was the result in case the check fails. > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - bundle floodable flag after RSTP disabled]) RSTP test is mostly the same as STP, and so are my comments. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
