bridge: Eliminate unnecessary forward delay

bridge: Eliminate unnecessary forward delay

[Herbert Xu]

Hi:

bridge: Eliminate unnecessary forward delay

When the forward delay is set to zero, we still delay the setting
of the forwarding state by one or possibly two timers depending
on whether STP is enabled.  This could either turn out to be
instantaneous, or horribly slow depending on the load of the
machine.

As there is nothing preventing us from enabling forwarding straight
away, this patch eliminates this potential delay by executing the
code directly if the forward delay is zero.

The effect of this problem is that immediately after the carrier
comes on a port, the bridge will drop all packets received from
that port until it enters forwarding mode, thus causing unnecessary
packet loss.

Note that this patch doesn't fully remove the delay due to the
link watcher.  We should also check the carrier state when we
are about to drop an incoming packet because the port is disabled.
But that's for another patch.

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

diff --git a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
index 8b650f7..c612c02 100644
--- a/net/bridge/br_private_stp.h
+++ b/net/bridge/br_private_stp.h
@@ -53,4 +53,7 @@ extern void br_topology_change_detection(struct net_bridge *br);
 extern void br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
 extern void br_send_tcn_bpdu(struct net_bridge_port *);
 
+/* br_stp_timer.c */
+void __br_forward_delay_timer_expired(struct net_bridge_port *p);
+
 #endif
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 921bbe5..70e0890 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -375,7 +375,12 @@ static void br_make_forwarding(struct net_bridge_port *p)
 			p->state = BR_STATE_LEARNING;
 
 		br_log_state(p);
-		mod_timer(&p->forward_delay_timer, jiffies + p->br->forward_delay);	}
+		if (p->br->forward_delay)
+			mod_timer(&p->forward_delay_timer,
+				  jiffies + p->br->forward_delay);
+		else
+			__br_forward_delay_timer_expired(p);
+	}
 }
 
 /* called under bridge lock */
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index 772a140..c061907 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -82,24 +82,41 @@ static void br_message_age_timer_expired(unsigned long arg)
 	spin_unlock(&br->lock);
 }
 
-static void br_forward_delay_timer_expired(unsigned long arg)
+/* called under bridge lock */
+void __br_forward_delay_timer_expired(struct net_bridge_port *p)
 {
-	struct net_bridge_port *p = (struct net_bridge_port *) arg;
 	struct net_bridge *br = p->br;
 
 	pr_debug("%s: %d(%s) forward delay timer\n",
 		 br->dev->name, p->port_no, p->dev->name);
-	spin_lock(&br->lock);
+
 	if (p->state == BR_STATE_LISTENING) {
 		p->state = BR_STATE_LEARNING;
-		mod_timer(&p->forward_delay_timer,
-			  jiffies + br->forward_delay);
-	} else if (p->state == BR_STATE_LEARNING) {
+		if (br->forward_delay) {
+			mod_timer(&p->forward_delay_timer,
+				  jiffies + br->forward_delay);
+			goto out;
+		}
+		br_log_state(p);
+	}
+
+	if (p->state == BR_STATE_LEARNING) {
 		p->state = BR_STATE_FORWARDING;
 		if (br_is_designated_for_some_port(br))
 			br_topology_change_detection(br);
 	}
+
+out:
 	br_log_state(p);
+}
+
+static void br_forward_delay_timer_expired(unsigned long arg)
+{
+	struct net_bridge_port *p = (struct net_bridge_port *) arg;
+	struct net_bridge *br = p->br;
+
+	spin_lock(&br->lock);
+	__br_forward_delay_timer_expired(p);
 	spin_unlock(&br->lock);
 }
 

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Eliminate unnecessary forward delay

[Stephen Hemminger]

On Tue, 5 Aug 2008 01:04:39 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi:
> 
> bridge: Eliminate unnecessary forward delay
> 
> When the forward delay is set to zero, we still delay the setting
> of the forwarding state by one or possibly two timers depending
> on whether STP is enabled.  This could either turn out to be
> instantaneous, or horribly slow depending on the load of the
> machine.
> 
> As there is nothing preventing us from enabling forwarding straight
> away, this patch eliminates this potential delay by executing the
> code directly if the forward delay is zero.
> 
> The effect of this problem is that immediately after the carrier
> comes on a port, the bridge will drop all packets received from
> that port until it enters forwarding mode, thus causing unnecessary
> packet loss.
> 
> Note that this patch doesn't fully remove the delay due to the
> link watcher.  We should also check the carrier state when we
> are about to drop an incoming packet because the port is disabled.
> But that's for another patch.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

