Re: [Bridge] Re: [Patch] [2.6.7] Bridge - Fix BPDU message_age

Re: [Bridge] Re: [Patch] [2.6.7] Bridge - Fix BPDU message_age

[Kishore A K]

Hi Stephen,

The patch looks good except that br_send_config_bpdu() returns zero in either case.

But I would suggest you try & keep the logic in the function br_transmit_config(). So 
that it would remain in sync with the IEEE 802.1D spec. And that way it would easier
to debug any problems and maintain the code in the future.

Oneway would be incrementing the message age by a factor,
   ((HZ > 255) ? ( HZ  >> 8 ) : 1)

I know you would now say that when (HZ <= 255),  an increment of 1 jiffie here would be 
greater than a corresponding increment in (1/256) ticks. But the protocol allows for a
Message Age Increment overestimate of 1 second.

Otherwise the patch looks really fine.

Regards,
Kishore

 

>>> Stephen Hemminger <shemminger@osdl.org> 06/24/04 10:24PM >>>
On Thu, 24 Jun 2004 15:02:26 +0530
"Kishore A K" <kishoreak@myw.ltindia.com> wrote:

> Hi Stephen,
> 
> >>First, we shouldn't send out the config info if our info about the root is too old.
> 
> Thats why I had introduced the check "if (bpdu.message_age < br->max_age)".
> The purpose of maintaining the message_age parameter is to enable a bridge to 
> discard information whose age exceeds Max Age. So I think this would be the right 
> way of doing it. You may refer the section "8.6.1.3.3 step 3" in the IEEE 802.1D spec. 
> Above that I really did not get the logic behind doing "if (age < stp_t_to_jiffies(2))".
> Pls clarify.
> 
> >>Second, since the time values in the BPDU are scaled from jiffies to 1/256 sec later
> >>in the send process, need to increment age by the appropriate value.
> 
> I had completely forgotten about this. However, the purpose of doing the increment is to 
> avoid underestimating the age, which is very unlikely to happen. Anyway this can be easily 
> done by incrementing the age by a factor ( (1 * HZ)>>8) instead of 1.
> 
> >>+		long age = (long) root->message_age_timer.expires
> >>+			- (long)jiffies;

> "message_age_timer.expires - jiffies" does not give the message age. Instead it gives
> the time left before the message age timer expires. Whereas Message age is the time
> elapsed since the generation of the configuration BPDU by the root. So the right way
> of getting the message age according to me would be 
> 
> message_age = max_age - (message_age_timer.expires - jiffies) + (HZ>>8)
> 
> Pls correct me if I am wrong here.

Doesn't work if HZ == 100 because then 100 >> 8 = 0.  

Here is an alternative that pushes the increment and message_age < max_age
check down into the send_config_bpdu routine where the conversion to (1/256) ticks
has already taken place.

diff -Nru a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
--- a/net/bridge/br_private_stp.h	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_private_stp.h	2004-06-24 09:52:25 -07:00
@@ -52,7 +52,7 @@
 extern void br_topology_change_detection(struct net_bridge *br);
 
 /* br_stp_bpdu.c */
-extern void br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
+extern int br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
 extern void br_send_tcn_bpdu(struct net_bridge_port *);
 
 #endif
diff -Nru a/net/bridge/br_stp.c b/net/bridge/br_stp.c
--- a/net/bridge/br_stp.c	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_stp.c	2004-06-24 09:52:25 -07:00
@@ -157,24 +157,25 @@
 	bpdu.root_path_cost = br->root_path_cost;
 	bpdu.bridge_id = br->bridge_id;
 	bpdu.port_id = p->port_id;
-	bpdu.message_age = 0;
-	if (!br_is_root_bridge(br)) {
+
+	if (br_is_root_bridge(br)) 
+		bpdu.message_age = 0;
+	else {
 		struct net_bridge_port *root
 			= br_get_port(br, br->root_port);
-		bpdu.max_age = root->message_age_timer.expires - jiffies;
-
-		if (bpdu.max_age <= 0) bpdu.max_age = 1;
+		bpdu.message_age = br->max_age 
+			- (root->message_age_timer.expires - jiffies);
 	}
+
 	bpdu.max_age = br->max_age;
 	bpdu.hello_time = br->hello_time;
 	bpdu.forward_delay = br->forward_delay;
 
-	br_send_config_bpdu(p, &bpdu);
-
-	p->topology_change_ack = 0;
-	p->config_pending = 0;
-	
-	mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	if (br_send_config_bpdu(p, &bpdu)) {
+		p->topology_change_ack = 0;
+		p->config_pending = 0;
+		mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	}
 }
 
 /* called under bridge lock */
diff -Nru a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
--- a/net/bridge/br_stp_bpdu.c	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_stp_bpdu.c	2004-06-24 09:52:25 -07:00
@@ -72,9 +72,10 @@
 }
 
 /* called under bridge lock */
-void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
+int br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 {
 	unsigned char buf[38];
+	u16 max_age;
 
 	buf[0] = 0x42;
 	buf[1] = 0x42;
@@ -108,12 +109,26 @@
 	buf[28] = (bpdu->port_id >> 8) & 0xFF;
 	buf[29] = bpdu->port_id & 0xFF;
 
-	br_set_ticks(buf+30, bpdu->message_age);
-	br_set_ticks(buf+32, bpdu->max_age);
+	max_age = JIFFIES_TO_TICKS(bpdu->max_age);
+	if (bpdu->message_age) {
+		u16 age = JIFFIES_TO_TICKS(bpdu->message_age)+1;
+		if (age >= max_age)
+			return 0;
+		buf[30] = (age >> 8) & 0xFF;
+		buf[31] = age & 0xFF;
+	} else {
+		buf[30] = 0;
+		buf[31] = 0;
+	}
+
+	buf[32] = (max_age >> 8) & 0xFF;
+	buf[33] = max_age & 0xFF;
+
 	br_set_ticks(buf+34, bpdu->hello_time);
 	br_set_ticks(buf+36, bpdu->forward_delay);
 
 	br_send_bpdu(p, buf, 38);
+	return 0;
 }
 
 /* called under bridge lock */

Re: [Bridge] Re: [Patch] [2.6.7] Bridge - Fix BPDU message_age

[Stephen Hemminger]

On Thu, 24 Jun 2004 15:02:26 +0530
"Kishore A K" <kishoreak@myw.ltindia.com> wrote:

> Hi Stephen,
> 
> >>First, we shouldn't send out the config info if our info about the root is too old.
> 
> Thats why I had introduced the check "if (bpdu.message_age < br->max_age)".
> The purpose of maintaining the message_age parameter is to enable a bridge to 
> discard information whose age exceeds Max Age. So I think this would be the right 
> way of doing it. You may refer the section "8.6.1.3.3 step 3" in the IEEE 802.1D spec. 
> Above that I really did not get the logic behind doing "if (age < stp_t_to_jiffies(2))".
> Pls clarify.
> 
> >>Second, since the time values in the BPDU are scaled from jiffies to 1/256 sec later
> >>in the send process, need to increment age by the appropriate value.
> 
> I had completely forgotten about this. However, the purpose of doing the increment is to 
> avoid underestimating the age, which is very unlikely to happen. Anyway this can be easily 
> done by incrementing the age by a factor ( (1 * HZ)>>8) instead of 1.
> 
> >>+		long age = (long) root->message_age_timer.expires
> >>+			- (long)jiffies;

> "message_age_timer.expires - jiffies" does not give the message age. Instead it gives
> the time left before the message age timer expires. Whereas Message age is the time
> elapsed since the generation of the configuration BPDU by the root. So the right way
> of getting the message age according to me would be 
> 
> message_age = max_age - (message_age_timer.expires - jiffies) + (HZ>>8)
> 
> Pls correct me if I am wrong here.

