Joakim Tjernlund
2012-Feb-28 18:37 UTC
[Bridge] [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
min age increment needs to round up its min age tick for all HZ values to guarantee message age is increasing. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se> --- net/bridge/br_stp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index a04eeb6..9a8aebd 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -17,9 +17,9 @@ #include "br_private_stp.h" /* since time values in bpdu are in jiffies and then scaled (1/256) - * before sending, make sure that is at least one. + * before sending, make sure that is at least one STP tick. */ -#define MESSAGE_AGE_INCR ((HZ < 256) ? 1 : (HZ/256)) +#define MESSAGE_AGE_INCR ((HZ / 256) + 1) static const char *const br_port_state_names[] = { [BR_STATE_DISABLED] = "disabled", -- 1.7.3.4
Joakim Tjernlund
2012-Feb-28 18:37 UTC
[Bridge] [PATCH 2/2] bridge: message age needs to increase, not decrease.
commit bridge: send proper message_age in config BPDU added this gem: bpdu.message_age = (jiffies - root->designated_age) p->designated_age = jiffies + bpdu->message_age; Notice how bpdu->message_age is negated when reassigned to bpdu.message_age. This causes message age to decrease breaking the STP protocol. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se> --- net/bridge/br_stp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index 9a8aebd..45331fe 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -188,7 +188,7 @@ static inline void br_record_config_information(struct net_bridge_port *p, p->designated_cost = bpdu->root_path_cost; p->designated_bridge = bpdu->bridge_id; p->designated_port = bpdu->port_id; - p->designated_age = jiffies + bpdu->message_age; + p->designated_age = jiffies - bpdu->message_age; mod_timer(&p->message_age_timer, jiffies + (p->br->max_age - bpdu->message_age)); -- 1.7.3.4
Joakim Tjernlund
2012-Mar-01 00:31 UTC
[Bridge] [PATCH 2/2] bridge: message age needs to increase, not decrease.
Ping? I think this a fairly big problem and should go to Linus before next release. Same for bridge: Adjust min age inc for HZ > 256 Jocke Joakim Tjernlund <Joakim.Tjernlund at transmode.se> wrote on 2012/02/28 19:37:11:> > commit bridge: send proper message_age in config BPDU > added this gem: > bpdu.message_age = (jiffies - root->designated_age) > p->designated_age = jiffies + bpdu->message_age; > Notice how bpdu->message_age is negated when reassigned to > bpdu.message_age. This causes message age to decrease breaking the > STP protocol. > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se> > --- > net/bridge/br_stp.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c > index 9a8aebd..45331fe 100644 > --- a/net/bridge/br_stp.c > +++ b/net/bridge/br_stp.c > @@ -188,7 +188,7 @@ static inline void br_record_config_information(struct net_bridge_port *p, > p->designated_cost = bpdu->root_path_cost; > p->designated_bridge = bpdu->bridge_id; > p->designated_port = bpdu->port_id; > - p->designated_age = jiffies + bpdu->message_age; > + p->designated_age = jiffies - bpdu->message_age; > > mod_timer(&p->message_age_timer, jiffies > + (p->br->max_age - bpdu->message_age)); > -- > 1.7.3.4 >
Vitalii Demianets
2012-Mar-01 09:36 UTC
[Bridge] [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
On Tuesday 28 February 2012 20:37:10 Joakim Tjernlund wrote:> min age increment needs to round up its min age tick for all > HZ values to guarantee message age is increasing. > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se> > --- > net/bridge/br_stp.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c > index a04eeb6..9a8aebd 100644 > --- a/net/bridge/br_stp.c > +++ b/net/bridge/br_stp.c > @@ -17,9 +17,9 @@ > #include "br_private_stp.h" > > /* since time values in bpdu are in jiffies and then scaled (1/256) > - * before sending, make sure that is at least one. > + * before sending, make sure that is at least one STP tick. > */ > -#define MESSAGE_AGE_INCR ((HZ < 256) ? 1 : (HZ/256)) > +#define MESSAGE_AGE_INCR ((HZ / 256) + 1) > > static const char *const br_port_state_names[] = { > [BR_STATE_DISABLED] = "disabled",In general I can see the problem which is solved by your patches and it seems to me that solution is adequate. Just a nitpick: I think the following is a little bit more accurate (take into account cases when HZ is multiple of 256, which is rare but possible): -#define MESSAGE_AGE_INCR ((HZ < 256) ? 1 : (HZ/256)) +#define MESSAGE_AGE_INCR ((HZ / 256) + ((HZ % 256) ? 1 : 0)) Also I'd advice to send patches to the netdev list too, people there are much more responsive than at the bridge list. NB: don't you want to try 802.1Q-2005 compatible user-space {m/r}stp implementation instead of rusty in-kernel 802.1D? You can find sources here: http://sourceforge.net/p/mstpd/code/28/tree/ -- Vitalii Demianets