Ilya Maximets <[email protected]> writes: > 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.
Yes, I'll mention in v3 >> 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? That probably makes more sense. >> + 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. ACK (notice they are kindof just c/p of each other) >> + 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. These kinds of comments were there for me when I was building the test case; I can remove them all (plus there's a spelling mistake in here, too - Esnure.) >> +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. Yes. >> + >> +dnl Ensure normal mode. >> +AT_CHECK([ovs-ofctl del-flows br0], [0], [ignore]) > > We should not need that. You're right. I think I had copied it over from a machine I was testing on. >> +AT_CHECK([ovs-ofctl add-flow br0 action=NORMAL], [0], [ignore]) > > '[0], [ignore]' not needed here. ack >> + >> +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? Okay, I'll fix up this verification part. >> + >> +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. I added it because I wasn't confident that the bundle correction would happen within the same update interval. I guess I was overthinking it - I can drop that. >> + >> +dnl Test flood behavior > > This line seems unnecessary. > >> +dnl Send a broadcast packet on p2, verify it floods to p1 > > Period at the end. ACK >> +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. I originally had it '\' but changed it to dnl because previous times I used '\' in tests I was asked to use dnl instead. I can switch. >> + -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. ACK, can do. >> + >> +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. Yes, it was c/p and just rename (and I missed one). > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
