From: Jonathan Toppins <jtoppins@cumulusnetworks•com>
To: Jay Vosburgh <jay.vosburgh@canonical•com>
Cc: netdev@vger•kernel.org, Scott Feldman <sfeldma@gmail•com>,
Andy Gospodarek <gospo@cumulusnetworks•com>,
Veaceslav Falico <vfalico@gmail•com>,
Nikolay Aleksandrov <nikolay@redhat•com>
Subject: Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member
Date: Wed, 21 Jan 2015 00:22:14 -0500 [thread overview]
Message-ID: <54BF3786.9050505@cumulusnetworks.com> (raw)
In-Reply-To: <20956.1421702213@famine>
On 1/19/15 4:16 PM, Jay Vosburgh wrote:
> Jonathan Toppins <jtoppins@cumulusnetworks•com> wrote:
>
>> From: Scott Feldman <sfeldma@cumulusnetworks•com>
>>
>> Bonding driver parameter min_links is now used to signal upper-level
>> protocols of bond status. The way it works is if the total number of
>> active members in slaves drops below min_links, the bond link carrier
>> will go down, signaling upper levels that bond is inactive. When active
>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>> and protocols can resume. When bond is carrier down, member ports are
>> in stp fwd state blocked (rather than normal disabled state), so
>> low-level ctrl protocols (LACP) can still get in and be processed by
>> bonding driver.
>
> Presuming that "stp" is Spanning Tree, is the last sentence
> above actually describing the behavior of a bridge port when a bond is
> the member of the bridge? I'm not sure I understand what "member ports"
> refers to (bridge ports or bonding slaves).
Ack, maybe replacing the last sentence with something like:
When bond is carrier down, the slave ports are only forwarding
low-level control protocols (e.g. LACP PDU) and discarding all other
packets.
>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>> ret = 0;
>> goto out;
>> }
>> +
>> + bond_for_each_slave_rcu(bond, slave, iter)
>> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>> + active_slaves++;
>> +
>> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>> - if (active) {
>> + if (active && __agg_has_partner(active)) {
>
> Why "__agg_has_partner"? Since the "else" of this clause is:
>
> } else if (netif_carrier_ok(bond->dev)) {
> netif_carrier_off(bond->dev);
> }
>
> I'm wondering if this will do the right thing for the case that
> there are no LACP partners at all (e.g., the switch ports do not have
> LACP enabled), in which case the active aggregator should be a single
> "individual" port as a fallback, but will not have a partner.
>
> -J
>
I see your point. The initial thinking was the logical bond carrier
should not be brought up until the bond has a partner and is ready to
pass traffic, otherwise we start blackholing frames. Looking over the
code it seems the aggregator.is_individual flag is only set to true when
a slave is in half-duplex, this seems odd?
My initial thinking to alleviate the concern is something like the
following:
if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
__agg_has_partner(active)) {
/* set carrier based on min_links */
} else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
/* set bond carrier state according to carrier state of slave */
} else if (netif_carrier_ok(bond->dev)) {
netif_carrier_off(bond->dev);
}
Maybe I am missing something and there is a simpler option.
Thinking about how to validate this, it seems having a bond with two
slaves and both slaves in half-duplex will force an aggregator that is
individual to be selected.
Thoughts?
-Jon
next prev parent reply other threads:[~2015-01-21 5:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-16 15:57 [PATCH net-next 0/5] bonding: various 802.3ad fixes Jonathan Toppins
2015-01-16 15:57 ` [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member Jonathan Toppins
2015-01-19 19:27 ` Nikolay Aleksandrov
2015-01-19 20:54 ` Jonathan Toppins
2015-01-19 21:16 ` Jay Vosburgh
2015-01-21 5:22 ` Jonathan Toppins [this message]
2015-01-21 7:14 ` Jay Vosburgh
2015-01-23 23:04 ` Jonathan Toppins
2015-01-24 3:15 ` Jay Vosburgh
[not found] ` <CAE4R7bBzeO5MvL5zS5Piq6Ld2ZbD8smGx8XaJy5SvY7kHbX_Kw@mail.gmail.com>
2015-01-21 5:27 ` Jonathan Toppins
2015-01-16 15:57 ` [PATCH net-next 2/5] bonding: fix bond_open() don't always set slave active flag Jonathan Toppins
2015-01-16 15:57 ` [PATCH net-next 3/5] bonding: fix incorrect lacp mux state when agg not active Jonathan Toppins
2015-01-19 19:26 ` Nikolay Aleksandrov
2015-01-19 20:50 ` Jonathan Toppins
2015-01-19 20:56 ` David Miller
2015-01-16 15:57 ` [PATCH net-next 4/5] bonding: fix LACP PDU not sent on slave port sometimes Jonathan Toppins
2015-01-16 15:57 ` [PATCH net-next 5/5] bonding: cleanup and remove dead code Jonathan Toppins
2015-01-19 19:29 ` [PATCH net-next 0/5] bonding: various 802.3ad fixes Nikolay Aleksandrov
2015-01-19 20:39 ` Jonathan Toppins
2015-01-19 20:19 ` David Miller
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=54BF3786.9050505@cumulusnetworks.com \
--to=jtoppins@cumulusnetworks$(echo .)com \
--cc=gospo@cumulusnetworks$(echo .)com \
--cc=jay.vosburgh@canonical$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@redhat$(echo .)com \
--cc=sfeldma@gmail$(echo .)com \
--cc=vfalico@gmail$(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