* [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
@ 2012-03-01 18:12 Joakim Tjernlund
2012-03-01 18:12 ` [PATCH 2/2] bridge: message age needs to increase, not decrease Joakim Tjernlund
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Joakim Tjernlund @ 2012-03-01 18:12 UTC (permalink / raw)
To: netdev; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] bridge: message age needs to increase, not decrease.
2012-03-01 18:12 [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 Joakim Tjernlund
@ 2012-03-01 18:12 ` Joakim Tjernlund
2012-03-05 2:57 ` David Miller
2012-03-05 2:57 ` [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 David Miller
2012-03-05 5:03 ` Stephen Hemminger
2 siblings, 1 reply; 12+ messages in thread
From: Joakim Tjernlund @ 2012-03-01 18:12 UTC (permalink / raw)
To: netdev; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-01 18:12 [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 Joakim Tjernlund
2012-03-01 18:12 ` [PATCH 2/2] bridge: message age needs to increase, not decrease Joakim Tjernlund
@ 2012-03-05 2:57 ` David Miller
2012-03-05 5:03 ` Stephen Hemminger
2 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-03-05 2:57 UTC (permalink / raw)
To: Joakim.Tjernlund; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bridge: message age needs to increase, not decrease.
2012-03-01 18:12 ` [PATCH 2/2] bridge: message age needs to increase, not decrease Joakim Tjernlund
@ 2012-03-05 2:57 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-03-05 2:57 UTC (permalink / raw)
To: Joakim.Tjernlund; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-01 18:12 [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 Joakim Tjernlund
2012-03-01 18:12 ` [PATCH 2/2] bridge: message age needs to increase, not decrease Joakim Tjernlund
2012-03-05 2:57 ` [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 David Miller
@ 2012-03-05 5:03 ` Stephen Hemminger
2012-03-05 5:09 ` David Miller
` (2 more replies)
2 siblings, 3 replies; 12+ messages in thread
From: Stephen Hemminger @ 2012-03-05 5:03 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: netdev
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).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 5:03 ` Stephen Hemminger
@ 2012-03-05 5:09 ` David Miller
2012-03-05 8:14 ` Joakim Tjernlund
2012-03-05 8:09 ` Joakim Tjernlund
2012-03-05 10:15 ` Vitalii Demianets
2 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2012-03-05 5:09 UTC (permalink / raw)
To: shemminger; +Cc: Joakim.Tjernlund, netdev
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 5:03 ` Stephen Hemminger
2012-03-05 5:09 ` David Miller
@ 2012-03-05 8:09 ` Joakim Tjernlund
2012-03-05 10:15 ` Vitalii Demianets
2 siblings, 0 replies; 12+ messages in thread
From: Joakim Tjernlund @ 2012-03-05 8:09 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 5:09 ` David Miller
@ 2012-03-05 8:14 ` Joakim Tjernlund
2012-03-05 8:24 ` David Miller
0 siblings, 1 reply; 12+ messages in thread
From: Joakim Tjernlund @ 2012-03-05 8:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 8:14 ` Joakim Tjernlund
@ 2012-03-05 8:24 ` David Miller
2012-03-05 8:47 ` Joakim Tjernlund
0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2012-03-05 8:24 UTC (permalink / raw)
To: joakim.tjernlund; +Cc: netdev, shemminger
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 8:24 ` David Miller
@ 2012-03-05 8:47 ` Joakim Tjernlund
2012-03-05 21:30 ` Stephen Hemminger
0 siblings, 1 reply; 12+ messages in thread
From: Joakim Tjernlund @ 2012-03-05 8:47 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 5:03 ` Stephen Hemminger
2012-03-05 5:09 ` David Miller
2012-03-05 8:09 ` Joakim Tjernlund
@ 2012-03-05 10:15 ` Vitalii Demianets
2 siblings, 0 replies; 12+ messages in thread
From: Vitalii Demianets @ 2012-03-05 10:15 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Joakim Tjernlund, netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bridge: Adjust min age inc for HZ > 256
2012-03-05 8:47 ` Joakim Tjernlund
@ 2012-03-05 21:30 ` Stephen Hemminger
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2012-03-05 21:30 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-03-05 21:30 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-01 18:12 [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 Joakim Tjernlund
2012-03-01 18:12 ` [PATCH 2/2] bridge: message age needs to increase, not decrease Joakim Tjernlund
2012-03-05 2:57 ` David Miller
2012-03-05 2:57 ` [PATCH 1/2] bridge: Adjust min age inc for HZ > 256 David Miller
2012-03-05 5:03 ` Stephen Hemminger
2012-03-05 5:09 ` David Miller
2012-03-05 8:14 ` Joakim Tjernlund
2012-03-05 8:24 ` David Miller
2012-03-05 8:47 ` Joakim Tjernlund
2012-03-05 21:30 ` Stephen Hemminger
2012-03-05 8:09 ` Joakim Tjernlund
2012-03-05 10:15 ` Vitalii Demianets
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).