[PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Joakim Tjernlund]

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@transmode.se>
---

The bridge list seems very slow so try netdev instead.

 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

[PATCH 2/2] bridge: message age needs to increase, not decrease.

[Joakim Tjernlund]

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@transmode.se>
---

The bridge list seems very slow so try netdev instead.

 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

Re: [PATCH 2/2] bridge: message age needs to increase, not decrease.

[David Miller]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Thu, 1 Mar 2012 19:12:19 +0100

> 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@transmode.se>

Applied.

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[David Miller]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Thu, 1 Mar 2012 19:12:18 +0100

> 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@transmode.se>

Applied.

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Stephen Hemminger]

On Thu, 1 Mar 2012 19:12:18 +0100
Joakim Tjernlund <Joakim.Tjernlund@transmode.se> 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@transmode.se>
> ---
> 
> The bridge list seems very slow so try netdev instead.
> 
>  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",

I don't understand why this is required.
  HZ = 100 then incr = 1
  HZ = 1000 then incr = 3 (your patch would make it for 4).

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 4 Mar 2012 21:03:50 -0800

> I don't understand why this is required.

And I don't understand why it takes nearly a week to review a one line
patch like this.

If you don't say anything I'm going to just apply it after a few days,
which is what I did here already.

Me applying it should never be your trigger to review the patch, yet I
see this happen all the time.  What do you think I was waiting for
these past 5 days, divine intervention?

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Joakim Tjernlund]

David Miller <davem@davemloft.net> wrote on 2012/03/05 06:09:21:
>
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Sun, 4 Mar 2012 21:03:50 -0800
>
> > I don't understand why this is required.
>
> And I don't understand why it takes nearly a week to review a one line
> patch like this.
>
> If you don't say anything I'm going to just apply it after a few days,
> which is what I did here already.
>
> Me applying it should never be your trigger to review the patch, yet I
> see this happen all the time.  What do you think I was waiting for
> these past 5 days, divine intervention?

hmm, does this mean you "unapplied" the patch now?

 Jocke

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[David Miller]

From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Date: Mon, 5 Mar 2012 09:14:00 +0100

> David Miller <davem@davemloft.net> wrote on 2012/03/05 06:09:21:
>>
>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Sun, 4 Mar 2012 21:03:50 -0800
>>
>> > I don't understand why this is required.
>>
>> And I don't understand why it takes nearly a week to review a one line
>> patch like this.
>>
>> If you don't say anything I'm going to just apply it after a few days,
>> which is what I did here already.
>>
>> Me applying it should never be your trigger to review the patch, yet I
>> see this happen all the time.  What do you think I was waiting for
>> these past 5 days, divine intervention?
> 
> hmm, does this mean you "unapplied" the patch now?

I can't, it's now in the permanent record so I can't remove it.

At best I can apply a revert patch, but you guys are still discussing
this so I have no idea whether that's even necessary or not.

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Joakim Tjernlund]

David Miller <davem@davemloft.net> wrote on 2012/03/05 09:24:10:
>
> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Date: Mon, 5 Mar 2012 09:14:00 +0100
>
> > David Miller <davem@davemloft.net> wrote on 2012/03/05 06:09:21:
> >>
> >> From: Stephen Hemminger <shemminger@vyatta.com>
> >> Date: Sun, 4 Mar 2012 21:03:50 -0800
> >>
> >> > I don't understand why this is required.
> >>
> >> And I don't understand why it takes nearly a week to review a one line
> >> patch like this.
> >>
> >> If you don't say anything I'm going to just apply it after a few days,
> >> which is what I did here already.
> >>
> >> Me applying it should never be your trigger to review the patch, yet I
> >> see this happen all the time.  What do you think I was waiting for
> >> these past 5 days, divine intervention?
> >
> > hmm, does this mean you "unapplied" the patch now?
>
> I can't, it's now in the permanent record so I can't remove it.
>
> At best I can apply a revert patch, but you guys are still discussing
> this so I have no idea whether that's even necessary or not.

OK, hopefully Stephen is happy with my explanation. In any case
I won't be able to retest if only patch 2 is needed as our test window closed
this weekend.

 Jocke

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Stephen Hemminger]

On Mon, 5 Mar 2012 09:47:52 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> David Miller <davem@davemloft.net> wrote on 2012/03/05 09:24:10:
> >
> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > Date: Mon, 5 Mar 2012 09:14:00 +0100
> >
> > > David Miller <davem@davemloft.net> wrote on 2012/03/05 06:09:21:
> > >>
> > >> From: Stephen Hemminger <shemminger@vyatta.com>
> > >> Date: Sun, 4 Mar 2012 21:03:50 -0800
> > >>
> > >> > I don't understand why this is required.
> > >>
> > >> And I don't understand why it takes nearly a week to review a one line
> > >> patch like this.
> > >>
> > >> If you don't say anything I'm going to just apply it after a few days,
> > >> which is what I did here already.
> > >>
> > >> Me applying it should never be your trigger to review the patch, yet I
> > >> see this happen all the time.  What do you think I was waiting for
> > >> these past 5 days, divine intervention?
> > >
> > > hmm, does this mean you "unapplied" the patch now?
> >
> > I can't, it's now in the permanent record so I can't remove it.
> >
> > At best I can apply a revert patch, but you guys are still discussing
> > this so I have no idea whether that's even necessary or not.
> 
> OK, hopefully Stephen is happy with my explanation. In any case
> I won't be able to retest if only patch 2 is needed as our test window closed
> this weekend.
> 
>  Jocke
> 

I am okay with the change, but that whole section needs more comments
and explanation. 

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Joakim Tjernlund]

Stephen Hemminger <shemminger@vyatta.com> wrote on 2012/03/05 06:03:50:
>
> On Thu, 1 Mar 2012 19:12:18 +0100
> Joakim Tjernlund <Joakim.Tjernlund@transmode.se> 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@transmode.se>
> > ---
> >
> > The bridge list seems very slow so try netdev instead.
> >
> >  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",
>
> I don't understand why this is required.
>   HZ = 100 then incr = 1
>   HZ = 1000 then incr = 3 (your patch would make it for 4).

Since message age is rounded down, DIV_ROUND_UP(ticks * HZ, STP_HZ), I wanted
to make sure that message age won't decrease or stay the same by adding at least one
STP tick. We did see such age times when we were debugging this. They went away with
both patches applied so there is a chance that only patch 2 is strictly required but
I did not test that combo.

 Jocke

Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256

[Vitalii Demianets]

On Monday 05 March 2012 07:03:50 Stephen Hemminger wrote:
> >
> >  /* 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",
>
> I don't understand why this is required.
>   HZ = 100 then incr = 1
>   HZ = 1000 then incr = 3 (your patch would make it for 4).

And then we have in br_transmit_config():
bpdu.message_age = (jiffies - root->designated_age)
			+ MESSAGE_AGE_INCR;
So in the corner case when jiffies == root->designated_age the message age 
increase only by 3 (when HZ=1000).
Then, in br_set_ticks() we have:
ticks = (STP_HZ * j)/ HZ;
So, again in the corner case, the increase in age is (256*3)/1000 == 0.
That's the case Joakim wanted to avoid.

-- 
Vitalii Demianets