I don't recall if we arrived at a decision about this.
Daniele
2014-11-24 15:02 GMT+01:00 Daniele Venturino <daniele.ventur...@m3s.it>:
>
> commit bacdb85ad82f981697245eefb40a3b360cfe379b
>> Author: Alex Wang <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>
>> Acked-by: Ben Pfaff <b...@nicira.com>
>> Acked-by: Joe Stringer <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>
> 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> ha scritto:
>
>
>
> Sent from my iPhone
>
> On Sep 11, 2014, at 2:08 AM, Daniele Venturino <daniele.ventur...@m3s.it>
> wrote:
>
>
> Il giorno 10/set/2014, alle ore 18:54, Jarno Rajahalme <
> 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>
> wrote:
>
> See commit bacdb85ad82f981697245eefb40a3b360cfe379b.
>
> Signed-off by: Daniele Venturino <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
