Date:   Thu, 19 Nov 2020 22:20:34 +0100
From:   "Tobias Waldekranz" <tobias@...dekranz.com>
To:     "Jay Vosburgh" <jay.vosburgh@...onical.com>
Cc:     <davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
        <vivien.didelot@...il.com>, <f.fainelli@...il.com>,
        <olteanv@...il.com>, <vfalico@...il.com>, <andy@...yhouse.net>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/4] net: bonding: Notify ports about their
 initial state

On Thu Nov 19, 2020 at 11:18 AM CET, Jay Vosburgh wrote:
> Tobias Waldekranz <tobias@...dekranz.com> wrote:
>
> >When creating a static bond (e.g. balance-xor), all ports will always
> >be enabled. This is set, and the corresponding notification is sent
> >out, before the port is linked to the bond upper.
> >
> >In the offloaded case, this ordering is hard to deal with.
> >
> >The lower will first see a notification that it can not associate with
> >any bond. Then the bond is joined. After that point no more
> >notifications are sent, so all ports remain disabled.
>
> Why are the notifications generated in __netdev_upper_dev_link
> (via bond_master_upper_dev_link) not sufficient?

That notification only lets the switchdev driver know that the port is
now a part of the bond; it says nothing about if the port is in the
active transmit set or not.

That notification actually is sent. Unfortunately this happens before
the event your referencing, in the `switch (BOND_MODE(bond))` section
just a few lines above. Essentially the conversation goes like this:

bond0: swp0 has link and is in the active tx set.
dsa:   Cool, but swp0 is not a part of any LAG afaik; ignore.
bond0: swp0 is now a part of bond0.
dsa:   OK, I'll set up the hardware, setting swp0 as inactive initially.

This change just repeats the initial message at the end when the
driver can make sense of it. Without it, modes that default ports to
be inactive (e.g. LACP) still work, as the driver and the bond agree
on the initial state in those cases. But for a static LAG, there will
never be another event (until the link fails).

> >This change simply sends an extra notification once the port has been
> >linked to the upper to synchronize the initial state.
> >
> >Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> >---
> > drivers/net/bonding/bond_main.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 71c9677d135f..80c164198dcf 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1897,6 +1897,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > 		goto err_unregister;
> > 	}
> > 
> >+	bond_lower_state_changed(new_slave);
> >+
> > 	res = bond_sysfs_slave_add(new_slave);
> > 	if (res) {
> > 		slave_dbg(bond_dev, slave_dev, "Error %d calling bond_sysfs_slave_add\n", res);
>
> Would it be better to add this call further down, after all
> possible failures have been checked? I.e., if this new call to
> bond_lower_state_changed() completes, and then very soon afterwards the
> upper is unlinked, could that cause any issues?

All the work of configuring the LAG offload is done in the
notification send out by netdev_upper_dev_link. So that will all have
to be torn down in that case no matter where we place this call.

So from the DSA/switchdev point-of-view, I would say no, and I believe
these are the only consumers of the events.

Additionally, I think it makes sense to place the call as early as
possible as that means you have a smaller window of time where the
bond and the switchdev driver may disagree on the port's state.

Powered by blists - more mailing lists