| Message ID | 1499268990-19251-6-git-send-email-arkadis@mellanox.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote: > Add support for learning FDB through notification. The driver defers > the hardware update via ordered work queue. In case of a successful > FDB add a notification is sent back to bridge. > > Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> > --- > net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 122 insertions(+), 2 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 19395cc..d0592f2 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, > return NOTIFY_DONE; > } > > +struct dsa_switchdev_event_work { > + struct work_struct work; > + struct switchdev_notifier_fdb_info fdb_info; > + struct net_device *dev; > + unsigned long event; > +}; > + > +static void dsa_switchdev_event_work(struct work_struct *work) > +{ > + struct dsa_switchdev_event_work *switchdev_work = > + container_of(work, struct dsa_switchdev_event_work, work); > + struct net_device *dev = switchdev_work->dev; > + struct switchdev_notifier_fdb_info *fdb_info; > + struct dsa_slave_priv *p = netdev_priv(dev); > + int err; > + > + rtnl_lock(); > + switch (switchdev_work->event) { > + case SWITCHDEV_FDB_ADD_TO_DEVICE: > + fdb_info = &switchdev_work->fdb_info; You could probably move this assignment outside of the switch()/case statement since it is repeated > + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); > + if (err) { > + netdev_dbg(dev, "fdb add failed err=%d\n", err); > + break; > + } > + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, > + &fdb_info->info); > + break; > + > + case SWITCHDEV_FDB_DEL_TO_DEVICE: > + fdb_info = &switchdev_work->fdb_info; > + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); > + if (err) > + netdev_dbg(dev, "fdb del failed err=%d\n", err); OK I must have missed from the off-list discussion why we are not calling the switchdev notifier here? > + break; > + } > + rtnl_unlock(); > + > + kfree(switchdev_work->fdb_info.addr); > + kfree(switchdev_work); > + dev_put(dev); > +} > + > +static int > +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work * > + switchdev_work, > + const struct switchdev_notifier_fdb_info * > + fdb_info) > +{ > + memcpy(&switchdev_work->fdb_info, fdb_info, > + sizeof(switchdev_work->fdb_info)); > + switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC); > + if (!switchdev_work->fdb_info.addr) > + return -ENOMEM; Can't we have the fdb_info structure contain pre-allocated 6 bytes storage for address, it's really silly to do this here and every time we need to offload an FDB entry. > + ether_addr_copy((u8 *)switchdev_work->fdb_info.addr, > + fdb_info->addr); > + return 0; > +} > + > +/* Called under rcu_read_lock() */ > +static int dsa_slave_switchdev_event(struct notifier_block *unused, > + unsigned long event, void *ptr) > +{ > + struct net_device *dev = switchdev_notifier_info_to_dev(ptr); > + struct dsa_switchdev_event_work *switchdev_work; > + > + if (!dsa_slave_dev_check(dev)) > + return NOTIFY_DONE; > + > + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); > + if (!switchdev_work) > + return NOTIFY_BAD; Same here, can we try to re-use a heap allocated workqueue item and we just re-initialize it when we have to? > + > + INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work); > + switchdev_work->dev = dev; > + switchdev_work->event = event; > + > + switch (event) { > + case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */ > + case SWITCHDEV_FDB_DEL_TO_DEVICE: > + if (dsa_slave_switchdev_fdb_work_init(switchdev_work, > + ptr)) > + goto err_fdb_work_init; > + dev_hold(dev); > + break; > + default: > + kfree(switchdev_work); > + return NOTIFY_DONE; > + } > + > + dsa_schedule_work(&switchdev_work->work); > + return NOTIFY_OK; > + > +err_fdb_work_init: > + kfree(switchdev_work); > + return NOTIFY_BAD; > +} > + > static struct notifier_block dsa_slave_nb __read_mostly = { > - .notifier_call = dsa_slave_netdevice_event, > + .notifier_call = dsa_slave_netdevice_event, > +}; > + > +static struct notifier_block dsa_slave_switchdev_notifier = { > + .notifier_call = dsa_slave_switchdev_event, > }; > > int dsa_slave_register_notifier(void) > { > - return register_netdevice_notifier(&dsa_slave_nb); > + int err; > + > + err = register_netdevice_notifier(&dsa_slave_nb); > + if (err) > + return err; > + > + err = register_switchdev_notifier(&dsa_slave_switchdev_notifier); > + if (err) > + goto err_register_switchdev; this label is a bit misnamed since it really deals with the slave dsa notifier, how about no label or just renaming it "error_slave_nb"? > + > + return 0; > + > +err_register_switchdev: > + unregister_netdevice_notifier(&dsa_slave_nb); > + return err; > } > > void dsa_slave_unregister_notifier(void) > { > int err; > > + err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier); > + if (err) > + pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err); > + > err = unregister_netdevice_notifier(&dsa_slave_nb); > if (err) > pr_err("DSA: failed to unregister slave notifier (%d)\n", err); >
On 07/05/2017 10:35 PM, Florian Fainelli wrote: > On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote: >> Add support for learning FDB through notification. The driver defers >> the hardware update via ordered work queue. In case of a successful >> FDB add a notification is sent back to bridge. >> >> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> >> --- >> net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 122 insertions(+), 2 deletions(-) >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 19395cc..d0592f2 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, >> return NOTIFY_DONE; >> } >> >> +struct dsa_switchdev_event_work { >> + struct work_struct work; >> + struct switchdev_notifier_fdb_info fdb_info; >> + struct net_device *dev; >> + unsigned long event; >> +}; >> + >> +static void dsa_switchdev_event_work(struct work_struct *work) >> +{ >> + struct dsa_switchdev_event_work *switchdev_work = >> + container_of(work, struct dsa_switchdev_event_work, work); >> + struct net_device *dev = switchdev_work->dev; >> + struct switchdev_notifier_fdb_info *fdb_info; >> + struct dsa_slave_priv *p = netdev_priv(dev); >> + int err; >> + >> + rtnl_lock(); >> + switch (switchdev_work->event) { >> + case SWITCHDEV_FDB_ADD_TO_DEVICE: >> + fdb_info = &switchdev_work->fdb_info; > > You could probably move this assignment outside of the switch()/case > statement since it is repeated > Yeah, but this cast is only correct for only this two cases. If in the future it will be extended it will be weird getting it back inside the switch. Thanks for the review! >> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >> + if (err) { >> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >> + break; >> + } >> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >> + &fdb_info->info); >> + break; >> + >> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >> + fdb_info = &switchdev_work->fdb_info; >> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >> + if (err) >> + netdev_dbg(dev, "fdb del failed err=%d\n", err); > > OK I must have missed from the off-list discussion why we are not > calling the switchdev notifier here? > We do not agree on it actually, that is why it was moved to the list. I think that delete should succeed, you should retry until succession. The deletion is done under spinlock in the bridge so you cannot block, thus delete cannot fail due to hardware failure. Calling it here doesn't make sense because the bridge probably already deleted this FDB. >> + break; >> + } >> + rtnl_unlock(); >> + >> + kfree(switchdev_work->fdb_info.addr); >> + kfree(switchdev_work); >> + dev_put(dev); >> +} >> + >> +static int >> +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work * >> + switchdev_work, >> + const struct switchdev_notifier_fdb_info * >> + fdb_info) >> +{ >> + memcpy(&switchdev_work->fdb_info, fdb_info, >> + sizeof(switchdev_work->fdb_info)); >> + switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC); >> + if (!switchdev_work->fdb_info.addr) >> + return -ENOMEM; > > Can't we have the fdb_info structure contain pre-allocated 6 bytes > storage for address, it's really silly to do this here and every time we > need to offload an FDB entry. > This fdb_info contains pointer to MAC which resides in the bridge's internal fdb struct (struct net_bridge_fdb_entry). It has to be copied at least once due to the async processing in the workqeue. Note that no reference is taken on this struct, and while you process (async) the add this struct can be already deleted followed by a del notification. In case the fdb_info contains this 6 bytes, 2 copies would be performed: 1. Inside the bridge: from the net_bridge_fdb_entry to the fdb_info (which is allocated on the stack and used only during the notification call) 2. Inside the driver: duplication of the fdb_info to the workqeue item The current way only does one copy, so I think its better. We can do a little optimization here by having the dsa_switchdev_event_work contain the address itself instead of ptr to fdb_info. That way only one allocation of the dsa_switchdev_event_work is done, but I think its not that crucial. >> + ether_addr_copy((u8 *)switchdev_work->fdb_info.addr, >> + fdb_info->addr); >> + return 0; >> +} >> + >> +/* Called under rcu_read_lock() */ >> +static int dsa_slave_switchdev_event(struct notifier_block *unused, >> + unsigned long event, void *ptr) >> +{ >> + struct net_device *dev = switchdev_notifier_info_to_dev(ptr); >> + struct dsa_switchdev_event_work *switchdev_work; >> + >> + if (!dsa_slave_dev_check(dev)) >> + return NOTIFY_DONE; >> + >> + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); >> + if (!switchdev_work) >> + return NOTIFY_BAD; > > Same here, can we try to re-use a heap allocated workqueue item and we > just re-initialize it when we have to? > It cannot be the same one. Because if user added multiple FDBs and the kernel thread which does the work was preempted this work should be accumulated in the queue. Maybe I misunderstood the comment.. >> + >> + INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work); >> + switchdev_work->dev = dev; >> + switchdev_work->event = event; >> + >> + switch (event) { >> + case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */ >> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >> + if (dsa_slave_switchdev_fdb_work_init(switchdev_work, >> + ptr)) >> + goto err_fdb_work_init; >> + dev_hold(dev); >> + break; >> + default: >> + kfree(switchdev_work); >> + return NOTIFY_DONE; >> + } >> + >> + dsa_schedule_work(&switchdev_work->work); >> + return NOTIFY_OK; >> + >> +err_fdb_work_init: >> + kfree(switchdev_work); >> + return NOTIFY_BAD; >> +} >> + >> static struct notifier_block dsa_slave_nb __read_mostly = { >> - .notifier_call = dsa_slave_netdevice_event, >> + .notifier_call = dsa_slave_netdevice_event, >> +}; >> + >> +static struct notifier_block dsa_slave_switchdev_notifier = { >> + .notifier_call = dsa_slave_switchdev_event, >> }; >> >> int dsa_slave_register_notifier(void) >> { >> - return register_netdevice_notifier(&dsa_slave_nb); >> + int err; >> + >> + err = register_netdevice_notifier(&dsa_slave_nb); >> + if (err) >> + return err; >> + >> + err = register_switchdev_notifier(&dsa_slave_switchdev_notifier); >> + if (err) >> + goto err_register_switchdev; > > this label is a bit misnamed since it really deals with the slave dsa > notifier, how about no label or just renaming it "error_slave_nb"? > You have two nb here so its misnamed too.. Maybe err_switchdev_nb? >> + >> + return 0; >> + >> +err_register_switchdev: >> + unregister_netdevice_notifier(&dsa_slave_nb); >> + return err; >> } >> >> void dsa_slave_unregister_notifier(void) >> { >> int err; >> >> + err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier); >> + if (err) >> + pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err); >> + >> err = unregister_netdevice_notifier(&dsa_slave_nb); >> if (err) >> pr_err("DSA: failed to unregister slave notifier (%d)\n", err); >> > >
Hi Arkadi, Arkadi Sharshevsky <arkadis@mellanox.com> writes: >>> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >>> + if (err) { >>> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >>> + break; >>> + } >>> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >>> + &fdb_info->info); >>> + break; >>> + >>> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >>> + fdb_info = &switchdev_work->fdb_info; >>> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >>> + if (err) >>> + netdev_dbg(dev, "fdb del failed err=%d\n", err); >> >> OK I must have missed from the off-list discussion why we are not >> calling the switchdev notifier here? > > We do not agree on it actually, that is why it was moved to the list. > I think that delete should succeed, you should retry until succession. > > The deletion is done under spinlock in the bridge so you cannot block, > thus delete cannot fail due to hardware failure. Calling it here doesn't > make sense because the bridge probably already deleted this FDB. So as we discussed, the problem here is that if dsa_port_fdb_del fails for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux bridge will delete the entry in software, dumping bridge fdb will show nothing, but the entry would still be programmed in hardware and the network can thus be inconsistent, unsupposedly switching frames. IMHO the correct way for bridge to use the notification chain is to make SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE: if an entry has been marked as offloaded, bridge must mark the entry as to-be-deleted and do not delete the software entry until the driver notifies back the successful deletion. If that is hardly feasible due to some bridge limitations, we must explain this in a comment and use something more explosive than a simple netdev_dbg to warn the user about the broken network setup... Thanks, Vivien
On 07/10/2017 11:59 PM, Vivien Didelot wrote: > Hi Arkadi, > > Arkadi Sharshevsky <arkadis@mellanox.com> writes: > >>>> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >>>> + if (err) { >>>> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >>>> + break; >>>> + } >>>> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >>>> + &fdb_info->info); >>>> + break; >>>> + >>>> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >>>> + fdb_info = &switchdev_work->fdb_info; >>>> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >>>> + if (err) >>>> + netdev_dbg(dev, "fdb del failed err=%d\n", err); >>> >>> OK I must have missed from the off-list discussion why we are not >>> calling the switchdev notifier here? >> >> We do not agree on it actually, that is why it was moved to the list. >> I think that delete should succeed, you should retry until succession. >> >> The deletion is done under spinlock in the bridge so you cannot block, >> thus delete cannot fail due to hardware failure. Calling it here doesn't >> make sense because the bridge probably already deleted this FDB. > > So as we discussed, the problem here is that if dsa_port_fdb_del fails > for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux > bridge will delete the entry in software, dumping bridge fdb will show > nothing, but the entry would still be programmed in hardware and the > network can thus be inconsistent, unsupposedly switching frames. > > IMHO the correct way for bridge to use the notification chain is to make > SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE: > if an entry has been marked as offloaded, bridge must mark the entry as > to-be-deleted and do not delete the software entry until the driver > notifies back the successful deletion. > > If that is hardly feasible due to some bridge limitations, we must > explain this in a comment and use something more explosive than a simple > netdev_dbg to warn the user about the broken network setup... > > > Thanks, > > Vivien > Hi Nikolay, Vivien raised inconsistency issue with the current switchdev notification chain in case of FDB del. In case of static FDB delete, notification will be sent to the driver, followed by deletion of the software entry without waiting for the hardware delete. In case of hardware deletion failure the consecutive FDB dump will not show the deleted entry, yet, the entry will stay in hardware. The deletion is done under lock thus the hardware deletion is deferred, and cannot fail due to hardware removal failure. Thus the above proposed solution by Vivien can lead to confusing situation: 1. User deletes the entry 2. Deletion succeed 3. User dumps FDB and still sees this entry due to hardware failure, what should he do? retry to delete until the FDB dump will not show the entry? Would like to hear you opinion about this solution. IMHO in this case the driver should retry to delete, in case of several retries the driver should maybe: 1. Trap the traffic to CPU (dint think it possible in case of DSA). 2. Disable the port (its more explosive then netdev_dbg). Thanks, Arkadi
On 11/07/17 13:26, Arkadi Sharshevsky wrote: > > > On 07/10/2017 11:59 PM, Vivien Didelot wrote: >> Hi Arkadi, >> >> Arkadi Sharshevsky <arkadis@mellanox.com> writes: >> >>>>> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >>>>> + if (err) { >>>>> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >>>>> + break; >>>>> + } >>>>> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >>>>> + &fdb_info->info); >>>>> + break; >>>>> + >>>>> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >>>>> + fdb_info = &switchdev_work->fdb_info; >>>>> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >>>>> + if (err) >>>>> + netdev_dbg(dev, "fdb del failed err=%d\n", err); >>>> >>>> OK I must have missed from the off-list discussion why we are not >>>> calling the switchdev notifier here? >>> >>> We do not agree on it actually, that is why it was moved to the list. >>> I think that delete should succeed, you should retry until succession. >>> >>> The deletion is done under spinlock in the bridge so you cannot block, >>> thus delete cannot fail due to hardware failure. Calling it here doesn't >>> make sense because the bridge probably already deleted this FDB. >> >> So as we discussed, the problem here is that if dsa_port_fdb_del fails >> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux >> bridge will delete the entry in software, dumping bridge fdb will show >> nothing, but the entry would still be programmed in hardware and the >> network can thus be inconsistent, unsupposedly switching frames. >> >> IMHO the correct way for bridge to use the notification chain is to make >> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE: >> if an entry has been marked as offloaded, bridge must mark the entry as >> to-be-deleted and do not delete the software entry until the driver >> notifies back the successful deletion. >> >> If that is hardly feasible due to some bridge limitations, we must >> explain this in a comment and use something more explosive than a simple >> netdev_dbg to warn the user about the broken network setup... >> >> >> Thanks, >> >> Vivien >> > > Hi Nikolay, > > Vivien raised inconsistency issue with the current switchdev > notification chain in case of FDB del. In case of static FDB delete, > notification will be sent to the driver, followed by deletion of the > software entry without waiting for the hardware delete. In case of > hardware deletion failure the consecutive FDB dump will not show the > deleted entry, yet, the entry will stay in hardware. > > The deletion is done under lock thus the hardware deletion is deferred, > and cannot fail due to hardware removal failure. Thus the above proposed > solution by Vivien can lead to confusing situation: > > 1. User deletes the entry > 2. Deletion succeed > 3. User dumps FDB and still sees this entry due to hardware failure, > what should he do? retry to delete until the FDB dump will not show > the entry? > > Would like to hear you opinion about this solution. > > IMHO in this case the driver should retry to delete, in case of > several retries the driver should maybe: > 1. Trap the traffic to CPU (dint think it possible in case of DSA). > 2. Disable the port (its more explosive then netdev_dbg). > > Thanks, > Arkadi > > Hi, Looking at the code - it would seem that retrying is the only current option with the way these switchdev notifications are handled. They cannot fail, meaning from the bridge POV these ops must always succeed and errors are ignored, so the driver should do everything possible to stay in sync, and in case all fails then disabling the port seems like the best option to me, to show that something is clearly wrong and avoid further issues, but DSA maintainers can comment more on how to handle failure. That being said: This sounds a lot like the switchdev notifications vs callbacks discussions that we've had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock is the main problem let's work towards improving that and making the fdb code handle switchdev similar to the vlan code. Cheers, Nik
On 07/11/2017 06:05 PM, Nikolay Aleksandrov wrote: > On 11/07/17 13:26, Arkadi Sharshevsky wrote: >> >> >> On 07/10/2017 11:59 PM, Vivien Didelot wrote: >>> Hi Arkadi, >>> >>> Arkadi Sharshevsky <arkadis@mellanox.com> writes: >>> >>>>>> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >>>>>> + if (err) { >>>>>> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >>>>>> + break; >>>>>> + } >>>>>> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >>>>>> + &fdb_info->info); >>>>>> + break; >>>>>> + >>>>>> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >>>>>> + fdb_info = &switchdev_work->fdb_info; >>>>>> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >>>>>> + if (err) >>>>>> + netdev_dbg(dev, "fdb del failed err=%d\n", err); >>>>> >>>>> OK I must have missed from the off-list discussion why we are not >>>>> calling the switchdev notifier here? >>>> >>>> We do not agree on it actually, that is why it was moved to the list. >>>> I think that delete should succeed, you should retry until succession. >>>> >>>> The deletion is done under spinlock in the bridge so you cannot block, >>>> thus delete cannot fail due to hardware failure. Calling it here doesn't >>>> make sense because the bridge probably already deleted this FDB. >>> >>> So as we discussed, the problem here is that if dsa_port_fdb_del fails >>> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux >>> bridge will delete the entry in software, dumping bridge fdb will show >>> nothing, but the entry would still be programmed in hardware and the >>> network can thus be inconsistent, unsupposedly switching frames. >>> >>> IMHO the correct way for bridge to use the notification chain is to make >>> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE: >>> if an entry has been marked as offloaded, bridge must mark the entry as >>> to-be-deleted and do not delete the software entry until the driver >>> notifies back the successful deletion. >>> >>> If that is hardly feasible due to some bridge limitations, we must >>> explain this in a comment and use something more explosive than a simple >>> netdev_dbg to warn the user about the broken network setup... >>> >>> >>> Thanks, >>> >>> Vivien >>> >> >> Hi Nikolay, >> >> Vivien raised inconsistency issue with the current switchdev >> notification chain in case of FDB del. In case of static FDB delete, >> notification will be sent to the driver, followed by deletion of the >> software entry without waiting for the hardware delete. In case of >> hardware deletion failure the consecutive FDB dump will not show the >> deleted entry, yet, the entry will stay in hardware. >> >> The deletion is done under lock thus the hardware deletion is deferred, >> and cannot fail due to hardware removal failure. Thus the above proposed >> solution by Vivien can lead to confusing situation: >> >> 1. User deletes the entry >> 2. Deletion succeed >> 3. User dumps FDB and still sees this entry due to hardware failure, >> what should he do? retry to delete until the FDB dump will not show >> the entry? >> >> Would like to hear you opinion about this solution. >> >> IMHO in this case the driver should retry to delete, in case of >> several retries the driver should maybe: >> 1. Trap the traffic to CPU (dint think it possible in case of DSA). >> 2. Disable the port (its more explosive then netdev_dbg). >> >> Thanks, >> Arkadi >> >> > > Hi, > Looking at the code - it would seem that retrying is the only current option > with the way these switchdev notifications are handled. They cannot fail, meaning > from the bridge POV these ops must always succeed and errors are ignored, so the > driver should do everything possible to stay in sync, and in case all fails > then disabling the port seems like the best option to me, to show that something is > clearly wrong and avoid further issues, but DSA maintainers can comment more > on how to handle failure. > > That being said: > This sounds a lot like the switchdev notifications vs callbacks discussions that we've > had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock > is the main problem let's work towards improving that and making the fdb code handle > switchdev similar to the vlan code. > > Cheers, > Nik > Vlans can be only added by user under rtnl so its possible to sleep. On the other hand FDBs can be learned in atomic context, thus the notification chain is atomic. One Possible way I thought about doing it is to maintain bridge internal ordered workqueue for the FDBs learned in atomic context, furthermore the FDB table will be protected by mutex. The STATIC entries are added in user context so the user will add directly to this hash table and could sleep. Its not very good, but I don't think about another solution here. Regarding notification or not, drivers should know about FDBs that are not directly related to them like in case of vxlan (vxlan device is not upper device of some siwthcdev port) so notification is better approach in my opinion. Regarding prepare/commit, it is not used in mlxsw. And we would like to remove it from DSA (in case of FDBs). The prepare phase cannot assure success because errors could happen unrelated to reserved space in the switch in the commit stage.. Thanks, Arkadi > > > > > > > >
On 12/07/17 14:23, Arkadi Sharshevsky wrote: > > > On 07/11/2017 06:05 PM, Nikolay Aleksandrov wrote: >> On 11/07/17 13:26, Arkadi Sharshevsky wrote: >>> >>> >>> On 07/10/2017 11:59 PM, Vivien Didelot wrote: >>>> Hi Arkadi, >>>> >>>> Arkadi Sharshevsky <arkadis@mellanox.com> writes: >>>> >>>>>>> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); >>>>>>> + if (err) { >>>>>>> + netdev_dbg(dev, "fdb add failed err=%d\n", err); >>>>>>> + break; >>>>>>> + } >>>>>>> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, >>>>>>> + &fdb_info->info); >>>>>>> + break; >>>>>>> + >>>>>>> + case SWITCHDEV_FDB_DEL_TO_DEVICE: >>>>>>> + fdb_info = &switchdev_work->fdb_info; >>>>>>> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); >>>>>>> + if (err) >>>>>>> + netdev_dbg(dev, "fdb del failed err=%d\n", err); >>>>>> >>>>>> OK I must have missed from the off-list discussion why we are not >>>>>> calling the switchdev notifier here? >>>>> >>>>> We do not agree on it actually, that is why it was moved to the list. >>>>> I think that delete should succeed, you should retry until succession. >>>>> >>>>> The deletion is done under spinlock in the bridge so you cannot block, >>>>> thus delete cannot fail due to hardware failure. Calling it here doesn't >>>>> make sense because the bridge probably already deleted this FDB. >>>> >>>> So as we discussed, the problem here is that if dsa_port_fdb_del fails >>>> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux >>>> bridge will delete the entry in software, dumping bridge fdb will show >>>> nothing, but the entry would still be programmed in hardware and the >>>> network can thus be inconsistent, unsupposedly switching frames. >>>> >>>> IMHO the correct way for bridge to use the notification chain is to make >>>> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE: >>>> if an entry has been marked as offloaded, bridge must mark the entry as >>>> to-be-deleted and do not delete the software entry until the driver >>>> notifies back the successful deletion. >>>> >>>> If that is hardly feasible due to some bridge limitations, we must >>>> explain this in a comment and use something more explosive than a simple >>>> netdev_dbg to warn the user about the broken network setup... >>>> >>>> >>>> Thanks, >>>> >>>> Vivien >>>> >>> >>> Hi Nikolay, >>> >>> Vivien raised inconsistency issue with the current switchdev >>> notification chain in case of FDB del. In case of static FDB delete, >>> notification will be sent to the driver, followed by deletion of the >>> software entry without waiting for the hardware delete. In case of >>> hardware deletion failure the consecutive FDB dump will not show the >>> deleted entry, yet, the entry will stay in hardware. >>> >>> The deletion is done under lock thus the hardware deletion is deferred, >>> and cannot fail due to hardware removal failure. Thus the above proposed >>> solution by Vivien can lead to confusing situation: >>> >>> 1. User deletes the entry >>> 2. Deletion succeed >>> 3. User dumps FDB and still sees this entry due to hardware failure, >>> what should he do? retry to delete until the FDB dump will not show >>> the entry? >>> >>> Would like to hear you opinion about this solution. >>> >>> IMHO in this case the driver should retry to delete, in case of >>> several retries the driver should maybe: >>> 1. Trap the traffic to CPU (dint think it possible in case of DSA). >>> 2. Disable the port (its more explosive then netdev_dbg). >>> >>> Thanks, >>> Arkadi >>> >>> >> >> Hi, >> Looking at the code - it would seem that retrying is the only current option >> with the way these switchdev notifications are handled. They cannot fail, meaning >> from the bridge POV these ops must always succeed and errors are ignored, so the >> driver should do everything possible to stay in sync, and in case all fails >> then disabling the port seems like the best option to me, to show that something is >> clearly wrong and avoid further issues, but DSA maintainers can comment more >> on how to handle failure. >> >> That being said: >> This sounds a lot like the switchdev notifications vs callbacks discussions that we've >> had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock >> is the main problem let's work towards improving that and making the fdb code handle >> switchdev similar to the vlan code. >> >> Cheers, >> Nik >> > > Vlans can be only added by user under rtnl so its possible to sleep. On > the other hand FDBs can be learned in atomic context, thus the > notification chain is atomic. One Possible way I thought about doing it Right, that's why I mentioned the hash_lock. :-) > is to maintain bridge internal ordered workqueue for the FDBs learned in > atomic context, furthermore the FDB table will be protected by mutex. > The STATIC entries are added in user context so the user will add > directly to this hash table and could sleep. I was thinking in the same direction but instead just keep per-cpu delayed learning lists that get emptied (i.e. learned) from a workqueue that gets scheduled from atomic context, but it seems to me the complexity is unjustified if the drivers can handle the errors, they're the only reason for doing this right now. So options so far look like: 1) return success & driver retry until op is successful or deal with fallout (e.g. disable hw fwd, disable port etc) 2) move to mutex so we can fail the user requested op on hw failure 3) return success but don't actually delete fdb until hw succeeds In the current situation 1) seems like a good option as we discussed, I don't like 3). 2) and similar alternatives can be discussed further, I have plans to migrate the fdb code to a rhashtable and can adjust the change for it. > > Its not very good, but I don't think about another solution here. > > Regarding notification or not, drivers should know about FDBs that are > not directly related to them like in case of vxlan (vxlan device is not > upper device of some siwthcdev port) so notification is better approach > in my opinion. > > Regarding prepare/commit, it is not used in mlxsw. And we would like > to remove it from DSA (in case of FDBs). The prepare phase cannot > assure success because errors could happen unrelated to reserved space > in the switch in the commit stage.. > Yeah, I figured as much. > Thanks, > Arkadi >> Cheers, Nik >> >> >> >> >> >> >>
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 19395cc..d0592f2 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, return NOTIFY_DONE; } +struct dsa_switchdev_event_work { + struct work_struct work; + struct switchdev_notifier_fdb_info fdb_info; + struct net_device *dev; + unsigned long event; +}; + +static void dsa_switchdev_event_work(struct work_struct *work) +{ + struct dsa_switchdev_event_work *switchdev_work = + container_of(work, struct dsa_switchdev_event_work, work); + struct net_device *dev = switchdev_work->dev; + struct switchdev_notifier_fdb_info *fdb_info; + struct dsa_slave_priv *p = netdev_priv(dev); + int err; + + rtnl_lock(); + switch (switchdev_work->event) { + case SWITCHDEV_FDB_ADD_TO_DEVICE: + fdb_info = &switchdev_work->fdb_info; + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid); + if (err) { + netdev_dbg(dev, "fdb add failed err=%d\n", err); + break; + } + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, + &fdb_info->info); + break; + + case SWITCHDEV_FDB_DEL_TO_DEVICE: + fdb_info = &switchdev_work->fdb_info; + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid); + if (err) + netdev_dbg(dev, "fdb del failed err=%d\n", err); + break; + } + rtnl_unlock(); + + kfree(switchdev_work->fdb_info.addr); + kfree(switchdev_work); + dev_put(dev); +} + +static int +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work * + switchdev_work, + const struct switchdev_notifier_fdb_info * + fdb_info) +{ + memcpy(&switchdev_work->fdb_info, fdb_info, + sizeof(switchdev_work->fdb_info)); + switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC); + if (!switchdev_work->fdb_info.addr) + return -ENOMEM; + ether_addr_copy((u8 *)switchdev_work->fdb_info.addr, + fdb_info->addr); + return 0; +} + +/* Called under rcu_read_lock() */ +static int dsa_slave_switchdev_event(struct notifier_block *unused, + unsigned long event, void *ptr) +{ + struct net_device *dev = switchdev_notifier_info_to_dev(ptr); + struct dsa_switchdev_event_work *switchdev_work; + + if (!dsa_slave_dev_check(dev)) + return NOTIFY_DONE; + + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); + if (!switchdev_work) + return NOTIFY_BAD; + + INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work); + switchdev_work->dev = dev; + switchdev_work->event = event; + + switch (event) { + case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */ + case SWITCHDEV_FDB_DEL_TO_DEVICE: + if (dsa_slave_switchdev_fdb_work_init(switchdev_work, + ptr)) + goto err_fdb_work_init; + dev_hold(dev); + break; + default: + kfree(switchdev_work); + return NOTIFY_DONE; + } + + dsa_schedule_work(&switchdev_work->work); + return NOTIFY_OK; + +err_fdb_work_init: + kfree(switchdev_work); + return NOTIFY_BAD; +} + static struct notifier_block dsa_slave_nb __read_mostly = { - .notifier_call = dsa_slave_netdevice_event, + .notifier_call = dsa_slave_netdevice_event, +}; + +static struct notifier_block dsa_slave_switchdev_notifier = { + .notifier_call = dsa_slave_switchdev_event, }; int dsa_slave_register_notifier(void) { - return register_netdevice_notifier(&dsa_slave_nb); + int err; + + err = register_netdevice_notifier(&dsa_slave_nb); + if (err) + return err; + + err = register_switchdev_notifier(&dsa_slave_switchdev_notifier); + if (err) + goto err_register_switchdev; + + return 0; + +err_register_switchdev: + unregister_netdevice_notifier(&dsa_slave_nb); + return err; } void dsa_slave_unregister_notifier(void) { int err; + err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier); + if (err) + pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err); + err = unregister_netdevice_notifier(&dsa_slave_nb); if (err) pr_err("DSA: failed to unregister slave notifier (%d)\n", err);
Add support for learning FDB through notification. The driver defers the hardware update via ordered work queue. In case of a successful FDB add a notification is sent back to bridge. Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> --- net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 122 insertions(+), 2 deletions(-)