Re: [ovs-dev] [PATCH 3/6] stp: Add link-state checking support for stp ports.

[nickcooper-zhangtonghao]

> On Apr 21, 2017, at 8:52 AM, Jarno Rajahalme  wrote:
> 
>> 
>> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao > > wrote:
>> 
>> When bridge stp enabled, we enable the stp ports despite
>> ports are down. When initializing, this patch checks
>> link-state of ports and enable or disable them according
>> to their link-state. This patch also allow user to enable
>> and disable a port when bridge stp is running.
>> 
> 
> This describes what the patch does but gives little help for understanding 
> why this change is needed. STP would notice that the link is down as it is 
> not able to exchange BPDUs over that link. Also, a link that is in 
> STP_DISABLED state forwards all traffic, so that when the link comes up, but 
> before stp_run() manages to enable STP there would be a loop in the network. 
> To prevent this it seems to me that we should leave STP enabled also when the 
> link goes down, so that STP would have the chance to initially block to port 
> when it comes back up.
> 
>  Jarno


Yes, STP would notice that the link is down as it is not able to exchange BPDUs 
over that link. If a link is down(e.g ifconfig eth0 down), it will not forward 
any traffic whenever stp port is STP_DISABLE or STP_ENABLE. And is there a loop 
in the network ? And if one interface is down, the command ‘ovs-appctl 
stp/show’ still show it(its role is designated and state is forwarding). The 
forwarding state of interface which is down, may confuse the users and 
developers.

I tested stp on cisco switch. If you shutdown the stp interface, you cannot get 
the interface info and the interface linked to it.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/6] stp: Add link-state checking support for stp ports.

[Jarno Rajahalme]

> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao  
> wrote:
> 
> When bridge stp enabled, we enable the stp ports despite
> ports are down. When initializing, this patch checks
> link-state of ports and enable or disable them according
> to their link-state. This patch also allow user to enable
> and disable a port when bridge stp is running.
> 

This describes what the patch does but gives little help for understanding why 
this change is needed. STP would notice that the link is down as it is not able 
to exchange BPDUs over that link. Also, a link that is in STP_DISABLED state 
forwards all traffic, so that when the link comes up, but before stp_run() 
manages to enable STP there would be a loop in the network. To prevent this it 
seems to me that we should leave STP enabled also when the link goes down, so 
that STP would have the chance to initially block to port when it comes back up.

  Jarno

> Signed-off-by: nickcooper-zhangtonghao 
> ---
> ofproto/ofproto-dpif.c | 41 +++-
> tests/stp.at   | 73 ++
> 2 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4beacda..f015131 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2488,6 +2488,37 @@ update_stp_port_state(struct ofport_dpif *ofport)
> }
> }
> 
> +static void
> +stp_check_and_update_link_state(struct ofproto_dpif *ofproto)
> +{
> +struct ofport *ofport_;
> +struct ofport_dpif *ofport;
> +bool up;
> +
> +HMAP_FOR_EACH (ofport_, hmap_node, &ofproto->up.ports) {
> +ofport = ofport_dpif_cast(ofport_);
> +up = netdev_get_carrier(ofport_->netdev);
> +
> +if (ofport->stp_port &&
> +up != (stp_port_get_state(ofport->stp_port) != STP_DISABLED)) {
> +
> +VLOG_DBG("bridge: %s, port: %s is %s, %s it", ofproto->up.name,
> + netdev_get_name(ofport->up.netdev),
> + up ? "up" : "down",
> + up ? "enabling" : "disabling");
> +
> +if (up) {
> +stp_port_enable(ofport->stp_port);
> +stp_port_set_aux(ofport->stp_port, ofport);
> +} else {
> +stp_port_disable(ofport->stp_port);
> +}
> +
> +update_stp_port_state(ofport);
> +}
> +}
> +}
> +
> /* Configures STP on 'ofport_' using the settings defined in 's'.  The
>  * caller is responsible for assigning STP port numbers and ensuring
>  * there are no duplicates. */
> @@ -2518,7 +2549,12 @@ set_stp_port(struct ofport *ofport_,
> /* Set name before enabling the port so that debugging messages can print
>  * the name. */
> stp_port_set_name(sp, netdev_get_name(ofport->up.netdev));
> -stp_port_enable(sp);
> +
> +if (netdev_get_carrier(ofport_->netdev)) {
> +stp_port_enable(sp);
> +} else {
> +stp_port_disable(sp);
> +}
> 
> stp_port_set_aux(sp, ofport);
> stp_port_set_priority(sp, s->priority);
> @@ -2580,6 +2616,9 @@ stp_run(struct ofproto_dpif *ofproto)
> stp_tick(ofproto->stp, MIN(INT_MAX, elapsed));
> ofproto->stp_last_tick = now;
> }
> +
> +stp_check_and_update_link_state(ofproto);
> +
> while (stp_get_changed_port(ofproto->stp, &sp)) {
> struct ofport_dpif *ofport = stp_port_get_aux(sp);
> 
> diff --git a/tests/stp.at b/tests/stp.at
> index 98632a8..de8f971 100644
> --- a/tests/stp.at
> +++ b/tests/stp.at
> @@ -420,6 +420,8 @@ AT_CHECK([ovs-vsctl add-port br1 p8 -- \
>set port p8 other_config:stp-enable=false -- \
> ])
> 
> +ovs-appctl netdev-dummy/set-admin-state up
> +
> ovs-appctl time/stop
> 
> AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"])
> @@ -519,6 +521,7 @@ AT_CHECK([
> set interface p6 type=dummy options:pstream=punix:$OVS_RUNDIR/p6.sock 
> ofport_request=6
> ], [0])
> 
> +ovs-appctl netdev-dummy/set-admin-state up
> 
> ovs-appctl time/stop
> 
> @@ -633,6 +636,8 @@ AT_CHECK([
> set interface p2 type=dummy ofport_request=2
> ], [0])
> 
> +ovs-appctl netdev-dummy/set-admin-state up
> +
> ovs-appctl time/stop
> 
> # give time for STP to move initially
> @@ -653,6 +658,8 @@ AT_CHECK([
> set interface p3 type=dummy ofport_request=3
> ], [0])
> 
> +ovs-appctl netdev-dummy/set-admin-state p3 up
> +
> # 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
> @@ -676,5 +683,71 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl
>   p3 designated listening  19128.3
> ])
> 
> +AT_CLEANUP
> +
> +AT_SETUP([STP - check link-state when stp is running])
> +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:h

