[PATCH net-next v2] bridge: multicast: start querier timer when running user-space stp

[PATCH net-next v2] bridge: multicast: start querier timer when running user-space stp

[Nikolay Aleksandrov]

When STP is running in user-space and querier is configured, the
querier timer is not started when a port goes to a non-blocking state.
This patch unifies the user- and kernel-space stp multicast port enable
path and enables it in all states different from blocking. Note that when a
port goes in BR_STATE_DISABLED it's not enabled because that is handled
in the beginning of the port list loop.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
v2: Repurposed for net-next and implemented Herbert's suggestion for
    unifying both user- and kernel-space multicast enable port paths.

 net/bridge/br_stp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 45f1ff113af9..e7ab74b405a1 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -428,7 +428,6 @@ static void br_make_forwarding(struct net_bridge_port *p)
 	else
 		br_set_state(p, BR_STATE_LEARNING);
 
-	br_multicast_enable_port(p);
 	br_log_state(p);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
@@ -462,6 +461,8 @@ void br_port_state_selection(struct net_bridge *br)
 			}
 		}
 
+		if (p->state != BR_STATE_BLOCKING)
+			br_multicast_enable_port(p);
 		if (p->state == BR_STATE_FORWARDING)
 			++liveports;
 	}
-- 
2.4.3

Re: [PATCH net-next v2] bridge: multicast: start querier timer when running user-space stp

[Herbert Xu]

On Fri, Jun 19, 2015 at 01:45:50AM -0700, Nikolay Aleksandrov wrote:
> When STP is running in user-space and querier is configured, the
> querier timer is not started when a port goes to a non-blocking state.
> This patch unifies the user- and kernel-space stp multicast port enable
> path and enables it in all states different from blocking. Note that when a
> port goes in BR_STATE_DISABLED it's not enabled because that is handled
> in the beginning of the port list loop.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

On a related note, we never disable the multicast querying when
the port goes into blocking mode and we probably should.  So could
you take a look at that and create a patch for it?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next v2] bridge: multicast: start querier timer when running user-space stp

[Nikolay Aleksandrov]

> On Jun 19, 2015, at 4:47 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> On Fri, Jun 19, 2015 at 01:45:50AM -0700, Nikolay Aleksandrov wrote:
>> When STP is running in user-space and querier is configured, the
>> querier timer is not started when a port goes to a non-blocking state.
>> This patch unifies the user- and kernel-space stp multicast port enable
>> path and enables it in all states different from blocking. Note that when a
>> port goes in BR_STATE_DISABLED it's not enabled because that is handled
>> in the beginning of the port list loop.
>> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> On a related note, we never disable the multicast querying when
> the port goes into blocking mode and we probably should.  So could
> you take a look at that and create a patch for it?
> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in

Good catch, I’ll look into it.

Thanks,
 Nik--
To unsubscribe from this list: send the line "unsubscribe netdev" in

[PATCH net-next] bridge: multicast: disable port when in blocking state

[Nikolay Aleksandrov]

Currently when a port goes in blocking state the multicast is not
disabled. Fix it by disabling a port if its state has transitioned to
blocking, this has effect for both user- and kernel-space stp.

Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
note: this is on top of patch:
"bridge: multicast: start querier timer when running user-space stp"

 net/bridge/br_stp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index e7ab74b405a1..1a73c5595f52 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -463,6 +463,8 @@ void br_port_state_selection(struct net_bridge *br)
 
 		if (p->state != BR_STATE_BLOCKING)
 			br_multicast_enable_port(p);
+		else
+			br_multicast_disable_port(p);
 		if (p->state == BR_STATE_FORWARDING)
 			++liveports;
 	}
-- 
2.4.3

Re: [PATCH net-next] bridge: multicast: disable port when in blocking state

[Nikolay Aleksandrov]

> On Jun 22, 2015, at 11:12 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
> Currently when a port goes in blocking state the multicast is not
> disabled. Fix it by disabling a port if its state has transitioned to
> blocking, this has effect for both user- and kernel-space stp.
> 
> Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> note: this is on top of patch:
> "bridge: multicast: start querier timer when running user-space stp"
> 
> net/bridge/br_stp.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index e7ab74b405a1..1a73c5595f52 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -463,6 +463,8 @@ void br_port_state_selection(struct net_bridge *br)
> 
> 		if (p->state != BR_STATE_BLOCKING)
> 			br_multicast_enable_port(p);
> +		else
> +			br_multicast_disable_port(p);
> 		if (p->state == BR_STATE_FORWARDING)
> 			++liveports;
> 	}
> -- 
> 2.4.3
> 

Actually I don’t think this is the correct way to go about this because when the
port goes in blocking state and br_multicast_disable_port() is called then all groups are deleted
which includes the user-added ones.--
To unsubscribe from this list: send the line "unsubscribe netdev" in

Re: [PATCH net-next] bridge: multicast: disable port when in blocking state

[Nikolay Aleksandrov]