How about this, it is simpler just to do the state change.

--- a/net/bridge/br_stp.c	2008-08-04 10:20:21.000000000 -0700
+++ b/net/bridge/br_stp.c	2008-08-04 10:26:04.000000000 -0700
@@ -368,14 +368,26 @@ static void br_make_blocking(struct net_
 /* called under bridge lock */
 static void br_make_forwarding(struct net_bridge_port *p)
 {
-	if (p->state == BR_STATE_BLOCKING) {
-		if (p->br->stp_enabled == BR_KERNEL_STP)
-			p->state = BR_STATE_LISTENING;
-		else
-			p->state = BR_STATE_LEARNING;
+	struct net_bridge *br = p->br;
 
-		br_log_state(p);
-		mod_timer(&p->forward_delay_timer, jiffies + p->br->forward_delay);	}
+	if (p->state != BR_STATE_BLOCKING)
+		return;
+
+	if (br->forward_delay == 0) {
+		p->state = BR_STATE_FORWARDING;
+		if (br_is_designated_for_some_port(br))
+			br_topology_change_detection(br);
+		del_timer(&p->forward_delay_timer);
+	}
+	else if (p->br->stp_enabled == BR_KERNEL_STP)
+		p->state = BR_STATE_LISTENING;
+	else
+		p->state = BR_STATE_LEARNING;
+
+	br_log_state(p);
+
+	if (br->forward_delay != 0)
+		mod_timer(&p->forward_delay_timer, jiffies + br->forward_delay);
 }
 
 /* called under bridge lock */

Re: bridge: Eliminate unnecessary forward delay

[Herbert Xu]

On Mon, Aug 04, 2008 at 10:27:03AM -0700, Stephen Hemminger wrote:
> 
> How about this, it is simpler just to do the state change.

Sure, that should work as well.

> --- a/net/bridge/br_stp.c	2008-08-04 10:20:21.000000000 -0700
> +++ b/net/bridge/br_stp.c	2008-08-04 10:26:04.000000000 -0700
> @@ -368,14 +368,26 @@ static void br_make_blocking(struct net_
>  /* called under bridge lock */
>  static void br_make_forwarding(struct net_bridge_port *p)
>  {
> -	if (p->state == BR_STATE_BLOCKING) {
> -		if (p->br->stp_enabled == BR_KERNEL_STP)
> -			p->state = BR_STATE_LISTENING;
> -		else
> -			p->state = BR_STATE_LEARNING;
> +	struct net_bridge *br = p->br;
>  
> -		br_log_state(p);
> -		mod_timer(&p->forward_delay_timer, jiffies + p->br->forward_delay);	}
> +	if (p->state != BR_STATE_BLOCKING)
> +		return;
> +
> +	if (br->forward_delay == 0) {
> +		p->state = BR_STATE_FORWARDING;
> +		if (br_is_designated_for_some_port(br))
> +			br_topology_change_detection(br);

Provided that you make br_is_designated_for_some_port available
first :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Eliminate unnecessary forward delay

[Stephen Hemminger]

On Tue, 5 Aug 2008 01:04:39 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi:
> 
> bridge: Eliminate unnecessary forward delay
> 
> When the forward delay is set to zero, we still delay the setting
> of the forwarding state by one or possibly two timers depending
> on whether STP is enabled.  This could either turn out to be
> instantaneous, or horribly slow depending on the load of the
> machine.
> 
> As there is nothing preventing us from enabling forwarding straight
> away, this patch eliminates this potential delay by executing the
> code directly if the forward delay is zero.
> 
> The effect of this problem is that immediately after the carrier
> comes on a port, the bridge will drop all packets received from
> that port until it enters forwarding mode, thus causing unnecessary
> packet loss.
> 
> Note that this patch doesn't fully remove the delay due to the
> link watcher.  We should also check the carrier state when we
> are about to drop an incoming packet because the port is disabled.
> But that's for another patch.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 

Same again, this time compile tested...

--- a/net/bridge/br_stp.c	2008-08-04 10:20:21.000000000 -0700
+++ b/net/bridge/br_stp.c	2008-08-04 10:29:26.000000000 -0700
@@ -368,14 +368,25 @@ static void br_make_blocking(struct net_
 /* called under bridge lock */
 static void br_make_forwarding(struct net_bridge_port *p)
 {
-	if (p->state == BR_STATE_BLOCKING) {
-		if (p->br->stp_enabled == BR_KERNEL_STP)
-			p->state = BR_STATE_LISTENING;
-		else
-			p->state = BR_STATE_LEARNING;
+	struct net_bridge *br = p->br;
 
-		br_log_state(p);
-		mod_timer(&p->forward_delay_timer, jiffies + p->br->forward_delay);	}
+	if (p->state != BR_STATE_BLOCKING)
+		return;
+
+	if (br->forward_delay == 0) {
+		p->state = BR_STATE_FORWARDING;
+		br_topology_change_detection(br);
+		del_timer(&p->forward_delay_timer);
+	}
+	else if (p->br->stp_enabled == BR_KERNEL_STP)
+		p->state = BR_STATE_LISTENING;
+	else
+		p->state = BR_STATE_LEARNING;
+
+	br_log_state(p);
+
+	if (br->forward_delay != 0)
+		mod_timer(&p->forward_delay_timer, jiffies + br->forward_delay);
 }
 
 /* called under bridge lock */

Re: bridge: Eliminate unnecessary forward delay

[Herbert Xu]

On Mon, Aug 04, 2008 at 10:35:34AM -0700, Stephen Hemminger wrote:
>
> Same again, this time compile tested...
> 
> --- a/net/bridge/br_stp.c	2008-08-04 10:20:21.000000000 -0700
> +++ b/net/bridge/br_stp.c	2008-08-04 10:29:26.000000000 -0700
> @@ -368,14 +368,25 @@ static void br_make_blocking(struct net_
>  /* called under bridge lock */
>  static void br_make_forwarding(struct net_bridge_port *p)
>  {
> -	if (p->state == BR_STATE_BLOCKING) {

Is it safe to remove this check, especially if the forward delay
is non-zero?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Eliminate unnecessary forward delay

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 5 Aug 2008 10:12:40 +0800

> On Mon, Aug 04, 2008 at 10:35:34AM -0700, Stephen Hemminger wrote:
> >
> > Same again, this time compile tested...
> > 
> > --- a/net/bridge/br_stp.c	2008-08-04 10:20:21.000000000 -0700
> > +++ b/net/bridge/br_stp.c	2008-08-04 10:29:26.000000000 -0700
> > @@ -368,14 +368,25 @@ static void br_make_blocking(struct net_
> >  /* called under bridge lock */
> >  static void br_make_forwarding(struct net_bridge_port *p)
> >  {
> > -	if (p->state == BR_STATE_BLOCKING) {
> 
> Is it safe to remove this check, especially if the forward delay
> is non-zero?

He's not deleting it as far as I can tell.

The old code is:

	if (p->state == BR_STATE_BLOCKING) {
		if (p->br->stp_enabled == BR_KERNEL_STP)
			p->state = BR_STATE_LISTENING;
		else
			p->state = BR_STATE_LEARNING;

		br_log_state(p);
		mod_timer(&p->forward_delay_timer, jiffies + p->br->forward_delay);
	}

and the new code is:

	struct net_bridge *br = p->br;
 
	if (p->state != BR_STATE_BLOCKING)
		return;

	if (br->forward_delay == 0) {
		p->state = BR_STATE_FORWARDING;
		br_topology_change_detection(br);
		del_timer(&p->forward_delay_timer);
	}
	else if (p->br->stp_enabled == BR_KERNEL_STP)
		p->state = BR_STATE_LISTENING;
	else
		p->state = BR_STATE_LEARNING;

	br_log_state(p);

	if (br->forward_delay != 0)
		mod_timer(&p->forward_delay_timer, jiffies + br->forward_delay);

So when p->state is not BR_STATE_BLOCKING, both before and after this
patch, the function does nothing.

Are your concerns still present?  I'd like to apply this, so let me know.

Re: bridge: Eliminate unnecessary forward delay

[Herbert Xu]

On Tue, Aug 05, 2008 at 01:40:06AM -0700, David Miller wrote:
>
> So when p->state is not BR_STATE_BLOCKING, both before and after this
> patch, the function does nothing.

Thanks for pointing this out.

> Are your concerns still present?  I'd like to apply this, so let me know.

No it looks good to me.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt