Tobias Waldekranz [off-list ref] wrote:

On Wed, Dec 02, 2020 at 11:09, Jay Vosburgh [off-list ref] wrote:

quoted

Tobias Waldekranz [off-list ref] wrote:

quoted

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.

This change simply sends an extra notification once the port has been
linked to the upper to synchronize the initial state.

	I'm not objecting to this per se, but looking at team and
net_failover (failover_slave_register), those drivers do not send the
same first notification that bonding does (the "can not associate" one),
but only send a notification after netdev_master_upper_dev_link is
complete.

	Does it therefore make more sense to move the existing
notification within bonding to take place after the upper_dev_link
(where you're adding this new call to bond_lower_state_changed)?  If the
existing notification is effectively useless, this would make the
sequence of notifications consistent across drivers.

From my point of view that makes more sense. I just assumed that the
current implementation was done this way for a reason. Therefore I opted
for a simple extension instead.

	I suspect the current implementation's ordering is more a side
effect of how the function was structured initially, and the
notifications were added later without giving thought to the ordering of
those events.

I could look at hoisting up the linking op before the first
notification. My main concern is that this is a new subsystem to me, so
I am not sure how to determine the adequate test coverage for a change
like this.

Another option would be to drop this change from this series and do it
separately. It would be nice to have both team and bond working though.

Not sure why I am the first to run into this. Presumably the mlxsw LAG
offloading would be affected in the same way. Maybe their main use-case
is LACP.

	I'm not sure about mlxsw specifically, but in the configurations
I see, LACP is by far the most commonly used mode, with active-backup a
distant second.  I can't recall the last time I saw a production
environment using balance-xor.

	I think that in the perfect world there should be exactly one
such notification, and occurring in the proper sequence.  A quick look
at the kernel consumers of the NETDEV_CHANGELOWERSTATE event (mlx5,
mlxsw, and nfp, looks like) suggests that those shouldn't have an issue.

	In user space, however, there are daemons that watch the events,
and may rely on the current ordering.  Some poking around reveals odd
bugs in user space when events are rearranged, so I think the prudent
thing is to not mess with what's there now, and just add the one event
here (i.e., apply your patch as-is).

	So, for this bonding change:

Acked-by: Jay Vosburgh <redacted>

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com