> On Jun 22, 2015, at 11:25 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
> 
>> On Jun 22, 2015, at 11:12 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> 
>> Currently when a port goes in blocking state the multicast is not
>> disabled. Fix it by disabling a port if its state has transitioned to
>> blocking, this has effect for both user- and kernel-space stp.
>> 
>> Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>> note: this is on top of patch:
>> "bridge: multicast: start querier timer when running user-space stp"
>> 
>> net/bridge/br_stp.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index e7ab74b405a1..1a73c5595f52 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -463,6 +463,8 @@ void br_port_state_selection(struct net_bridge *br)
>> 
>> 		if (p->state != BR_STATE_BLOCKING)
>> 			br_multicast_enable_port(p);
>> +		else
>> +			br_multicast_disable_port(p);
>> 		if (p->state == BR_STATE_FORWARDING)
>> 			++liveports;
>> 	}
>> -- 
>> 2.4.3
>> 
> 
> Actually I don’t think this is the correct way to go about this because when the
> port goes in blocking state and br_multicast_disable_port() is called then all groups are deleted
> which includes the user-added ones.--
> To unsubscribe from this list: send the line "unsubscribe netdev” in

One more thing - I don’t think we need any additional changes because there’s a check in
br_multicast_port_query_expired():
      if (port->state == BR_STATE_DISABLED ||
            port->state == BR_STATE_BLOCKING)
                goto out;

So it looks like the port should be fine (i.e. not sending) when it goes in blocking state.

Re: [PATCH net-next] bridge: multicast: disable port when in blocking state

[Herbert Xu]

On Mon, Jun 22, 2015 at 11:38:36PM +0300, Nikolay Aleksandrov wrote:
> 
> One more thing - I don’t think we need any additional changes because there’s a check in
> br_multicast_port_query_expired():
>       if (port->state == BR_STATE_DISABLED ||
>             port->state == BR_STATE_BLOCKING)
>                 goto out;
> 
> So it looks like the port should be fine (i.e. not sending) when it goes in blocking state.

Thanks for looking into this! Perhaps we could at least add a
comment to state that the disable_port isn't needed because the
timers will expire and kill themselves automatically?  I think
adding it to the place where you were going to place the disable_port
call would be the most obvious.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next] bridge: multicast: disable port when in blocking state

[Nikolay Aleksandrov]

> On Jun 23, 2015, at 2:52 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> On Mon, Jun 22, 2015 at 11:38:36PM +0300, Nikolay Aleksandrov wrote:
>> 
>> One more thing - I don’t think we need any additional changes because there’s a check in
>> br_multicast_port_query_expired():
>>      if (port->state == BR_STATE_DISABLED ||
>>            port->state == BR_STATE_BLOCKING)
>>                goto out;
>> 
>> So it looks like the port should be fine (i.e. not sending) when it goes in blocking state.
> 
> Thanks for looking into this! Perhaps we could at least add a
> comment to state that the disable_port isn't needed because the
> timers will expire and kill themselves automatically?  I think
> adding it to the place where you were going to place the disable_port
> call would be the most obvious.
> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Right, sounds good. I’ll post a patch with the comment in a bit.

Thank you,
 Nik

Re: [PATCH net-next v2] bridge: multicast: start querier timer when running user-space stp

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Fri, 19 Jun 2015 01:45:50 -0700

> When STP is running in user-space and querier is configured, the
> querier timer is not started when a port goes to a non-blocking state.
> This patch unifies the user- and kernel-space stp multicast port enable
> path and enables it in all states different from blocking. Note that when a
> port goes in BR_STATE_DISABLED it's not enabled because that is handled
> in the beginning of the port list loop.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> v2: Repurposed for net-next and implemented Herbert's suggestion for
>     unifying both user- and kernel-space multicast enable port paths.

Applied.

[PATCH net-next] bridge: multicast: add a comment to br_port_state_selection about blocking state

[Nikolay Aleksandrov]

Add a comment to explain why we're not disabling port's multicast when it
goes in blocking state. Since there's a check in the timer's function which
bypasses the timer if the port's in blocking/disabled state, the timer will
simply expire and stop without sending more queries.

Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_stp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index e7ab74b405a1..b4b6dab9c285 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -463,6 +463,10 @@ void br_port_state_selection(struct net_bridge *br)
 
 		if (p->state != BR_STATE_BLOCKING)
 			br_multicast_enable_port(p);
+		/* Multicast is not disabled for the port when it goes in
+		 * blocking state because the timers will expire and stop by
+		 * themselves without sending more queries.
+		 */
 		if (p->state == BR_STATE_FORWARDING)
 			++liveports;
 	}
-- 
1.9.1

Re: [PATCH net-next] bridge: multicast: add a comment to br_port_state_selection about blocking state

[Herbert Xu]

On Tue, Jun 23, 2015 at 04:47:44AM -0700, Nikolay Aleksandrov wrote:
> Add a comment to explain why we're not disabling port's multicast when it
> goes in blocking state. Since there's a check in the timer's function which
> bypasses the timer if the port's in blocking/disabled state, the timer will
> simply expire and stop without sending more queries.
> 
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt