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

Jarno Rajahalme jrajahalme at nicira.com
Fri Nov 14 22:30:02 UTC 2014


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