Doesn't work if HZ == 100 because then 100 >> 8 = 0.  

Here is an alternative that pushes the increment and message_age < max_age
check down into the send_config_bpdu routine where the conversion to (1/256) ticks
has already taken place.

diff -Nru a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
--- a/net/bridge/br_private_stp.h	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_private_stp.h	2004-06-24 09:52:25 -07:00
@@ -52,7 +52,7 @@
 extern void br_topology_change_detection(struct net_bridge *br);
 
 /* br_stp_bpdu.c */
-extern void br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
+extern int br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
 extern void br_send_tcn_bpdu(struct net_bridge_port *);
 
 #endif
diff -Nru a/net/bridge/br_stp.c b/net/bridge/br_stp.c
--- a/net/bridge/br_stp.c	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_stp.c	2004-06-24 09:52:25 -07:00
@@ -157,24 +157,25 @@
 	bpdu.root_path_cost = br->root_path_cost;
 	bpdu.bridge_id = br->bridge_id;
 	bpdu.port_id = p->port_id;
-	bpdu.message_age = 0;
-	if (!br_is_root_bridge(br)) {
+
+	if (br_is_root_bridge(br)) 
+		bpdu.message_age = 0;
+	else {
 		struct net_bridge_port *root
 			= br_get_port(br, br->root_port);
-		bpdu.max_age = root->message_age_timer.expires - jiffies;
-
-		if (bpdu.max_age <= 0) bpdu.max_age = 1;
+		bpdu.message_age = br->max_age 
+			- (root->message_age_timer.expires - jiffies);
 	}
+
 	bpdu.max_age = br->max_age;
 	bpdu.hello_time = br->hello_time;
 	bpdu.forward_delay = br->forward_delay;
 
-	br_send_config_bpdu(p, &bpdu);
-
-	p->topology_change_ack = 0;
-	p->config_pending = 0;
-	
-	mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	if (br_send_config_bpdu(p, &bpdu)) {
+		p->topology_change_ack = 0;
+		p->config_pending = 0;
+		mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	}
 }
 
 /* called under bridge lock */
diff -Nru a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
--- a/net/bridge/br_stp_bpdu.c	2004-06-24 09:52:25 -07:00
+++ b/net/bridge/br_stp_bpdu.c	2004-06-24 09:52:25 -07:00
@@ -72,9 +72,10 @@
 }
 
 /* called under bridge lock */
-void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
+int br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 {
 	unsigned char buf[38];
+	u16 max_age;
 
 	buf[0] = 0x42;
 	buf[1] = 0x42;
@@ -108,12 +109,26 @@
 	buf[28] = (bpdu->port_id >> 8) & 0xFF;
 	buf[29] = bpdu->port_id & 0xFF;
 
-	br_set_ticks(buf+30, bpdu->message_age);
-	br_set_ticks(buf+32, bpdu->max_age);
+	max_age = JIFFIES_TO_TICKS(bpdu->max_age);
+	if (bpdu->message_age) {
+		u16 age = JIFFIES_TO_TICKS(bpdu->message_age)+1;
+		if (age >= max_age)
+			return 0;
+		buf[30] = (age >> 8) & 0xFF;
+		buf[31] = age & 0xFF;
+	} else {
+		buf[30] = 0;
+		buf[31] = 0;
+	}
+
+	buf[32] = (max_age >> 8) & 0xFF;
+	buf[33] = max_age & 0xFF;
+
 	br_set_ticks(buf+34, bpdu->hello_time);
 	br_set_ticks(buf+36, bpdu->forward_delay);
 
 	br_send_bpdu(p, buf, 38);
+	return 0;
 }
 
 /* called under bridge lock */

[Bridge] Re: [Patch] [2.6.7] Bridge - Fix BPDU message_age

[Kishore A K]

Hi Stephen,

>>First, we shouldn't send out the config info if our info about the root is too old.

