[ovs-dev] [PATCH 02/17] rstp: add admin-point-to-point and admin-port-state setters.

Daniele Venturino daniele.venturino at m3s.it
Fri Nov 14 22:50:06 UTC 2014


You're right, it's used only in rstp_port_set_admin_point_to_point_mac__()
where it's set, if the old value change. The action on oper_point_to_point_mac
it's inside that function.

Maybe it would be clearer to have a function
"update_oper_point_to_point_mac__()"
that updates oper_point_to_point_mac accordingly to admin_point_to_point_mac,
instead of calling rstp_port_set_oper_point_to_point_mac__() in
rstp_port_set_admin_point_to_point_mac__()
? Something like:

diff --git a/lib/rstp.c b/lib/rstp.c
> index 46414bf..03d5b06 100644
> --- a/lib/rstp.c
> +++ b/lib/rstp.c
> @@ -1212,29 +1212,22 @@ static void
> rstp_port_set_admin_point_to_point_mac__(struct rstp_port *port,
>      VLOG_DBG("%s, port %u: set RSTP port admin-point-to-point-mac to %d",
>              port->rstp->name, port->port_number, admin_p2p_mac_state);
> -    if (port->admin_point_to_point_mac != admin_p2p_mac_state) {
> -        if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_TRUE) {
> -            port->admin_point_to_point_mac = admin_p2p_mac_state;
> -            rstp_port_set_oper_point_to_point_mac__(port,
> -                    RSTP_OPER_P2P_MAC_STATE_ENABLED);
> -        } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_FALSE)
> {
> -            port->admin_point_to_point_mac = admin_p2p_mac_state;
> -            rstp_port_set_oper_point_to_point_mac__(port,
> -                    RSTP_OPER_P2P_MAC_STATE_DISABLED);
> -        } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO) {
> -            /* If adminPointToPointMAC is set to Auto, then the value of
> -             * operPointToPointMAC is determined in accordance with the
> -             * specific procedures defined for the MAC entity concerned,
> as
> -             * defined in 6.5. If these procedures determine that the MAC
> -             * entity is connected to a point-to-point LAN, then
> -             * operPointToPointMAC is set TRUE; otherwise it is set FALSE.
> -             * In the absence of a specific definition of how to determine
> -             * whether the MAC is connected to a point-to-point LAN or
> not,
> -             * the value of operPointToPointMAC shall be FALSE. */
> -            port->admin_point_to_point_mac = admin_p2p_mac_state;
> -            rstp_port_set_oper_point_to_point_mac__(port,
> -                    RSTP_OPER_P2P_MAC_STATE_DISABLED);
> -        }
> +    /* If adminPointToPointMAC is set to Auto, then the value of
> +     * operPointToPointMAC is determined in accordance with the
> +     * specific procedures defined for the MAC entity concerned, as
> +     * defined in 6.5. If these procedures determine that the MAC
> +     * entity is connected to a point-to-point LAN, then
> +     * operPointToPointMAC is set TRUE; otherwise it is set FALSE.
> +     * In the absence of a specific definition of how to determine
> +     * whether the MAC is connected to a point-to-point LAN or not,
> +     * the value of operPointToPointMAC shall be FALSE. */
> +
> +    if (port->admin_point_to_point_mac != admin_p2p_mac_state
> +        && (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_TRUE
> +            || admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_FALSE
> +            || admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO)) {
> +        port->admin_point_to_point_mac = admin_p2p_mac_state;
> +        update_oper_point_to_point_mac__(port)
>      }
>  }


where update_oper_point_to_point_mac__() sets oper_point_to_point_mac
to RSTP_OPER_P2P_MAC_STATE_ENABLED if admin_point_to_point_mac
is RSTP_ADMIN_P2P_MAC_FORCE_TRUE, it sets oper_point_to_point_mac
to RSTP_OPER_P2P_MAC_STATE_DISABLED if admin_point_to_point_mac
is RSTP_ADMIN_P2P_MAC_FORCE_FALSE and it sets oper_point_to_point_mac
to RSTP_OPER_P2P_MAC_STATE_DISABLED if admin_point_to_point_mac
is RSTP_ADMIN_P2P_MAC_AUTO (for now, later on it will be the correct auto
procedure).

