[ovs-dev] [PATCH] lib/rstp: Make rstp-disabled port forward stp bpdu packets.

Jarno Rajahalme jrajahalme at nicira.com
Thu Sep 11 14:11:18 UTC 2014



Sent from my iPhone

> On Sep 11, 2014, at 2:08 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:
> 
> 
>> Il giorno 10/set/2014, alle ore 18:54, Jarno Rajahalme <jrajahalme at 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.venturino at m3s.it> wrote:
>>> 
>>> See commit bacdb85ad82f981697245eefb40a3b360cfe379b.
>>> 
>>> Signed-off by: Daniele Venturino <daniele.venturino at 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
> 



More information about the dev mailing list