commit bacdb85ad82f981697245eefb40a3b360cfe379b Author: Alex Wang <al...@nicira.com <mailto:al...@nicira.com>> Date: Tue Jul 15 18:52:19 2014 -0700 stp: Make stp-disabled port forward stp bpdu packets. Commit 0d1cee123a84 (stp: Fix bpdu tx problem in listening state) makes ovs drop the stp bpdu packets if stp is not enabled on the input port. However, when pif bridge is used and stp is enabled on the integration bridge. The flow translation of stp bpdu packets will go through a level of resubmission which changes the input port to the corresponding peer port. Since, the patch port on the pif bridge does not have stp enabled, the flow translation will drop the bpdu packets. This commit fixes the issue by making ovs forward stp bpdu packets on stp-disabled port. VMware-BZ: #1284695 Signed-off-by: Alex Wang <al...@nicira.com <mailto:al...@nicira.com>> Acked-by: Ben Pfaff <b...@nicira.com <mailto:b...@nicira.com>> Acked-by: Joe Stringer <joestrin...@nicira.com <mailto:joestrin...@nicira.com>>
Since the problem was that the bpdu packets were dropped if stp was not enabled on the input port, I think we can add the distinction between a port which has a disabled state with stp (or rstp) enabled and a port with stp (or rstp) not enabled in stp_should_forward_bpdu() and rstp_should_manage_bpdu() as well. If you think this is a good solution I’ve already prepared a patch. Regards, Daniele > Il giorno 20/nov/2014, alle ore 23:39, Jarno Rajahalme > <jrajaha...@nicira.com> ha scritto: > > Now that there is a difference between non-configured STP/RSTP and disabled > STP/RSTP the situation may be different. > If they still after this patch (considering the recent changes) will always > return a fixed value, then yes. > > You should consider how the calling sites use these functions, as some of > them changed in the last series that was merged recently. > > Jarno > > On Nov 20, 2014, at 11:35 AM, Daniele Venturino <daniele.ventur...@m3s.it > <mailto:daniele.ventur...@m3s.it>> wrote: > >> We didn’t merge this patch back in september. >> Were you suggesting to remove both stp_should_forward_bpdu() and >> rstp_should_manage_bpdu() ? >> >> Regards, >> Daniele >> >>> Il giorno 11/set/2014, alle ore 16:11, Jarno Rajahalme >>> <jrajaha...@nicira.com <mailto:jrajaha...@nicira.com>> ha scritto: >>> >>> >>> >>> Sent from my iPhone >>> >>> On Sep 11, 2014, at 2:08 AM, Daniele Venturino <daniele.ventur...@m3s.it >>> <mailto:daniele.ventur...@m3s.it>> wrote: >>> >>>> >>>> Il giorno 10/set/2014, alle ore 18:54, Jarno Rajahalme >>>> <jrajaha...@nicira.com <mailto:jrajaha...@nicira.com>> ha scritto: >>>> >>>>> Daniele, >>>>> >>>>> See comments below. >>>>> >>>>> Also, it would be preferable to send related changes, or multiple >>>>> unrelated changes to any given subsystem, as a series of patches instead >>>>> of individual ones. “git format-patch” does that automatically. >>>>> >>>>> Jarno >>>>> >>>> >>>> I’ll send the next group of patches as a series. >>>> >>>>> On Sep 10, 2014, at 1:28 AM, Daniele Venturino <daniele.ventur...@m3s.it >>>>> <mailto:daniele.ventur...@m3s.it>> wrote: >>>>> >>>>>> See commit bacdb85ad82f981697245eefb40a3b360cfe379b. >>>>>> >>>>>> Signed-off by: Daniele Venturino <daniele.ventur...@m3s.it >>>>>> <mailto:daniele.ventur...@m3s.it>> >>>>>> >>>>>> --- >>>>>> lib/rstp.h | 42 >>>>>> +++++++++++++++++++++++++++++++++++++++--- >>>>>> ofproto/ofproto-dpif-xlate.c | 6 +++--- >>>>>> 2 files changed, 42 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/lib/rstp.h b/lib/rstp.h >>>>>> index 364a181..b15d22f 100644 >>>>>> --- a/lib/rstp.h >>>>>> +++ b/lib/rstp.h >>>>>> @@ -99,6 +99,38 @@ typedef uint64_t rstp_identifier; >>>>>> >>>>>> #define RSTP_PORT_ID_FMT "%04"PRIx16 >>>>>> >>>>>> +/* State of an RSTP port. >>>>>> + * >>>>>> + * The RSTP_DISABLED state means that the port is disabled by >>>>>> management. >>>>>> + * In our implementation, this state means that the port does not >>>>>> + * participate in the spanning tree, but it still forwards traffic as if >>>>>> + * it were in the RSTP_FORWARDING state. This may be different from >>>>>> + * other implementations. >>>>>> + * >>>>>> + * The following diagram describes the various states and what they are >>>>>> + * allowed to do in OVS: >>>>>> + * >>>>>> + * FWD LRN TX_BPDU RX_BPDU FWD_BPDU >>>>>> + * --- --- ------- ------- -------- >>>>>> + * Disabled Y - - - Y >>>>>> + * Discarding - - Y Y Y >>>>>> + * Learning - Y Y Y Y >>>>>> + * Forwarding Y Y Y Y Y >>>>>> + * >>>>>> + * >>>>>> + * FWD: the port should forward any incoming non-rstp-BPDU >>>>>> + * packets. >>>>>> + * >>>>>> + * LRN: the port should conduct MAC learning on packets >>>>>> received. >>>>>> + * >>>>>> + * TX_BPDU/RX_BPDU: the port could generate/consume bpdus. >>>>>> + * >>>>>> + * FWD_BPDU: the port should should always forward the BPDUS, >>>>>> + * whether they are generated by the port or received >>>>>> + * as incoming packets. >>>>>> + * >>>>>> + */ >>>>>> + >>>>>> enum rstp_state { >>>>>> RSTP_DISABLED, >>>>>> RSTP_LEARNING, >>>>>> @@ -128,7 +160,7 @@ const char *rstp_state_name(enum rstp_state); >>>>>> const char *rstp_port_role_name(enum rstp_port_role); >>>>>> static inline bool rstp_forward_in_state(enum rstp_state); >>>>>> static inline bool rstp_learn_in_state(enum rstp_state); >>>>>> -static inline bool rstp_should_manage_bpdu(enum rstp_state state); >>>>>> +static inline bool rstp_should_forward_bpdu(enum rstp_state state); >>>>>> >>>>>> void rstp_init(void) >>>>>> OVS_EXCLUDED(rstp_mutex); >>>>>> @@ -255,12 +287,16 @@ void rstp_port_set_state(struct rstp_port *p, enum >>>>>> rstp_state state) >>>>>> /* Inline functions. */ >>>>>> /* Returns true if 'state' is one in which BPDU packets should be >>>>>> received >>>>>> * and transmitted on a port, false otherwise. >>>>>> + * >>>>>> + * Returns true if 'state' is RSTP_DISABLED, since in that case the >>>>>> port does >>>>>> + * not generate the bpdu and should just forward it (e.g. patch port on >>>>>> pif >>>>>> + * bridge). >>>>>> */ >>>>>> static inline bool >>>>>> -rstp_should_manage_bpdu(enum rstp_state state) >>>>>> +rstp_should_forward_bpdu(enum rstp_state state) >>>>>> { >>>>>> return (state == RSTP_DISCARDING || state == RSTP_LEARNING || >>>>>> - state == RSTP_FORWARDING); >>>>>> + state == RSTP_FORWARDING || state == RSTP_DISABLED); >>>>>> } >>>>> >>>>> This now returns true for all the possible states, so this should be >>>>> removed. >>>>> >>>>>> >>>>>> /* Returns true if 'state' is one in which packets received on a port >>>>>> should >>>>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>>>>> index 786494d..51d25d4 100644 >>>>>> --- a/ofproto/ofproto-dpif-xlate.c >>>>>> +++ b/ofproto/ofproto-dpif-xlate.c >>>>>> @@ -1193,9 +1193,9 @@ xport_rstp_forward_state(const struct xport *xport) >>>>>> } >>>>>> >>>>>> static bool >>>>>> -xport_rstp_should_manage_bpdu(const struct xport *xport) >>>>>> +xport_rstp_should_forward_bpdu(const struct xport *xport) >>>>>> { >>>>>> - return rstp_should_manage_bpdu(xport_get_rstp_port_state(xport)); >>>>>> + return rstp_should_forward_bpdu(xport_get_rstp_port_state(xport)); >>>>>> } >>>>>> >>>>> >>>>> Ditto. >>>> >>>> You’re right. I left it there to show that the check was done on both >>>> protocols, but I guess it can be removed since it’s always true. >>>> >>>>> >>>>>> static void >>>>>> @@ -2493,7 +2493,7 @@ compose_output_action__(struct xlate_ctx *ctx, >>>>>> ofp_port_t ofp_port, >>>>>> } else if (check_stp) { >>>>>> if (is_stp(&ctx->base_flow)) { >>>>>> if (!xport_stp_should_forward_bpdu(xport) && >>>>>> - !xport_rstp_should_manage_bpdu(xport)) { >>>>>> + !xport_rstp_should_forward_bpdu(xport)) { >>>>> >>>>> Maybe the same is true for sport_stp_should_forward_bpdu()? At least the >>>>> comment in stp.h states that BPDUs should always be forwarded. >>>> >>>> Right now STP forwards BPDUs in all states but the initial state >>>> STP_BLOCKING. >>> >>> I don't recall it now offhand, but that state could be transitory and in >>> effect only during initialization, I.e., never when BPDUs are received. >>> >>> Jarno >>> >>>> >>>>> >>>>>> if (ctx->xbridge->stp != NULL) { >>>>>> xlate_report(ctx, "STP not in listening state, " >>>>>> "skipping bpdu output"); >>>>>> -- >>>>>> 1.8.1.2 >>>> >>>> Daniele >> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