Regards,
Daniele

2014-11-14 23:30 GMT+01:00 Jarno Rajahalme <jrajahalme at nicira.com>:

>
> On Nov 14, 2014, at 11:09 AM, Daniele Venturino <daniele.venturino at m3s.it>
> wrote:
>
>
>
> 2014-11-14 19:46 GMT+01:00 Jarno Rajahalme <jrajahalme at nicira.com>:
>
>>
>> On Nov 14, 2014, at 9:36 AM, Daniele Venturino <daniele.venturino at m3s.it>
>> wrote:
>>
>> A possible commit message:
>> "Admin-port-state is the Administrative Bridge Port state variable
>> defined in the 802.1D-2004 standard.
>> It can be set to include or exclude a port from the active topology by
>> management (see 7.4).
>>
>>
>> IMO we should use the user-visible (configuration database) names in the
>> commit messages ('rstp-admin-p2p-mac' and 'rstp-admin-port-state’).
>>
>
> I think you're right about this. Let's use the visible names.
>
>
>>
>> operPointToPointMAC and adminPointToPointMAC are a pair of parameters
>> that permit inspection of, and control over, the administrative and
>> operational state of the point-to-point status of the MAC entity by the MAC
>> Relay Entity.
>> adminPointToPointMAC can be set by management and its value is reflected
>> on operPointToPointMAC."
>>
>>
>> I still do not see ‘admin_point_to_point_mac’ being used for anything,
>> and I do not see operPointToPointMAC anywhere in this patch. Are these
>> added in the following patches? If so, we should mention that in the commit
>> message.
>>
>
> admin_point_to_point_mac is not used in other places, but it's a
> per-bridge variable and should be present.
> operPointToPointMAC is named oper_point_to_point_mac in the code.
> They're already present in the rstp_port struct in lib/rstp-common.h.
> There's a little of misalignment between the naming in the code and the
> naming in the standard I guess.
>
>
> I forgot about this and pushed this patch into master already.
>
> However, could you show me where in the code the variable ‘
> admin_point_to_point_mac’ is read and then used for anything. I only see
> it being set (and the old value being compared against while doing so). In
> short, it seems to me like a write-only variable. I guess it should have
> some effect on ‘opera_point_to_point_mac’?
>
>   Jarno
>
>
>> The AUTO mode should work as reported in the 802.1D-2004 standard
>> (6.4.3), by setting operPointToPointMAC to false if the described procedure
>> is not available.
>> For now, I would only add this change:
>>
>> diff --git a/lib/rstp.c b/lib/rstp.c
>>> index 144f2ba..2a2a60c 100644
>>> --- a/lib/rstp.c
>>> +++ b/lib/rstp.c
>>> @@ -1217,8 +1217,18 @@ static void
>>> rstp_port_set_admin_point_to_point_mac__(struct rstp_port *port,
>>>              rstp_port_set_oper_point_to_point_mac__(port,
>>>                      RSTP_OPER_P2P_MAC_STATE_DISABLED);
>>>          } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO) {
>>> +            /* If adminPointToPointMAC is set to Auto, then the value of
>>> +             * operPointToPointMAC is determined in accordance with the
>>> +             * specific procedures defined for the MAC entity
>>> concerned, as
>>> +             * defined in 6.5. If these procedures determine that the
>>> MAC
>>> +             * entity is connected to a point-to-point LAN, then
>>> +             * operPointToPointMAC is set TRUE; otherwise it is set
>>> FALSE.
>>> +             * In the absence of a specific definition of how to
>>> determine
>>> +             * whether the MAC is connected to a point-to-point LAN or
>>> not,
>>> +             * the value of operPointToPointMAC shall be FALSE. */
>>>              port->admin_point_to_point_mac = admin_p2p_mac_state;
>>> -            /* FIXME add auto procedure*/
>>> +            rstp_port_set_oper_point_to_point_mac__(port,
>>> +                    RSTP_OPER_P2P_MAC_STATE_DISABLED);
>>>          }
>>>      }
>>>  }
>>
>>
>> but we are going to implement this mechanism soon.
>>
>>
>> OK, I added the above.
>>
>>   Jarno
>>
>>
>> Here an extract from the 802.1D-2004 standard for a better explanation:
>>
>> operPointToPointMAC: This parameter can take two values, as follows:
>> a) True. The MAC is connected to a point-to-point LAN; i.e., there is at
>> most one other system attached to the LAN.
>> b) False. The MAC is connected to a non-point-to-point LAN; i.e., there
>> can be more than one other system attached to the LAN.
>>
>> adminPointToPointMAC: This parameter can take three values, as follows:
>> a) ForceTrue. The administrator requires the MAC to be treated as if it
>> is connected to a point-to-point LAN, regardless of any indications to the
>> contrary that are generated by the MAC entity.
>> b) ForceFalse. The administrator requires the MAC to be treated as
>> connected to a non-point-to-point LAN, regardless of any indications to the
>> contrary that are generated by the MAC entity.
>> c) Auto. The administrator requires the point-to-point status of the MAC
>> to be determined in accordance with the specific MAC procedures defined in
>> 6.5.
>> If adminPointToPointMAC is set to ForceTrue, then operPointToPointMAC
>> shall be set True. If adminPointToPointMAC is set to ForceFalse, then
>> operPointToPointMAC shall be set False.
>> *If adminPointToPointMAC is set to Auto, then the value of
>> operPointToPointMAC is determined in accordance with the specific
>> procedures defined for the MAC entity concerned, as defined in 6.5. If
>> these **procedures determine that the MAC entity is connected to a
>> point-to-point LAN, then*
>> *operPointToPointMAC is set TRUE; otherwise it is set FALSE. In the
>> absence of a specific definition of how to determine whether the MAC is
>> connected to a point-to-point LAN or not, the value of*
>> *operPointToPointMAC shall be FALSE.*
>>
>> Regards,
>> Daniele
>>
>> 2014-11-14 0:07 GMT+01:00 Jarno Rajahalme <jrajahalme at nicira.com>:
>>
>>> This patch needs a proper commit message. The user visible changes are
>>> in the corresponding configuration settings. Maybe briefly introduce them
>>> in the commit message?
>>>
>>> More comments inline below.
>>>
>>> I have a working copy with the white-space changes, so if you provide
>>> the commit message and the answer to the AUTO question below, I can accept
>>> and push this patch.
>>>
>>> Thanks!
>>>
>>>   Jarno
>>>
>>> On Nov 6, 2014, at 7:31 AM, Daniele Venturino <daniele.venturino at m3s.it>
>>> wrote:
>>>
>>> > Signed-off-by: Daniele Venturino <daniele.venturino at m3s.it>
>>> > ---
>>> > lib/rstp-common.h        |  6 ------
>>> > lib/rstp.c               | 31 ++++++++++++++++++++++++++++++-
>>> > lib/rstp.h               |  9 ++++++++-
>>> > ofproto/ofproto-dpif.c   |  4 +++-
>>> > ofproto/ofproto.h        |  2 ++
>>> > utilities/ovs-vsctl.8.in |  9 +++++++++
>>> > vswitchd/bridge.c        | 10 ++++++++++
>>> > 7 files changed, 62 insertions(+), 9 deletions(-)
>>> >
>>> > diff --git a/lib/rstp-common.h b/lib/rstp-common.h
>>> > index 587f88c..cd43079 100644
>>> > --- a/lib/rstp-common.h
>>> > +++ b/lib/rstp-common.h
>>> > @@ -240,12 +240,6 @@ struct rstp_bpdu {
>>> >     uint8_t padding[7];
>>> > });
>>> >
>>> > -enum rstp_admin_point_to_point_mac_state {
>>> > -    RSTP_ADMIN_P2P_MAC_FORCE_TRUE,
>>> > -    RSTP_ADMIN_P2P_MAC_FORCE_FALSE,
>>> > -    RSTP_ADMIN_P2P_MAC_FORCE_AUTO
>>> > -};
>>> > -
>>> > enum rstp_info_is {
>>> >     INFO_IS_DISABLED,
>>> >     INFO_IS_RECEIVED,
>>> > diff --git a/lib/rstp.c b/lib/rstp.c
>>> > index 0f96749..e3007e2 100644
>>> > --- a/lib/rstp.c
>>> > +++ b/lib/rstp.c
>>> > @@ -110,6 +110,9 @@ static void rstp_port_set_admin_edge__(struct
>>> rstp_port *, bool admin_edge)
>>> >     OVS_REQUIRES(rstp_mutex);
>>> > static void rstp_port_set_auto_edge__(struct rstp_port *, bool
>>> auto_edge)
>>> >     OVS_REQUIRES(rstp_mutex);
>>> > +static void rstp_port_set_admin_point_to_point_mac__(struct rstp_port
>>> *,
>>> > +        enum rstp_admin_point_to_point_mac_state admin_p2p_mac_state)
>>> > +    OVS_REQUIRES(rstp_mutex);
>>> > static void rstp_port_set_mcheck__(struct rstp_port *, bool mcheck)
>>> >     OVS_REQUIRES(rstp_mutex);
>>> > static void reinitialize_port__(struct rstp_port *p)
>>> > @@ -922,6 +925,8 @@ rstp_port_set_administrative_bridge_port__(struct
>>> rstp_port *p,
>>> >                                            uint8_t admin_port_state)
>>> >     OVS_REQUIRES(rstp_mutex)
>>> > {
>>> > +    VLOG_DBG("%s, port %u: set RSTP port admin-port-state to %d",
>>> > +             p->rstp->name, p->port_number, admin_port_state);
>>>
>>> Add an empty line here.
>>>
>>> >     if (admin_port_state == RSTP_ADMIN_BRIDGE_PORT_STATE_DISABLED
>>> >         || admin_port_state == RSTP_ADMIN_BRIDGE_PORT_STATE_ENABLED) {
>>> >
>>> > @@ -1120,6 +1125,27 @@ rstp_port_set_auto_edge__(struct rstp_port
>>> *port, bool auto_edge)
>>> >     }
>>> > }
>>> >
>>> > +/* Sets the port admin_point_to_point_mac parameter. */
>>> > +static void rstp_port_set_admin_point_to_point_mac__(struct rstp_port
>>> *port,
>>> > +        enum rstp_admin_point_to_point_mac_state admin_p2p_mac_state)
>>> > +    OVS_REQUIRES(rstp_mutex)
>>> > +{
>>> > +    VLOG_DBG("%s, port %u: set RSTP port admin-point-to-point-mac to
>>> %d",
>>> > +            port->rstp->name, port->port_number, admin_p2p_mac_state);
>>>
>>> Add an empty line here.
>>>
>>> > +    if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_TRUE) {
>>> > +        port->admin_point_to_point_mac =  admin_p2p_mac_state;
>>>
>>> Remove extra space after ‘=‘.
>>>
>>> > +        rstp_port_set_oper_point_to_point_mac__(port,
>>> > +                RSTP_OPER_P2P_MAC_STATE_ENABLED);
>>> > +    } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_FALSE)
>>> {
>>> > +        port->admin_point_to_point_mac =  admin_p2p_mac_state;
>>>
>>> Remove extra space after ‘=‘.
>>>
>>> > +        rstp_port_set_oper_point_to_point_mac__(port,
>>> > +                RSTP_OPER_P2P_MAC_STATE_DISABLED);
>>> > +    } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO) {
>>> > +        port->admin_point_to_point_mac = admin_p2p_mac_state;
>>> > +        /* FIXME fix auto behaviour */
>>>
>>> Elaborate on this. How the AUTO mode should work? Why not implement it
>>> now?
>>>
>>> > +    }
>>> > +}
>>> > +
>>> > /* Sets the port mcheck parameter.
>>> >  * [17.19.13] May be set by management to force the Port Protocol
>>> Migration
>>> >  * state machine to transmit RST BPDUs for a MigrateTime (17.13.9)
>>> period, to
>>> > @@ -1289,7 +1315,8 @@ rstp_port_get_status(const struct rstp_port *p,
>>> uint16_t *id,
>>> > void
>>> > rstp_port_set(struct rstp_port *port, uint16_t port_num, int priority,
>>> >               uint32_t path_cost, bool is_admin_edge, bool
>>> is_auto_edge,
>>> > -              bool do_mcheck, void *aux)
>>> > +              enum rstp_admin_point_to_point_mac_state
>>> admin_p2p_mac_state,
>>> > +              bool admin_port_state, bool do_mcheck, void *aux)
>>> >     OVS_EXCLUDED(rstp_mutex)
>>> > {
>>> >     ovs_mutex_lock(&rstp_mutex);
>>> > @@ -1299,6 +1326,8 @@ rstp_port_set(struct rstp_port *port, uint16_t
>>> port_num, int priority,
>>> >     rstp_port_set_path_cost__(port, path_cost);
>>> >     rstp_port_set_admin_edge__(port, is_admin_edge);
>>> >     rstp_port_set_auto_edge__(port, is_auto_edge);
>>> > +    rstp_port_set_admin_point_to_point_mac__(port,
>>> admin_p2p_mac_state);
>>> > +    rstp_port_set_administrative_bridge_port__(port,
>>> admin_port_state);
>>> >     rstp_port_set_mcheck__(port, do_mcheck);
>>> >     ovs_mutex_unlock(&rstp_mutex);
>>> > }
>>> > diff --git a/lib/rstp.h b/lib/rstp.h
>>> > index ccf8292..458aecf 100644
>>> > --- a/lib/rstp.h
>>> > +++ b/lib/rstp.h
>>> > @@ -120,6 +120,12 @@ enum rstp_port_role {
>>> >     ROLE_DISABLED
>>> > };
>>> >
>>> > +enum rstp_admin_point_to_point_mac_state {
>>> > +    RSTP_ADMIN_P2P_MAC_FORCE_FALSE,
>>> > +    RSTP_ADMIN_P2P_MAC_FORCE_TRUE,
>>> > +    RSTP_ADMIN_P2P_MAC_AUTO
>>> > +};
>>> > +
>>> > struct rstp;
>>> > struct rstp_port;
>>> > struct ofproto_rstp_settings;
>>> > @@ -211,7 +217,8 @@ uint32_t rstp_convert_speed_to_cost(unsigned int
>>> speed);
>>> >
>>> > void rstp_port_set(struct rstp_port *, uint16_t port_num, int priority,
>>> >                    uint32_t path_cost, bool is_admin_edge, bool
>>> is_auto_edge,
>>> > -                   bool do_mcheck, void *aux)
>>> > +                   enum rstp_admin_point_to_point_mac_state
>>> admin_p2p_mac_state,
>>> > +                   bool admin_port_state, bool do_mcheck, void *aux)
>>> >     OVS_EXCLUDED(rstp_mutex);
>>> >
>>> > enum rstp_state rstp_port_get_state(const struct rstp_port *)
>>> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> > index 2302073..95298f2 100644
>>> > --- a/ofproto/ofproto-dpif.c
>>> > +++ b/ofproto/ofproto-dpif.c
>>> > @@ -2402,7 +2402,9 @@ set_rstp_port(struct ofport *ofport_,
>>> >     }
>>> >
>>> >     rstp_port_set(rp, s->port_num, s->priority, s->path_cost,
>>> > -                  s->admin_edge_port, s->auto_edge, s->mcheck,
>>> ofport);
>>> > +                  s->admin_edge_port, s->auto_edge,
>>> > +                  s->admin_p2p_mac_state, s->admin_port_state,
>>> s->mcheck,
>>> > +                  ofport);
>>> >     update_rstp_port_state(ofport);
>>> >     /* Synchronize operational status. */
>>> >     rstp_port_set_mac_operational(rp, ofport->may_enable);
>>> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>>> > index 989747d..bb28376 100644
>>> > --- a/ofproto/ofproto.h
>>> > +++ b/ofproto/ofproto.h
>>> > @@ -126,6 +126,8 @@ struct ofproto_port_rstp_settings {
>>> >     bool admin_edge_port;
>>> >     bool auto_edge;
>>> >     bool mcheck;
>>> > +    uint8_t admin_p2p_mac_state;
>>> > +    bool admin_port_state;
>>> > };
>>> >
>>> > struct ofproto_stp_settings {
>>> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
>>> > index 8cf13ae..2902a27 100644
>>> > --- a/utilities/ovs-vsctl.8.in
>>> > +++ b/utilities/ovs-vsctl.8.in
>>> > @@ -1073,6 +1073,15 @@ Set the auto edge value of port \fBeth0\fR:
>>> > .IP
>>> > .B "ovs\-vsctl set Port eth0 other_config:rstp-port-auto-edge=true"
>>> > .PP
>>> > +Set the admin point to point mac value  of port \fBeth0\fR.
>>>
>>> Remove extra space after ‘value‘.
>>>
>>> > +Acceptable values are 0 (force false), 1 (force true) or 2 (auto).
>>> > +.IP
>>> > +.B "ovs\-vsctl set Port eth0 other_config:rstp-admin-p2p-mac=1"
>>> > +.PP
>>> > +Set the admin port state value of port \fBeth0\fR.
>>> > +.IP
>>> > +.B "ovs\-vsctl set Port eth0 other_config:rstp-admin-port-state=true"
>>> > +.PP
>>> > Set the mcheck value of port \fBeth0\fR:
>>> > .IP
>>> > .B "ovs\-vsctl set Port eth0 other_config:rstp-port-mcheck=true"
>>> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>>> > index 20cfcba..a5fb361 100644
>>> > --- a/vswitchd/bridge.c
>>> > +++ b/vswitchd/bridge.c
>>> > @@ -1398,6 +1398,16 @@ port_configure_rstp(const struct ofproto
>>> *ofproto, struct port *port,
>>> >         port_s->priority = RSTP_DEFAULT_PORT_PRIORITY;
>>> >     }
>>> >
>>> > +    config_str = smap_get(&port->cfg->other_config,
>>> "rstp-admin-p2p-mac");
>>> > +    if (config_str) {
>>> > +        port_s->admin_p2p_mac_state = strtoul(config_str, NULL, 0);
>>> > +    } else {
>>> > +        port_s->admin_p2p_mac_state =  RSTP_ADMIN_P2P_MAC_FORCE_TRUE;
>>>
>>> Remove extra space after ‘=‘.
>>>
>>> > +    }
>>> > +
>>> > +    port_s->admin_port_state = smap_get_bool(&port->cfg->other_config,
>>> > +                                             "rstp-admin-port-state",
>>> true);
>>> > +
>>> >     port_s->admin_edge_port = smap_get_bool(&port->cfg->other_config,
>>> >                                             "rstp-port-admin-edge",
>>> false);
>>> >     port_s->auto_edge = smap_get_bool(&port->cfg->other_config,
>>> > --
>>> > 1.8.1.2
>>> >
>>>
>>>
>>
>>
> Daniele
>
>
>



More information about the dev mailing list