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

Kishore A K kishoreak at myw.ltindia.com
Fri Jun 25 08:27:46 PDT 2004


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 at osdl.org> 06/24/04 10:24PM >>>
On Thu, 24 Jun 2004 15:02:26 +0530
"Kishore A K" <kishoreak at 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 */




More information about the Bridge mailing list