Thats why I had introduced the check "if (bpdu.message_age < br->max_age)".
The purpose of maintaining the message_age parameter is to enable a bridge to 
discard information whose age exceeds Max Age. So I think this would be the right 
way of doing it. You may refer the section "8.6.1.3.3 step 3" in the IEEE 802.1D spec. 
Above that I really did not get the logic behind doing "if (age < stp_t_to_jiffies(2))".
Pls clarify.

>>Second, since the time values in the BPDU are scaled from jiffies to 1/256 sec later
>>in the send process, need to increment age by the appropriate value.

I had completely forgotten about this. However, the purpose of doing the increment is to 
avoid underestimating the age, which is very unlikely to happen. Anyway this can be easily 
done by incrementing the age by a factor ( (1 * HZ)>>8) instead of 1.

>>+		long age = (long) root->message_age_timer.expires
>>+			- (long)jiffies;

"message_age_timer.expires - jiffies" does not give the message age. Instead it gives
the time left before the message age timer expires. Whereas Message age is the time
elapsed since the generation of the configuration BPDU by the root. So the right way
of getting the message age according to me would be 

message_age = max_age - (message_age_timer.expires - jiffies) + (HZ>>8)

Pls correct me if I am wrong here.

Hence the updated patch,

Signed-off-by: Kishore A K <KishoreAK@myw.ltindia.com>

Index: linux-2.6.7/net/bridge/br_stp.c
=============================================================

--- linux-2.6.7/net/bridge/br_stp.c.orig	2004-06-17 20:17:27.000000000 +0530
+++ linux-2.6.7/net/bridge/br_stp.c	2004-06-24 14:45:22.318961664 +0530
@@ -161,20 +161,19 @@ void br_transmit_config(struct net_bridg
 	if (!br_is_root_bridge(br)) {
 		struct net_bridge_port *root
 			= br_get_port(br, br->root_port);
-		bpdu.max_age = root->message_age_timer.expires - jiffies;
-
-		if (bpdu.max_age <= 0) bpdu.max_age = 1;
+		bpdu.message_age = br->max_age - 
+			(root->message_age_timer.expires - jiffies) + (HZ >> 8);
 	}
 	bpdu.max_age = br->max_age;
 	bpdu.hello_time = br->hello_time;
 	bpdu.forward_delay = br->forward_delay;
 
-	br_send_config_bpdu(p, &bpdu);
-
-	p->topology_change_ack = 0;
-	p->config_pending = 0;
-	
-	mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	if (bpdu.message_age < br->max_age) {
+		br_send_config_bpdu(p, &bpdu);
+		p->topology_change_ack = 0;
+		p->config_pending = 0;
+		mod_timer(&p->hold_timer, jiffies + BR_HOLD_TIME);
+	}
 }
 
 /* called under bridge lock */

 

[Bridge] Re: [Patch] [2.6.7] Bridge - Fix BPDU message_age

[Stephen Hemminger]

On Tue, 22 Jun 2004 20:21:24 +0530
"Kishore A K" <kishoreak@myw.ltindia.com> wrote:

> Fixes message_age field update in config BPDUs. Also checks whether the BPDU 
> message age has exceeded bridge max age before transmitting config BPDUs.
> 

The idea is correct, but there are a couple of other issues.
First, we shouldn't send out the config info if our info about the root is too old.
Second, since the time values in the BPDU are scaled from jiffies to 1/256 sec later
in the send process, need to increment age by the appropriate value.

Try this instead please. If it works, I'll send it to Dave.


diff -Nru a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
--- a/net/bridge/br_private_stp.h	2004-06-23 14:18:53 -07:00
+++ b/net/bridge/br_private_stp.h	2004-06-23 14:18:53 -07:00
@@ -26,7 +26,7 @@
 	int		root_path_cost;
 	bridge_id	bridge_id;
 	port_id		port_id;
-	int		message_age;
+	u16		message_age;
 	int		max_age;
 	int		hello_time;
 	int		forward_delay;
@@ -39,6 +39,17 @@
 		(p->designated_port == p->port_id);
 }
 
