From: Stephen Hemminger <shemminger@vyatta•com>
To: Lennert Buytenhek <buytenh@wantstofly•org>
Cc: netdev@vger•kernel.org, Steven Kath <steven.kath@vyatta•com>,
Anatoly Kaplan <anatoly.kaplan@vyatta•com>,
Arthur Xiong <arthur.xiong@vyatta•com>,
Chris Healy <cphealy@gmail•com>
Subject: Re: stp issue and "bridge: send proper message_age in config BPDU"
Date: Thu, 15 Nov 2012 11:58:53 -0800 [thread overview]
Message-ID: <20121115115853.6b9a3ec3@s6510.linuxnetplumber.net> (raw)
In-Reply-To: <20121115195200.GD730@wantstofly.org>
On Thu, 15 Nov 2012 20:52:00 +0100
Lennert Buytenhek <buytenh@wantstofly•org> wrote:
> Hi!
>
> FWIW, I've been debugging an STP issue on an old product kernel tree
> that I couldn't find an upstream fix for, but after having debugged the
> issue, there does actually appear to be an upstream commit that makes
> the issue go away, but the commit message on that commit is somewhat
> unclear about what the issue is that it's fixing and why the given fix
> fixes it, and given that I spent considerable time debugging it I
> figured I'd send this out for the sake of the next person googling for
> this.
>
> The symptoms are pretty much what's described in this bug:
>
> https://bugzilla.vyatta.com/show_bug.cgi?id=7164
>
> And the upstream commit is:
>
> https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=0c03150e7ea8f7fcd03cfef29385e0010b22ee92
>
> commit 0c03150e7ea8f7fcd03cfef29385e0010b22ee92
> Author: stephen hemminger <shemminger@vyatta•com>
> Date: Fri Jul 22 07:47:06 2011 +0000
>
> bridge: send proper message_age in config BPDU
>
> What I was seeing was that as a non-root bridge, Linux STP would often
> fail to transmit BPDUs out of designated ports upon reception of a BPDU
> from an upstream port.
>
> br_received_config_bpdu() handles the received BPDU, and calls into
> br_record_config_information(), which resets the message age timer on
> this port to jiffies + (p->br->max_age - bpdu->message_age);
>
> When br_received_config_bpdu() then calls br_config_bpdu_generation(),
> the latter will call into br_transmit_config() for each enabled
> designated port, which will send out BPDUs with age br->max_age
> - (root->message_age_timer.expires - jiffies) + MESSAGE_AGE_INCR if
> we're not the root bridge, which if you plug in the previously
> computed timeout simplifies to bpdu->message_age + MESSAGE_AGE_INCR,
> which is exactly what we want it to be and this computation isn't
> wrong per se.
>
> The problem with the above logic, though, is that it fails to
> consider that mod_timer() can round up the timeout you give it (i.e.
> add timer slack), and that reading back root->message_age_timer.expires
> in br_transmit_config() won't necessarily return the value that was
> plugged into mod_timer() for this timer in br_record_config_information().
>
> E.g. if mod_timer() decides to add 5 jiffies to the timeout, the message
> age value that br_transmit_config() will compute will be:
>
> br->max_age - (root->message_age_timer.expires - jiffies) +
> MESSAGE_AGE_INCR
>
> = br->max_age - (jiffies + (p->br->max_age - bpdu->message_age) + 5
> - jiffies) + MESSAGE_AGE_INCR
>
> = br->max_age - (p->br->max_age - bpdu->message_age + 5) +
> MESSAGE_AGE_INCR
>
> = bpdu->message_age - 5 + MESSAGE_AGE_INCR
>
> Which will likely make the computed message age value negative.
> This message age is stored in a signed int, but is then compared
> against the bridge max age time:
>
> if (bpdu.message_age < br->max_age) {
>
> and br->max_age is an unsigned long, causing the comparison to be
> unsigned and always fail if the computed message age was negative,
> and no BPDU to be sent (causing our downstream neighbours to time
> us out after some time and etc).
>
> Commit 0c03150e7ea fixes the issue because it avoids reading back the
> expiration time (possibly with timer slack included) of a previously
> set timer. Disabling timer slack on the message age timer achieves
> the same thing (and is what I did initially):
>
> --- a/net/bridge/br_stp_timer.c
> +++ b/net/bridge/br_stp_timer.c
> @@ -158,6 +158,7 @@ void br_stp_port_timer_init(struct net_bridge_port *p)
> {
> setup_timer(&p->message_age_timer, br_message_age_timer_expired,
> (unsigned long) p);
> + set_timer_slack(&p->message_age_timer, 0);
>
> setup_timer(&p->forward_delay_timer, br_forward_delay_timer_expired,
> (unsigned long) p);
>
>
> thanks,
> Lennert
Disabling timer slack causes additional power consumption because
the tick wakeup has to be immediate. I prefer to handle late timer
in the code.
P.s: not sure if timer slack existed back when I first saw the problem.
next prev parent reply other threads:[~2012-11-15 19:58 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-15 19:52 stp issue and "bridge: send proper message_age in config BPDU" Lennert Buytenhek
2012-11-15 19:58 ` Stephen Hemminger [this message]
2012-11-15 20:09 ` Lennert Buytenhek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121115115853.6b9a3ec3@s6510.linuxnetplumber.net \
--to=shemminger@vyatta$(echo .)com \
--cc=anatoly.kaplan@vyatta$(echo .)com \
--cc=arthur.xiong@vyatta$(echo .)com \
--cc=buytenh@wantstofly$(echo .)org \
--cc=cphealy@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=steven.kath@vyatta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox