[ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

Daniele Venturino venturino.daniele at gmail.com
Tue Sep 9 10:53:56 UTC 2014


>
>
> Thanks for the review!
> It would be nice to have an Acked-by from you to the series. However, I
> plan to squash trivial CodingStyle fixes in before pushing the series to
> master. Also, I’ll add a News item stating that experimental RSTP is added,
> and more compliance and interoperability testing is needed.
> Responses below,
>   Jarno


Ok. I sent my acks.

I did some minor changes from June to August, that you can see here:
https://github.com/M3S/ovs-rstp/compare/b946221...c910081

About the compliance and interoperability testing, I've been working with
an IXIA validation software for a couple of weeks now, and I already have
some patches.
I'm still solving some problems and I expect to have some more fixes. This
should take a couple of weeks to obtain a compliant version, so maybe it
would be better to wait for a compliant version of the protocol before
merging it to the master. Anyway, if you still want to merge it and mark it
as experimental is still fine by me, since I'll be able to rebase my
patches on your version.


On Sep 3, 2014, at 7:44 AM, Daniele Venturino <venturino.daniele at gmail.com>
> wrote:
> I looked and applied the patches. They’re good to me, I just have some
> notes on patch 13/18 and 16/18.
>
>          memcpy(&p->received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
>          rstp->changes = true;
> -        move_rstp(rstp);
> +        move_rstp__(rstp);
>      } else {
> -        VLOG_DBG("%s, port %u: Bad BPDU received", p->rstp->name,
> +        VLOG_DBG("%s, port %u: Bad RSTP BPDU received", p->rstp->name,
>                   p->port_number);
>          p->error_count++;
>      }
> The received BPDU could also be a STP BPDU.
>
> Changed the language here to “Bad STP or RSTP BPDU received."
> +         * reference for that pointer.  This is OK as we never move
> +         * ports from one bridge to another, and holders always
> +         * release their ports before releasing the bridge.  This
> +         * means that there should be not ports at this time. */
> +        ovs_assert(rstp->ports_count == 0);
> Each RSTP port points back
> Thanks.
>
> +    rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
> +    rstp_set_bridge_forward_delay__(rstp,
> RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
> +    rstp_set_bridge_hello_time__(rstp);
> +    rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
> +    rstp_set_bridge_migrate_time__(rstp);
> +    rstp_set_bridge_transmit_hold_count__(rstp,
> +
>  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
> +    rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
> +                            RSTP_BRIDGE_HELLO_TIME,
> +                            RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
>
> These setters are the same in rstp_create() and reinitialize_rstp__(). We
> could define a funcion like rstp_initialize_port_defaults__() for the
> bridge.
>
> I will look into this.
> +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
> +    OVS_REQUIRES(rstp_mutex)
>  {
> -    struct rstp *rstp;
> +    /* XXX: Should we also support setting this to false, i.e., when port
> +     * configuration is changed? */
> +    if (mcheck == true && port->rstp->force_protocol_version >= 2) {
> +        port->mcheck = true;
> 802.1D-2004 standard claims mcheck to be set from management and cleared
> from its procedure.
> *17.19.13 mcheck*
> *A boolean. 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 test whether all STP Bridges
> (17.4) on the attached LAN*
> *have been removed and the Port can continue to transmit RSTP BPDUs.
> Setting mcheck has no effect if*
> *stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP
> Compatibility” mode.*
> However to use it twice, I need to reset it in the database (to make it
> change when i want to invoke its setter), so i use the command with 0, with
> the only purpouse to clear it in the db, no action is needed from rstp.
> Then i can set it again and trigger the procedure.
>
> Removed the comment and added a note to this effect
> to vswitchd/vswitch.xml.
>
>  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
>                  const struct netdev *netdev, const struct cfm *cfm,
> -                const struct bfd *bfd, int stp_port_no, int rstp_port_no,
> +                const struct bfd *bfd, int stp_port_no,
> +                const struct rstp_port* rstp_port,
>                  enum ofputil_port_config config, enum ofputil_port_state
> state,
>                  bool is_tunnel, bool may_enable)
>  {
>      xport->config = config;
>      xport->state = state;
>      xport->stp_port_no = stp_port_no;
> -    xport->rstp_port_no = rstp_port_no;
> I get a segfault when removing a port from a bridge. I don't if I add here
> this line:
> xport->rstp_port = rstp_port;
>
> I don’t seem to be able to reproduce the segfault. I tried this by adding
> ovs-vsctl del-br and del-port commands to the new test cases, and they
> succeed just fine.
> The code right below will set the rstp_port member, if the new value is
> different from the old value. So all the line you added above does is
> circumvent the unref of the old pointer, and the ref of the new pointer. I
> see that I missed the unref in xlate_xport_remove():
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index dd9536c..425b171 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct
> xport *xport)
>      hmap_remove(&xport->xbridge->xports, &xport->ofp_node);
>
>      netdev_close(xport->netdev);
> +    rstp_port_unref(xport->rstp_port);
>      cfm_unref(xport->cfm);
>      bfd_unref(xport->bfd);
>      free(xport);
> Could you check if this resolves the segfault you get?


It appears to be solved.


> @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void
> *bpdu, size_t bpdu_size)
>
        /* Each RSTP port poits back to struct rstp without holding a
>
 +    rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
>
+static void
>
static void
>


     xport->may_enable = may_enable;
     xport->odp_port = odp_port;
+    if (xport->rstp_port != rstp_port) {
+        rstp_port_unref(xport->rstp_port);
+        xport->rstp_port = rstp_port_ref(rstp_port);
+    }
@@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
     if (ofport->may_enable != enable) {
         struct ofproto_dpif *ofproto =
ofproto_dpif_cast(ofport->up.ofproto);
-        ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
-    }
-    ofport->may_enable = enable;
+        ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
-    if (ofport->rstp_port) {
-        if (rstp_port_get_mac_operational(ofport->rstp_port) != enable) {
+        if (ofport->rstp_port) {
             rstp_port_set_mac_operational(ofport->rstp_port, enable);
         }
     }
+
+    ofport->may_enable = enable;
 }
rstp_port_set_mac_operational(ofport->rstp_port, enable) should be outside
 if (ofport->may_enable != enable) otherwise ports remain disabled when
added.
I decided to initialize the of port->may_enable to false instead.

     xport->is_tunnel = is_tunnel;
>

I applied this and have the impression that this is ok if ports are added
to a bridge after rstp is turned on. Otherwise, if a bridge already has
some ports and rstp is enabled, ports remain disabled.


In patch 16/18:
> That condition is checked as the last alternative in the conditional
> expression above. I did not include a separate log message for that case,
> though.
> I found the original conditional expression confusing, at the minimum it
> has some dead logic, i.e.:
> If:
>
> then neither of the preceding “or" cases can be true and can be eliminated.
> Also, “superior_same_des” still returns SUPERIOR, and looking at the
> comment above the function, it seems to me that SAME is a subset of
> SUPERIOR (see also better_or_same_info()) :
> /* [17.6]
>  * This message priority vector is superior to the port priority vector and
>  * will replace it if, and only if, the message priority vector is better
>  * than the port priority vector, or the message has been transmitted from
> the
>  * same Designated Bridge and Designated Port as the port priority vector,
>  * i.e.,if the following is true:
>  *    ((RD  < RootBridgeID)) ||
>  *    ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
>  *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>  *         (D < designated_bridge_id)) ||
>  *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>  *         (D == designated_bridge_id) && (PD < designated_port_id)) ||
>  *    ((D  == designated_bridge_id.BridgeAddress) &&
>  *         (PD == designated_port_id.PortNumber))
>  */
> (By the "[17.6]" I believe this comment is a quotation from the
> standard(?))
> Note that the main conditional expression in my proposed change was
> exactly this, so it should be trivial to verify it for correctness. As SAME
> is a subset of the above, I carved out the SAME prior to this.  That leaves
> everything not matching this to be INFERIOR. However, maybe this is even
> clearer:
> /* compare_rstp_priority_vector() compares two struct rstp_priority_vectors
>  * and returns a value indicating if the first rstp_priority_vector is
>  * superior, same or inferior to the second one.
>  *
>  * Zero return value indicates INFERIOR, a non-zero return value indicates
>  * SUPERIOR.  When it makes a difference the non-zero return value SAME
>  * indicates the priority vectors are identical (a subset of SUPERIOR).
>  */
> static enum vector_comparison
> compare_rstp_priority_vectors(const struct rstp_priority_vector *v1,
>                               const struct rstp_priority_vector *v2)
> {
>     VLOG_DBG("v1: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
>              RSTP_ID_ARGS(v1->root_bridge_id), v1->root_path_cost,
>              RSTP_ID_ARGS(v1->designated_bridge_id),
> v1->designated_port_id);
>     VLOG_DBG("v2: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
>              RSTP_ID_ARGS(v2->root_bridge_id), v2->root_path_cost,
>              RSTP_ID_ARGS(v2->designated_bridge_id),
> v2->designated_port_id);
>     /* [17.6]
>      * This message priority vector is superior to the port priority
> vector and
>      * will replace it if, and only if, the message priority vector is
> better
>      * than the port priority vector, or the message has been transmitted
> from
>      * the same Designated Bridge and Designated Port as the port priority
>      * vector, i.e., if the following is true:
>      *
>      *    ((RD  < RootBridgeID)) ||
>      *    ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
>      *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>      *         (D < designated_bridge_id)) ||
>      *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>      *         (D == designated_bridge_id) && (PD < designated_port_id)) ||
>      *    ((D  == designated_bridge_id.BridgeAddress) &&
>      *         (PD == designated_port_id.PortNumber))
>      */
>     if (v1->root_bridge_id < v2->root_bridge_id        ||
> (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost < v2->root_path_cost)
>         || (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id < v2->designated_bridge_id)
>         || (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id == v2->designated_bridge_id
>             && v1->designated_port_id < v2->designated_port_id)
>         || (v1->designated_bridge_id == v2->designated_bridge_id
>   && v1->designated_port_id == v2->designated_port_id)) {
>         /* SAME is a subset of SUPERIOR. */
>         if (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id == v2->designated_bridge_id
>             && v1->designated_port_id == v2->designated_port_id) {
>             VLOG_DBG("superior_same");
>             return SAME;
>         }
>         VLOG_DBG("superior");
>         return SUPERIOR;
>     }
>     VLOG_DBG("inferior");
>     return INFERIOR;
> }
> Note, however that this should still be same logic as the original, just
> clearer and closer to the one in the standard.
> 802.1D-2004 is complex, and entails some keywords like "SUPERIOR" that are
> used in misleading way. I would like to mantain
> compare_rstp_priority_vector() as it is, since it is IMHO simpler to
> compare it with the standard for correctness.

diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>
 v1->designated_bridge_id == v2->designated_bridge_id
>
&& v1->designated_port_id == v2->designated_port_id


Ok, sorry about that. I'll test this anyway.
I'll send a patch adding a discrimination on the bridge_port_id in case two
vectors are the same.


Also in lib/rstp-state-machines.c, it's probably too drastic to call
> OVS_NOT_REACHED() when a BPDU is going to be transmitted on a port with
> role disabled.
> index 2a98f62..def9bdc 100644
> --- a/lib/rstp-state-machines.c
> +++ b/lib/rstp-state-machines.c
> @@ -848,7 +848,7 @@ tx_rstp(struct rstp_port *p)
>          /* should not happen! */
>          VLOG_ERR("%s transmitting bpdu in disabled role on port "
>                   ""RSTP_PORT_ID_FMT"", p->rstp->name, p->port_id);
> -        OVS_NOT_REACHED();
>          break;
>      }
>
> Fixed.

diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>


Daniele

2014-09-08 23:47 GMT+02:00 Jarno Rajahalme <jrajahalme at nicira.com>:

> Thanks for the review!
>
> It would be nice to have an Acked-by from you to the series. However, I
> plan to squash trivial CodingStyle fixes in before pushing the series to
> master. Also, I’ll add a News item stating that experimental RSTP is added,
> and more compliance and interoperability testing is needed.
>
> Responses below,
>
>   Jarno
>
> On Sep 3, 2014, at 7:44 AM, Daniele Venturino <venturino.daniele at gmail.com>
> wrote:
>
> I looked and applied the patches. They’re good to me, I just have some
> notes on patch 13/18 and 16/18.
>
>
> @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void
>> *bpdu, size_t bpdu_size)
>>          memcpy(&p->received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
>>          rstp->changes = true;
>> -        move_rstp(rstp);
>> +        move_rstp__(rstp);
>>      } else {
>> -        VLOG_DBG("%s, port %u: Bad BPDU received", p->rstp->name,
>> +        VLOG_DBG("%s, port %u: Bad RSTP BPDU received", p->rstp->name,
>>                   p->port_number);
>>          p->error_count++;
>>      }
>
>
> The received BPDU could also be a STP BPDU.
>
>
> Changed the language here to “Bad STP or RSTP BPDU received."
>
>         /* Each RSTP port poits back to struct rstp without holding a
>> +         * reference for that pointer.  This is OK as we never move
>> +         * ports from one bridge to another, and holders always
>> +         * release their ports before releasing the bridge.  This
>> +         * means that there should be not ports at this time. */
>> +        ovs_assert(rstp->ports_count == 0);
>
>
> Each RSTP port points back
>
>
> Thanks.
>
>
>  +    rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
>> +    rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
>> +    rstp_set_bridge_forward_delay__(rstp,
>> RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
>> +    rstp_set_bridge_hello_time__(rstp);
>> +    rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
>> +    rstp_set_bridge_migrate_time__(rstp);
>> +    rstp_set_bridge_transmit_hold_count__(rstp,
>> +
>>  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
>> +    rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
>> +                            RSTP_BRIDGE_HELLO_TIME,
>> +                            RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
>>
>>
> These setters are the same in rstp_create() and reinitialize_rstp__(). We
> could define a funcion like rstp_initialize_port_defaults__() for the
> bridge.
>
>
>
> I will look into this.
>
> +static void
>> +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
>> +    OVS_REQUIRES(rstp_mutex)
>>  {
>> -    struct rstp *rstp;
>> +    /* XXX: Should we also support setting this to false, i.e., when port
>> +     * configuration is changed? */
>> +    if (mcheck == true && port->rstp->force_protocol_version >= 2) {
>> +        port->mcheck = true;
>
>
> 802.1D-2004 standard claims mcheck to be set from management and cleared
> from its procedure.
>
> *17.19.13 mcheck*
> *A boolean. 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 test whether all STP Bridges
> (17.4) on the attached LAN*
> *have been removed and the Port can continue to transmit RSTP BPDUs.
> Setting mcheck has no effect if*
> *stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP
> Compatibility” mode.*
>
> However to use it twice, I need to reset it in the database (to make it
> change when i want to invoke its setter), so i use the command with 0, with
> the only purpouse to clear it in the db, no action is needed from rstp.
> Then i can set it again and trigger the procedure.
>
>
> Removed the comment and added a note to this effect
> to vswitchd/vswitch.xml.
>
>
> static void
>>  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
>>                  const struct netdev *netdev, const struct cfm *cfm,
>> -                const struct bfd *bfd, int stp_port_no, int rstp_port_no,
>> +                const struct bfd *bfd, int stp_port_no,
>> +                const struct rstp_port* rstp_port,
>>                  enum ofputil_port_config config, enum ofputil_port_state
>> state,
>>                  bool is_tunnel, bool may_enable)
>>  {
>>      xport->config = config;
>>      xport->state = state;
>>      xport->stp_port_no = stp_port_no;
>> -    xport->rstp_port_no = rstp_port_no;
>
>
> I get a segfault when removing a port from a bridge. I don't if I add here
> this line:
> xport->rstp_port = rstp_port;
>
>
> I don’t seem to be able to reproduce the segfault. I tried this by adding
> ovs-vsctl del-br and del-port commands to the new test cases, and they
> succeed just fine.
>
> The code right below will set the rstp_port member, if the new value is
> different from the old value. So all the line you added above does is
> circumvent the unref of the old pointer, and the ref of the new pointer. I
> see that I missed the unref in xlate_xport_remove():
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index dd9536c..425b171 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct
> xport *xport)
>      hmap_remove(&xport->xbridge->xports, &xport->ofp_node);
>
>      netdev_close(xport->netdev);
> +    rstp_port_unref(xport->rstp_port);
>      cfm_unref(xport->cfm);
>      bfd_unref(xport->bfd);
>      free(xport);
>
> Could you check if this resolves the segfault you get?
>
>
>      xport->is_tunnel = is_tunnel;
>>      xport->may_enable = may_enable;
>>      xport->odp_port = odp_port;
>> +    if (xport->rstp_port != rstp_port) {
>> +        rstp_port_unref(xport->rstp_port);
>> +        xport->rstp_port = rstp_port_ref(rstp_port);
>> +    }
>> @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
>>      if (ofport->may_enable != enable) {
>>          struct ofproto_dpif *ofproto =
>> ofproto_dpif_cast(ofport->up.ofproto);
>> -        ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
>> -    }
>> -    ofport->may_enable = enable;
>> +        ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
>> -    if (ofport->rstp_port) {
>> -        if (rstp_port_get_mac_operational(ofport->rstp_port) != enable) {
>> +        if (ofport->rstp_port) {
>>              rstp_port_set_mac_operational(ofport->rstp_port, enable);
>>          }
>>      }
>> +
>> +    ofport->may_enable = enable;
>>  }
>
>
> rstp_port_set_mac_operational(ofport->rstp_port, enable) should be outside
>  if (ofport->may_enable != enable) otherwise ports remain disabled when
> added.
>
>
> I decided to initialize the of port->may_enable to false instead.
>
>
>
> In patch 16/18:
>
> diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>> index e8b8438..5ae7124 100644
>> --- a/lib/rstp-state-machines.c
>> +++ b/lib/rstp-state-machines.c
>> @@ -2015,42 +2015,33 @@  compare_rstp_priority_vector(struct
>> rstp_priority_vector *v1,
>>               RSTP_ID_ARGS(v2->root_bridge_id), v2->root_path_cost,
>>               RSTP_ID_ARGS(v2->designated_bridge_id),
>> v2->designated_port_id);
>>
>> -    if (v1->root_bridge_id < v2->root_bridge_id
>> -        || (v1->root_bridge_id == v2->root_bridge_id &&
>> -            v1->root_path_cost < v2->root_path_cost)
>> -        || (v1->root_bridge_id == v2->root_bridge_id &&
>> -            v1->root_path_cost == v2->root_path_cost &&
>> -            v1->designated_bridge_id < v2->designated_bridge_id)
>> -        || (v1->root_bridge_id == v2->root_bridge_id &&
>> -            v1->root_path_cost == v2->root_path_cost &&
>> -            v1->designated_bridge_id == v2->designated_bridge_id &&
>> -            v1->designated_port_id < v2->designated_port_id)) {
>> -        VLOG_DBG("superior_absolute");
>> -        return SUPERIOR;
>> -    } else if ((v1->root_bridge_id > v2->root_bridge_id
>> -                || (v1->root_bridge_id == v2->root_bridge_id &&
>> -                    v1->root_path_cost > v2->root_path_cost)
>> -                || (v1->root_bridge_id == v2->root_bridge_id &&
>> -                    v1->root_path_cost == v2->root_path_cost &&
>> -                    v1->designated_bridge_id > v2->designated_bridge_id)
>> -                || (v1->root_bridge_id == v2->root_bridge_id &&
>> -                    v1->root_path_cost == v2->root_path_cost &&
>> -                    v1->designated_bridge_id == v2->designated_bridge_id
>> &&
>> -                    v1->designated_port_id > v2->designated_port_id))
>> -               && v1->designated_bridge_id == v2->designated_bridge_id
>> -               && v1->designated_port_id == v2->designated_port_id) {
>> -        VLOG_DBG("superior_same_des");
>> -        return SUPERIOR;
>> -    } else if (v1->root_bridge_id == v2->root_bridge_id &&
>> -               v1->root_path_cost == v2->root_path_cost &&
>> -               v1->designated_bridge_id == v2->designated_bridge_id &&
>> -               v1->designated_port_id == v2->designated_port_id) {
>> +    /* Testing for SAME first, so the SUPERIOR test can follow the logic
>> above
>> +     * as is. */
>> +    if (v1->root_bridge_id == v2->root_bridge_id
>> +        && v1->root_path_cost == v2->root_path_cost
>> +        && v1->designated_bridge_id == v2->designated_bridge_id
>> +        && v1->designated_port_id == v2->designated_port_id) {
>>          VLOG_DBG("same");
>>          return SAME;
>> -    } else {
>> -        VLOG_DBG("inferior");
>> -        return INFERIOR;
>>      }
>> +    if (v1->root_bridge_id < v2->root_bridge_id
>> +        || (v1->root_bridge_id == v2->root_bridge_id
>> +            && v1->root_path_cost < v2->root_path_cost)
>> +        || (v1->root_bridge_id == v2->root_bridge_id
>> +            && v1->root_path_cost == v2->root_path_cost
>> +            && v1->designated_bridge_id < v2->designated_bridge_id)
>> +        || (v1->root_bridge_id == v2->root_bridge_id
>> +            && v1->root_path_cost == v2->root_path_cost
>> +            && v1->designated_bridge_id == v2->designated_bridge_id
>> +            && v1->designated_port_id < v2->designated_port_id)
>> +        || (v1->designated_bridge_id == v2->designated_bridge_id
>> +            && v1->designated_port_id == v2->designated_port_id)) {
>> +        VLOG_DBG("superior");
>> +        return SUPERIOR;
>> +    }
>> +
>> +    VLOG_DBG("inferior");
>> +    return INFERIOR;
>>  }
>
>
>
> I think this is not correct, since the condition returning
> "superior_same_des" is no longer checked.
>
> That condition is checked as the last alternative in the conditional
> expression above. I did not include a separate log message for that case,
> though.
>
> I found the original conditional expression confusing, at the minimum it
> has some dead logic, i.e.:
>
> If:
>
>  v1->designated_bridge_id == v2->designated_bridge_id
>>
> && v1->designated_port_id == v2->designated_port_id
>
>
> then neither of the preceding “or" cases can be true and can be eliminated.
>
> Also, “superior_same_des” still returns SUPERIOR, and looking at the
> comment above the function, it seems to me that SAME is a subset of
> SUPERIOR (see also better_or_same_info()) :
>
> /* [17.6]
>  * This message priority vector is superior to the port priority vector and
>  * will replace it if, and only if, the message priority vector is better
>  * than the port priority vector, or the message has been transmitted from
> the
>  * same Designated Bridge and Designated Port as the port priority vector,
>  * i.e.,if the following is true:
>  *    ((RD  < RootBridgeID)) ||
>  *    ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
>  *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>  *         (D < designated_bridge_id)) ||
>  *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>  *         (D == designated_bridge_id) && (PD < designated_port_id)) ||
>  *    ((D  == designated_bridge_id.BridgeAddress) &&
>  *         (PD == designated_port_id.PortNumber))
>  */
>
> (By the "[17.6]" I believe this comment is a quotation from the
> standard(?))
>
> Note that the main conditional expression in my proposed change was
> exactly this, so it should be trivial to verify it for correctness. As SAME
> is a subset of the above, I carved out the SAME prior to this.  That leaves
> everything not matching this to be INFERIOR. However, maybe this is even
> clearer:
>
> /* compare_rstp_priority_vector() compares two struct rstp_priority_vectors
>  * and returns a value indicating if the first rstp_priority_vector is
>  * superior, same or inferior to the second one.
>  *
>  * Zero return value indicates INFERIOR, a non-zero return value indicates
>  * SUPERIOR.  When it makes a difference the non-zero return value SAME
>  * indicates the priority vectors are identical (a subset of SUPERIOR).
>  */
> static enum vector_comparison
> compare_rstp_priority_vectors(const struct rstp_priority_vector *v1,
>                               const struct rstp_priority_vector *v2)
> {
>     VLOG_DBG("v1: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
>              RSTP_ID_ARGS(v1->root_bridge_id), v1->root_path_cost,
>              RSTP_ID_ARGS(v1->designated_bridge_id),
> v1->designated_port_id);
>     VLOG_DBG("v2: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
>              RSTP_ID_ARGS(v2->root_bridge_id), v2->root_path_cost,
>              RSTP_ID_ARGS(v2->designated_bridge_id),
> v2->designated_port_id);
>
>     /* [17.6]
>      * This message priority vector is superior to the port priority
> vector and
>      * will replace it if, and only if, the message priority vector is
> better
>      * than the port priority vector, or the message has been transmitted
> from
>      * the same Designated Bridge and Designated Port as the port priority
>      * vector, i.e., if the following is true:
>      *
>      *    ((RD  < RootBridgeID)) ||
>      *    ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
>      *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>      *         (D < designated_bridge_id)) ||
>      *    ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
>      *         (D == designated_bridge_id) && (PD < designated_port_id)) ||
>      *    ((D  == designated_bridge_id.BridgeAddress) &&
>      *         (PD == designated_port_id.PortNumber))
>      */
>     if (v1->root_bridge_id < v2->root_bridge_id
>         || (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost < v2->root_path_cost)
>         || (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id < v2->designated_bridge_id)
>         || (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id == v2->designated_bridge_id
>             && v1->designated_port_id < v2->designated_port_id)
>         || (v1->designated_bridge_id == v2->designated_bridge_id
>             && v1->designated_port_id == v2->designated_port_id)) {
>         /* SAME is a subset of SUPERIOR. */
>         if (v1->root_bridge_id == v2->root_bridge_id
>             && v1->root_path_cost == v2->root_path_cost
>             && v1->designated_bridge_id == v2->designated_bridge_id
>             && v1->designated_port_id == v2->designated_port_id) {
>             VLOG_DBG("superior_same");
>             return SAME;
>         }
>         VLOG_DBG("superior");
>         return SUPERIOR;
>     }
>
>     VLOG_DBG("inferior");
>     return INFERIOR;
> }
>
> Note, however that this should still be same logic as the original, just
> clearer and closer to the one in the standard.
>
> 802.1D-2004 is complex, and entails some keywords like "SUPERIOR" that are
> used in misleading way. I would like to mantain
> compare_rstp_priority_vector() as it is, since it is IMHO simpler to
> compare it with the standard for correctness.
>
>
> Also in lib/rstp-state-machines.c, it's probably too drastic to call
> OVS_NOT_REACHED() when a BPDU is going to be transmitted on a port with
> role disabled.
>
> diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>> index 2a98f62..def9bdc 100644
>> --- a/lib/rstp-state-machines.c
>> +++ b/lib/rstp-state-machines.c
>> @@ -848,7 +848,7 @@ tx_rstp(struct rstp_port *p)
>>          /* should not happen! */
>>          VLOG_ERR("%s transmitting bpdu in disabled role on port "
>>                   ""RSTP_PORT_ID_FMT"", p->rstp->name, p->port_id);
>> -        OVS_NOT_REACHED();
>>          break;
>>      }
>
>
>
> Fixed.
>
>
> Daniele
>
>
>
> 2014-08-21 1:57 GMT+02:00 Jarno Rajahalme <jrajahalme at nicira.com>:
>
>> I took my time starting the review, so I decided address issues as I
>> see them rather than just comment on them.
>>
>> The first patch of this series is a minimally rebased version of the
>> v5 sent on ovs-dev on June 12th, 2014.  Rest of the series is my
>> proposal for fixes and enhancements.
>>
>> I could have reordered and squashed some of the patches together, but
>> that would have been more work...
>>
>> Daniele Venturino (1):
>>   Rapid Spanning Tree Protocol (IEEE 802.1D).
>>
>> Jarno Rajahalme (17):
>>   lib/stp,rstp: Add unit more unit tests.
>>   lib/stp: Some debugging support.
>>   lib/rstp: Better debug messages, style fixes.
>>   vswitch.xml: Fix RSTP configuration documentation.
>>   lib/rstp: Remove unused struct rstp_priority_vector4
>>   lib/rstp: Coding style fixes.
>>   lib/rstp: Refactor priority vector recalculation.
>>   lib/rstp: Refactor port number allocation.
>>   lib/rstp: Refactor port initialization.
>>   lib/rstp: CodingStyle changes.
>>   lib/rstp: Inline trivial predicate functions.
>>   lib/rstp: More robust thread safety.
>>   lib/rstp: Remove lock recursion.
>>   lib/rstp: CodingStyle fixes.
>>   lib/rstp: Simplify priority vector comparison.
>>   lib/rstp: Eliminate ports_count.
>>   lib/rstp: Use hmap instead of a list for ports.
>>
>>  AUTHORS                      |    1 +
>>  NOTICE                       |    3 +
>>  lib/automake.mk              |    5 +
>>  lib/packets.h                |    5 +
>>  lib/rstp-common.h            |  879 ++++++++++++++++++
>>  lib/rstp-state-machines.c    | 2025
>> ++++++++++++++++++++++++++++++++++++++++++
>>  lib/rstp-state-machines.h    |   46 +
>>  lib/rstp.c                   | 1369 ++++++++++++++++++++++++++++
>>  lib/rstp.h                   |  290 ++++++
>>  lib/stp.c                    |    7 +-
>>  lib/stp.h                    |    5 -
>>  ofproto/ofproto-dpif-xlate.c |  157 +++-
>>  ofproto/ofproto-dpif-xlate.h |    6 +-
>>  ofproto/ofproto-dpif.c       |  255 +++++-
>>  ofproto/ofproto-provider.h   |   47 +
>>  ofproto/ofproto.c            |   84 ++
>>  ofproto/ofproto.h            |   50 ++
>>  tests/.gitignore             |    1 +
>>  tests/automake.mk            |    3 +
>>  tests/ovs-vsctl.at           |    4 +
>>  tests/rstp.at                |  235 +++++
>>  tests/stp.at                 |  100 +++
>>  tests/test-rstp.c            |  714 +++++++++++++++
>>  tests/testsuite.at           |    1 +
>>  utilities/ovs-vsctl.8.in     |   79 ++
>>  vswitchd/bridge.c            |  295 ++++++
>>  vswitchd/vswitch.ovsschema   |   15 +-
>>  vswitchd/vswitch.xml         |  131 ++-
>>  28 files changed, 6749 insertions(+), 63 deletions(-)
>>  create mode 100644 lib/rstp-common.h
>>  create mode 100644 lib/rstp-state-machines.c
>>  create mode 100644 lib/rstp-state-machines.h
>>  create mode 100644 lib/rstp.c
>>  create mode 100644 lib/rstp.h
>>  create mode 100644 tests/rstp.at
>>  create mode 100644 tests/test-rstp.c
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
>



More information about the dev mailing list