+
+/* convert system to ticks to 1/256 sec */
+static inline u16 jiffies_to_stp_t(unsigned long j)
+{
+	return (j * 256) / HZ;
+}
+
+static inline unsigned long stp_t_to_jiffies(u16 t)
+{
+	return ((unsigned long) t * HZ) / 256;
+}
 
 /* br_stp.c */
 extern void br_become_root_bridge(struct net_bridge *br);
diff -Nru a/net/bridge/br_stp.c b/net/bridge/br_stp.c
--- a/net/bridge/br_stp.c	2004-06-23 14:18:53 -07:00
+++ b/net/bridge/br_stp.c	2004-06-23 14:18:53 -07:00
@@ -157,14 +157,21 @@
 	bpdu.root_path_cost = br->root_path_cost;
 	bpdu.bridge_id = br->bridge_id;
 	bpdu.port_id = p->port_id;
-	bpdu.message_age = 0;
-	if (!br_is_root_bridge(br)) {
-		struct net_bridge_port *root
-			= br_get_port(br, br->root_port);
-		bpdu.max_age = root->message_age_timer.expires - jiffies;
 
-		if (bpdu.max_age <= 0) bpdu.max_age = 1;
+	if (br_is_root_bridge(br)) 
+		bpdu.message_age = 0;
+	else {
+		struct net_bridge_port *root = br_get_port(br, br->root_port);
+		long age = (long) root->message_age_timer.expires
+			- (long)jiffies;
+
+		/* our info about the root is stale */
+		if (age < stp_t_to_jiffies(2))
+			return;
+
+		bpdu.message_age = jiffies_to_stp_t(age) + 1;
 	}
+
 	bpdu.max_age = br->max_age;
 	bpdu.hello_time = br->hello_time;
 	bpdu.forward_delay = br->forward_delay;
diff -Nru a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
--- a/net/bridge/br_stp_bpdu.c	2004-06-23 14:18:53 -07:00
+++ b/net/bridge/br_stp_bpdu.c	2004-06-23 14:18:53 -07:00
@@ -19,9 +19,6 @@
 #include "br_private.h"
 #include "br_private_stp.h"
 
-#define JIFFIES_TO_TICKS(j) (((j) << 8) / HZ)
-#define TICKS_TO_JIFFIES(j) (((j) * HZ) >> 8)
-
 static void br_send_bpdu(struct net_bridge_port *p, unsigned char *data, int length)
 {
 	struct net_device *dev;
@@ -57,18 +54,17 @@
 		dev_queue_xmit);
 }
 
-static __inline__ void br_set_ticks(unsigned char *dest, int jiff)
+static inline void br_set_ticks(unsigned char *dest, int jiff)
 {
-	__u16 ticks;
+	__u16 ticks = jiffies_to_stp_t(jiff);
 
-	ticks = JIFFIES_TO_TICKS(jiff);
 	dest[0] = (ticks >> 8) & 0xFF;
 	dest[1] = ticks & 0xFF;
 }
 
-static __inline__ int br_get_ticks(unsigned char *dest)
+static inline int br_get_ticks(const unsigned char *dest)
 {
-	return TICKS_TO_JIFFIES((dest[0] << 8) | dest[1]);
+	return stp_t_to_jiffies((dest[0] << 8) | dest[1]);
 }
 
 /* called under bridge lock */
@@ -108,7 +104,9 @@
 	buf[28] = (bpdu->port_id >> 8) & 0xFF;
 	buf[29] = bpdu->port_id & 0xFF;
 
-	br_set_ticks(buf+30, bpdu->message_age);
+	buf[30] = (bpdu->message_age >> 8) & 0xFF;
+	buf[31] = bpdu->message_age & 0xFF;
+
 	br_set_ticks(buf+32, bpdu->max_age);
 	br_set_ticks(buf+34, bpdu->hello_time);
 	br_set_ticks(buf+36, bpdu->forward_delay);