Subject: Re: [PATCH v2 net-next 0/5] Make SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE blocking

From: Ido Schimmel <[email-masked]>
Date: Sun, 22 Aug 2021 20:06:00 +0300

On Sun, Aug 22, 2021 at 04:31:45PM +0300, Vladimir Oltean wrote:
> 3. There is a larger issue that SWITCHDEV_FDB_ADD_TO_DEVICE events are
>    deferred by drivers even from code paths that are initially blocking
>    (are running in process context):
> 
> br_fdb_add
> -> __br_fdb_add
>    -> fdb_add_entry
>       -> fdb_notify
>          -> br_switchdev_fdb_notify
> 
>     It seems fairly trivial to move the fdb_notify call outside of the
>     atomic section of fdb_add_entry, but with switchdev offering only an
>     API where the SWITCHDEV_FDB_ADD_TO_DEVICE is atomic, drivers would
>     still have to defer these events and are unable to provide
>     synchronous feedback to user space (error codes, extack).
> 
> The above issues would warrant an attempt to fix a central problem, and
> make switchdev expose an API that is easier to consume rather than
> having drivers implement lateral workarounds.
> 
> In this case, we must notice that
> 
> (a) switchdev already has the concept of notifiers emitted from the fast
>     path that are still processed by drivers from blocking context. This
>     is accomplished through the SWITCHDEV_F_DEFER flag which is used by
>     e.g. SWITCHDEV_OBJ_ID_HOST_MDB.
> 
> (b) the bridge del_nbp() function already calls switchdev_deferred_process().
>     So if we could hook into that, we could have a chance that the
>     bridge simply waits for our FDB entry offloading procedure to finish
>     before it calls netdev_upper_dev_unlink() - which is almost
>     immediately afterwards, and also when switchdev drivers typically
>     break their stateful associations between the bridge upper and
>     private data.
> 
> So it is in fact possible to use switchdev's generic
> switchdev_deferred_enqueue mechanism to get a sleepable callback, and
> from there we can call_switchdev_blocking_notifiers().
> 
> To address all requirements:
> 
> - drivers that are unconverted from atomic to blocking still work
> - drivers that currently have a private workqueue are not worse off
> - drivers that want the bridge to wait for their deferred work can use
>   the bridge's defer mechanism
> - a SWITCHDEV_FDB_ADD_TO_DEVICE event which does not have any interested
>   parties does not get deferred for no reason, because this takes the
>   rtnl_mutex and schedules a worker thread for nothing
> 
> it looks like we can in fact start off by emitting
> SWITCHDEV_FDB_ADD_TO_DEVICE on the atomic chain. But we add a new bit in
> struct switchdev_notifier_fdb_info called "needs_defer", and any
> interested party can set this to true.
> 
> This way:
> 
> - unconverted drivers do their work (i.e. schedule their private work
>   item) based on the atomic notifier, and do not set "needs_defer"
> - converted drivers only mark "needs_defer" and treat a separate
>   notifier, on the blocking chain, called SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED
> - SWITCHDEV_FDB_ADD_TO_DEVICE events with no interested party do not
>   generate any follow-up SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED
> 
> Additionally, code paths that are blocking right not, like br_fdb_replay,
> could notify only SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED, as long as all
> consumers of the replayed FDB events support that (right now, that is
> DSA and dpaa2-switch).
> 
> Once all consumers of SWITCHDEV_FDB_ADD_TO_DEVICE are converted to set
> needs_defer as appropriate, then the notifiers emitted from process
> context by the bridge could call SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED
> directly, and we would also have fully blocking context all the way
> down, with the opportunity for error propagation and extack.

IIUC, at this stage all the FDB notifications drivers get are blocking,
either from the work queue (because they were deferred) or directly from
process context. If so, how do we synchronize the two and ensure drivers
get the notifications at the correct order?

I was thinking of adding all the notifications to the 'deferred' list
when 'hash_lock' is held and then calling switchdev_deferred_process()
directly in process context. It's not very pretty (do we return an error
only for the entry the user added or for any other entry we flushed from
the list?), but I don't have a better idea right now.

> 
> Some disadvantages of this solution though:
> 
> - A driver now needs to check whether it is interested in an event
>   twice: first on the atomic call chain, then again on the blocking call
>   chain (because it is a notifier chain, it is potentially not the only
>   driver subscribed to it, it may be listening to another driver's
>   needs_defer request). The flip side: on sistems with mixed switchdev
>   setups (dpaa2-switch + DSA, and DSA sniffs dynamically learned FDB
>   entries on foreign interfaces), there are some "synergies", and the
>   SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED event is only emitted once, as
>   opposed to what would happen if each driver scheduled its own private
>   work item.
> 
> - Right now drivers take rtnl_lock() as soon as their private work item
>   runs. They need the rtnl_lock for the call to
>   call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED). 

I think RCU is enough?

>   But it doesn't really seem necessary for them to perform the actual
>   hardware manipulation (adding the FDB entry) with the rtnl_lock held
>   (anyway most do that). But with the new option of servicing
>   SWITCHDEV_FDB_ADD_TO_DEVICE_DEFERRED, the rtnl_lock is taken
>   top-level by switchdev, so even if these drivers wanted to be more
>   self-conscious, they couldn't.

Yes, I want to remove this dependency in mlxsw assuming notifications
remain atomic. The more pressing issue is actually removing it from the
learning path.