[ovs-dev] [PATCH 3/6] stp: Add link-state checking support for stp ports.

[nickcooper-zhangtonghao]

When bridge stp enabled, we enable the stp ports despite
ports are down. When initializing, this patch checks
link-state of ports and enable or disable them according
to their link-state. This patch also allow user to enable
and disable a port when bridge stp is running.

Signed-off-by: nickcooper-zhangtonghao 
---
 ofproto/ofproto-dpif.c | 41 +++-
 tests/stp.at   | 73 ++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4beacda..f015131 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2488,6 +2488,37 @@ update_stp_port_state(struct ofport_dpif *ofport)
 }
 }
 
+static void
+stp_check_and_update_link_state(struct ofproto_dpif *ofproto)
+{
+struct ofport *ofport_;
+struct ofport_dpif *ofport;
+bool up;
+
+HMAP_FOR_EACH (ofport_, hmap_node, &ofproto->up.ports) {
+ofport = ofport_dpif_cast(ofport_);
+up = netdev_get_carrier(ofport_->netdev);
+
+if (ofport->stp_port &&
+up != (stp_port_get_state(ofport->stp_port) != STP_DISABLED)) {
+
+VLOG_DBG("bridge: %s, port: %s is %s, %s it", ofproto->up.name,
+ netdev_get_name(ofport->up.netdev),
+ up ? "up" : "down",
+ up ? "enabling" : "disabling");
+
+if (up) {
+stp_port_enable(ofport->stp_port);
+stp_port_set_aux(ofport->stp_port, ofport);
+} else {
+stp_port_disable(ofport->stp_port);
+}
+
+update_stp_port_state(ofport);
+}
+}
+}
+
 /* Configures STP on 'ofport_' using the settings defined in 's'.  The
  * caller is responsible for assigning STP port numbers and ensuring
  * there are no duplicates. */
@@ -2518,7 +2549,12 @@ set_stp_port(struct ofport *ofport_,
 /* Set name before enabling the port so that debugging messages can print
  * the name. */
 stp_port_set_name(sp, netdev_get_name(ofport->up.netdev));
-stp_port_enable(sp);
+
+if (netdev_get_carrier(ofport_->netdev)) {
+stp_port_enable(sp);
+} else {
+stp_port_disable(sp);
+}
 
 stp_port_set_aux(sp, ofport);
 stp_port_set_priority(sp, s->priority);
@@ -2580,6 +2616,9 @@ stp_run(struct ofproto_dpif *ofproto)
 stp_tick(ofproto->stp, MIN(INT_MAX, elapsed));
 ofproto->stp_last_tick = now;
 }
+
+stp_check_and_update_link_state(ofproto);
+
 while (stp_get_changed_port(ofproto->stp, &sp)) {
 struct ofport_dpif *ofport = stp_port_get_aux(sp);
 
diff --git a/tests/stp.at b/tests/stp.at
index 98632a8..de8f971 100644
--- a/tests/stp.at
+++ b/tests/stp.at
@@ -420,6 +420,8 @@ AT_CHECK([ovs-vsctl add-port br1 p8 -- \
set port p8 other_config:stp-enable=false -- \
 ])
 
+ovs-appctl netdev-dummy/set-admin-state up
+
 ovs-appctl time/stop
 
 AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"])
@@ -519,6 +521,7 @@ AT_CHECK([
 set interface p6 type=dummy options:pstream=punix:$OVS_RUNDIR/p6.sock 
ofport_request=6
 ], [0])
 
+ovs-appctl netdev-dummy/set-admin-state up
 
 ovs-appctl time/stop
 
@@ -633,6 +636,8 @@ AT_CHECK([
 set interface p2 type=dummy ofport_request=2
 ], [0])
 
+ovs-appctl netdev-dummy/set-admin-state up
+
 ovs-appctl time/stop
 
 # give time for STP to move initially
@@ -653,6 +658,8 @@ AT_CHECK([
 set interface p3 type=dummy ofport_request=3
 ], [0])
 
+ovs-appctl netdev-dummy/set-admin-state p3 up
+
 # 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
@@ -676,5 +683,71 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl
p3 designated listening  19128.3
 ])
 
+AT_CLEANUP
+
+AT_SETUP([STP - check link-state when stp is running])
+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 netdev-dummy/set-admin-state up
+
+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 19128.1
+])
+AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl
+   p2 designated forwarding 19128.2
+])
+
+# add a stp port
+AT_CHECK([
+ovs-vsctl add-port br0 p3 -- \
+set interface p3 type=dummy ofport_request=3
+], [0])
+
+ovs-appctl netdev-dummy/set-admin-state p3 down
+
+# We should not show the p