| Message ID | 1491016286-86218-2-git-send-email-nic@opencloud.tech |
|---|---|
| State | Changes Requested |
| Headers | show |
> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech> wrote: > > When a bridge stp enabled, we assign sequentially element > of stp_port array (in stp struct) to bridge ports. That is > ok when no ports are added to bridge. When adding a port > to bridge which stp enabled, the ovs-vswitchd will assign > stp_port sequentially again. Then the stp_port belonging > to one port may belong to other one. Could you elaborate the problem with this STP port renumbering? > This patch uses the > OpenFlow port numbers instead of sequence numbers to avoid > it. > Using OpenFlow port numbers (32-bit) as STP port numbers (8-bit) seems wrong to me. Besides, you can always use the other-config stp-port-num in ovsdb to specify which STP port number you want, if you do not want them to be numbered automatically. > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > --- > tests/stp.at | 90 ++++++++++++++++++++++++++++++++++++++++++------------- > vswitchd/bridge.c | 10 +++++-- > 2 files changed, 77 insertions(+), 23 deletions(-) > > diff --git a/tests/stp.at b/tests/stp.at > index bd5d208..98632a8 100644 > --- a/tests/stp.at > +++ b/tests/stp.at > @@ -570,11 +570,6 @@ for i in $(seq 0 35); do > done > > # root bridge sends query packet > -# we don't want to lose that message, so send it twice > -AT_CHECK([ovs-appctl netdev-dummy/receive br0 \ > - '01005E010101000C29A027D18100000108004500001C000100004002CBCBAC102201E00101011114EEEB00000000']) > - > -ovs-appctl time/warp 1000 > AT_CHECK([ovs-appctl netdev-dummy/receive br0 \ > '01005E010101000C29A027D18100000108004500001C000100004002CBCBAC102201E00101011114EEEB00000000']) > > @@ -589,21 +584,14 @@ OVS_WAIT_UNTIL([ovs-appctl mdb/show br2 | grep 'querier']) > # del p2 on the br0, the topology will be changed > AT_CHECK([ovs-vsctl del-port br0 p2]) > > -# give time for STP to synchronize > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > - > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > - > -ovs-appctl time/warp 3000 > -ovs-appctl time/warp 3000 > +# We give time for STP to synchronize. The message age of > +# p6 port will time out after 20s. The p5 will left blocked > +# state to forwarding after 30s and then br2 bridge will > +# detect the topology change, flush the fdb and sent tcn BPDU. > +# br1 and br0 will also flush fdb when receiving tcn BPDU. > +for i in $(seq 0 52); do > + ovs-appctl time/warp 1000 > +done > > # check fdb and mdb > AT_CHECK([ovs-appctl fdb/show br0], [0], [dnl > @@ -626,5 +614,67 @@ AT_CHECK([ovs-appctl mdb/show br2], [0], [dnl > port VLAN GROUP Age > ]) > > +AT_CLEANUP > + > +AT_SETUP([STP - check the stp ports num]) > +OVS_VSWITCHD_START([]) > + > +AT_CHECK([ > + ovs-vsctl -- \ > + set port br0 other_config:stp-enable=false -- \ > + set bridge br0 datapath-type=dummy stp_enable=true \ > + other-config:hwaddr=aa:66:aa:66:00:00 > +], [0]) > + > +AT_CHECK([ > + ovs-vsctl add-port br0 p1 -- \ > + set interface p1 type=dummy ofport_request=1 > + ovs-vsctl add-port br0 p2 -- \ > + set interface p2 type=dummy ofport_request=2 > +], [0]) > + > +ovs-appctl time/stop > + > +# give time for STP to move initially > +for i in $(seq 0 30); do > + ovs-appctl time/warp 1000 > +done > + > +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > + p1 designated forwarding 19 128.1 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > + > +# add a stp port > +AT_CHECK([ > + ovs-vsctl add-port br0 p3 -- \ > + set interface p3 type=dummy ofport_request=3 > +], [0]) > + > +# The new stp port should be a listening state and other > +# stp ports keep forwarding. > +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > + p1 designated forwarding 19 128.1 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl > + p3 designated listening 19 128.3 > +]) > + > +# delete p1 stp port > +AT_CHECK([ovs-vsctl del-port br0 p1]) > + > +# The other stp ports keep original state. > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl > + p3 designated listening 19 128.3 > +]) > + > OVS_VSWITCHD_STOP > AT_CLEANUP > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 867a26d..d683000 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -1396,13 +1396,17 @@ port_configure_stp(const struct ofproto *ofproto, struct port *port, > bitmap_set1(port_num_bitmap, port_idx); > port_s->port_num = port_idx; > } else { > - if (*port_num_counter >= STP_MAX_PORTS) { > - VLOG_ERR("port %s: too many STP ports, disabling", port->name); > + if (iface->ofp_port > STP_MAX_PORTS) { > + VLOG_ERR("port %s: too many STP ports or OpenFlow port number " > + "(%d) > STP_MAX_PORTS (%d), disabling", port->name, > + iface->ofp_port, STP_MAX_PORTS); > + > port_s->enable = false; > return; > } > > - port_s->port_num = (*port_num_counter)++; > + port_s->port_num = iface->ofp_port -1; > + (*port_num_counter)++; > } > > config_str = smap_get(&port->cfg->other_config, "stp-path-cost"); > -- > 1.8.3.1 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Apr 21, 2017, at 8:34 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > >> >> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> wrote: >> >> When a bridge stp enabled, we assign sequentially element >> of stp_port array (in stp struct) to bridge ports. That is >> ok when no ports are added to bridge. When adding a port >> to bridge which stp enabled, the ovs-vswitchd will assign >> stp_port sequentially again. Then the stp_port belonging >> to one port may belong to other one. > > Could you elaborate the problem with this STP port renumbering? When we add a port to bridge which stp is enabled, the stp ports are renumbered. For example: p0 is mapped to one array element of struct stp_port ports[STP_MAX_PORTS] (e.g. ports[0]) p1 is mapped to one array element of struct stp_port ports[STP_MAX_PORTS] (e.g. ports[1]) When we add a port p2 to bridge, the p2 may be mapped to ports[0], p0 is mapped to ports[1], and p1 is mapped to ports[2]. But we hope that p2 may be mapped to ports[2], p0 is mapped to ports[0], and p1 is mapped to ports[1]. If not, the stp ports of bridge will converge. As a general rule, the state of new port should be changed (e.g goto forwarding state or blocking.) and other stp ports remain the same. Test shell: #!/bin/bash ovs-vsctl add-br br0 ovs-vsctl add-br br1 ovs-appctl vlog/set ofproto_dpif:dbg ovs-vsctl add-port br0 p1 -- \ set interface p1 type=dummy options:pstream=punix:/tmp/p1.sock ofport_request=1 ovs-vsctl add-port br0 p2 -- \ set interface p2 type=dummy options:pstream=punix:/tmp/p2.sock ofport_request=2 ovs-vsctl add-port br1 p6 -- \ set interface p6 type=dummy options:stream=unix:/tmp/p1.sock ofport_request=6 ovs-vsctl add-port br1 p7 -- \ set interface p7 type=dummy options:stream=unix:/tmp/p2.sock ofport_request=7 ovs-ofctl add-flow br0 action=normal ovs-ofctl add-flow br1 action=normal ovs-appctl netdev-dummy/set-admin-state up ovs-vsctl set port br0 other_config:stp-enable=false -- \ set bridge br0 stp_enable=true other-config:hwaddr=aa:66:aa:66:00:00 ovs-vsctl set port br1 other_config:stp-enable=false -- \ set bridge br1 stp_enable=true other-config:hwaddr=aa:66:aa:66:00:01 # ovs-appctl stp/show <---cut---> Bridge ID: stp-priority 32768 stp-system-id aa:66:aa:66:00:01 stp-hello-time 2s stp-max-age 20s stp-fwd-delay 15s Interface Role State Cost Pri.Nbr ---------- ---------- ---------- ----- ------- p6 alternate blocking 19 128.1 p7 root forwarding 19 128.2 # ovs-vsctl add-port br1 p8 # ovs-appctl stp/show <---cut---> Bridge ID: stp-priority 32768 stp-system-id aa:66:aa:66:00:01 stp-hello-time 2s stp-max-age 20s stp-fwd-delay 15s Interface Role State Cost Pri.Nbr ---------- ---------- ---------- ----- ------- p8 alternate blocking 19 128.1 p6 root forwarding 19 128.2 p7 designated listening 19 128.3 After adding the p8 port, p8 uses the p6 state, p6 uses the p7 state, and p7 uses the new state. The rstp works well without elaboration. >> This patch uses the >> OpenFlow port numbers instead of sequence numbers to avoid >> it. >> > > Using OpenFlow port numbers (32-bit) as STP port numbers (8-bit) seems wrong to me. Besides, you can always use the other-config stp-port-num in ovsdb to specify which STP port number you want, if you do not want them to be numbered automatically. Yes, the max number of stp ports on bridge is STP_MAX_PORTS(255), and this should not be problem. Assigning the openflow port num to stp port is simple. But we can use other way to assign stp port number. If you have any idea, please let me know.
diff --git a/tests/stp.at b/tests/stp.at index bd5d208..98632a8 100644 --- a/tests/stp.at +++ b/tests/stp.at @@ -570,11 +570,6 @@ for i in $(seq 0 35); do done # root bridge sends query packet -# we don't want to lose that message, so send it twice -AT_CHECK([ovs-appctl netdev-dummy/receive br0 \ - '01005E010101000C29A027D18100000108004500001C000100004002CBCBAC102201E00101011114EEEB00000000']) - -ovs-appctl time/warp 1000 AT_CHECK([ovs-appctl netdev-dummy/receive br0 \ '01005E010101000C29A027D18100000108004500001C000100004002CBCBAC102201E00101011114EEEB00000000']) @@ -589,21 +584,14 @@ OVS_WAIT_UNTIL([ovs-appctl mdb/show br2 | grep 'querier']) # del p2 on the br0, the topology will be changed AT_CHECK([ovs-vsctl del-port br0 p2]) -# give time for STP to synchronize -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 - -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 - -ovs-appctl time/warp 3000 -ovs-appctl time/warp 3000 +# We give time for STP to synchronize. The message age of +# p6 port will time out after 20s. The p5 will left blocked +# state to forwarding after 30s and then br2 bridge will +# detect the topology change, flush the fdb and sent tcn BPDU. +# br1 and br0 will also flush fdb when receiving tcn BPDU. +for i in $(seq 0 52); do + ovs-appctl time/warp 1000 +done # check fdb and mdb AT_CHECK([ovs-appctl fdb/show br0], [0], [dnl @@ -626,5 +614,67 @@ AT_CHECK([ovs-appctl mdb/show br2], [0], [dnl port VLAN GROUP Age ]) +AT_CLEANUP + +AT_SETUP([STP - check the stp ports num]) +OVS_VSWITCHD_START([]) + +AT_CHECK([ + ovs-vsctl -- \ + set port br0 other_config:stp-enable=false -- \ + set bridge br0 datapath-type=dummy stp_enable=true \ + other-config:hwaddr=aa:66:aa:66:00:00 +], [0]) + +AT_CHECK([ + ovs-vsctl add-port br0 p1 -- \ + set interface p1 type=dummy ofport_request=1 + ovs-vsctl add-port br0 p2 -- \ + set interface p2 type=dummy ofport_request=2 +], [0]) + +ovs-appctl time/stop + +# give time for STP to move initially +for i in $(seq 0 30); do + ovs-appctl time/warp 1000 +done + +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl + p1 designated forwarding 19 128.1 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) + +# add a stp port +AT_CHECK([ + ovs-vsctl add-port br0 p3 -- \ + set interface p3 type=dummy ofport_request=3 +], [0]) + +# The new stp port should be a listening state and other +# stp ports keep forwarding. +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl + p1 designated forwarding 19 128.1 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl + p3 designated listening 19 128.3 +]) + +# delete p1 stp port +AT_CHECK([ovs-vsctl del-port br0 p1]) + +# The other stp ports keep original state. +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl + p3 designated listening 19 128.3 +]) + OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 867a26d..d683000 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -1396,13 +1396,17 @@ port_configure_stp(const struct ofproto *ofproto, struct port *port, bitmap_set1(port_num_bitmap, port_idx); port_s->port_num = port_idx; } else { - if (*port_num_counter >= STP_MAX_PORTS) { - VLOG_ERR("port %s: too many STP ports, disabling", port->name); + if (iface->ofp_port > STP_MAX_PORTS) { + VLOG_ERR("port %s: too many STP ports or OpenFlow port number " + "(%d) > STP_MAX_PORTS (%d), disabling", port->name, + iface->ofp_port, STP_MAX_PORTS); + port_s->enable = false; return; } - port_s->port_num = (*port_num_counter)++; + port_s->port_num = iface->ofp_port -1; + (*port_num_counter)++; } config_str = smap_get(&port->cfg->other_config, "stp-path-cost");
When a bridge stp enabled, we assign sequentially element of stp_port array (in stp struct) to bridge ports. That is ok when no ports are added to bridge. When adding a port to bridge which stp enabled, the ovs-vswitchd will assign stp_port sequentially again. Then the stp_port belonging to one port may belong to other one. This patch uses the OpenFlow port numbers instead of sequence numbers to avoid it. Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> --- tests/stp.at | 90 ++++++++++++++++++++++++++++++++++++++++++------------- vswitchd/bridge.c | 10 +++++-- 2 files changed, 77 insertions(+), 23 deletions(-)