| Message ID | 20201202091356.24075-2-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: Link aggregation support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Tobias Waldekranz <tobias@waldekranz.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. > >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. -J >Signed-off-by: Tobias Waldekranz <tobias@waldekranz.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 e0880a3840d7..d6e1f9cf28d5 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1922,6 +1922,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); >-- >2.17.1 > --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Wed, Dec 02, 2020 at 11:09, Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > Tobias Waldekranz <tobias@waldekranz.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. >> >>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 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.
Tobias Waldekranz <tobias@waldekranz.com> wrote: >On Wed, Dec 02, 2020 at 11:09, Jay Vosburgh <jay.vosburgh@canonical.com> wrote: >> Tobias Waldekranz <tobias@waldekranz.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. >>> >>>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 <jay.vosburgh@canonical.com> -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Wed, Dec 02, 2020 at 16:39, Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > Tobias Waldekranz <tobias@waldekranz.com> wrote: >>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. Makes sense. We (Westermo) have a few customers using static LAGs, so it does happen. That said, LACP is way more common for us as well. > 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). This is exactly the sort of thing I was worried about. Thank you so much for testing it!
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e0880a3840d7..d6e1f9cf28d5 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1922,6 +1922,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);
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. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/bonding/bond_main.c | 2 ++ 1 file changed, 2 insertions(+)