| Message ID | 20201119144508.29468-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. Why are the notifications generated in __netdev_upper_dev_link (via bond_master_upper_dev_link) not sufficient? >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(+) > >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? -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Thu Nov 19, 2020 at 11:18 AM CET, Jay Vosburgh 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. > > 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@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 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.
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);
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(+)