| Message ID | 1330625539-10247-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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",
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(-)