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

On Mon, Aug 23, 2021 at 06:18:08PM +0300, Ido Schimmel wrote:

On Mon, Aug 23, 2021 at 05:29:53PM +0300, Vladimir Oltean wrote:

quoted

On Mon, Aug 23, 2021 at 03:16:48PM +0300, Ido Schimmel wrote:

quoted

I was thinking about the following case:

t0 - <MAC1,VID1,P1> is added in syscall context under 'hash_lock'
t1 - br_fdb_delete_by_port() flushes entries under 'hash_lock' in
     response to STP state. Notifications are added to 'deferred' list
t2 - switchdev_deferred_process() is called in syscall context
t3 - <MAC1,VID1,P1> is notified as blocking

Updates to the SW FDB are protected by 'hash_lock', but updates to the
HW FDB are not. In this case, <MAC1,VID1,P1> does not exist in SW, but
it will exist in HW.

Another case assuming switchdev_deferred_process() is called first:

t0 - switchdev_deferred_process() is called in syscall context
t1 - <MAC1,VID,P1> is learned under 'hash_lock'. Notification is added
     to 'deferred' list
t2 - <MAC1,VID1,P1> is modified in syscall context under 'hash_lock' to
     <MAC1,VID1,P2>
t3 - <MAC1,VID1,P2> is notified as blocking
t4 - <MAC1,VID1,P1> is notified as blocking (next time the 'deferred'
     list is processed)

In this case, the HW will have <MAC1,VID1,P1>, but SW will have
<MAC1,VID1,P2>

Ok, so if the hardware FDB entry needs to be updated under the same
hash_lock as the software FDB entry, then it seems that the goal of
updating the hardware FDB synchronously and in a sleepable manner is if
the data path defers the learning to sleepable context too. That in turn
means that there will be 'dead time' between the reception of a packet
from a given {MAC SA, VID} flow and the learning of that address. So I
don't think that is really desirable. So I don't know if it is actually
realistic to do this.

Can we drop it from the requirements of this change, or do you feel like
it's not worth it to make my change if this problem is not solved?

I didn't pose it as a requirement, but as a desirable goal that I don't
know how to achieve w/o a surgery in the bridge driver that Nik and you
(understandably) don't like.

Regarding a more practical solution, earlier versions (not what you
posted yesterday) have the undesirable properties of being both
asynchronous (current state) and mandating RTNL to be held. If we are
going with the asynchronous model, then I think we should have a model
that doesn't force RTNL and allows batching.

I have the following proposal, which I believe solves your problem and
allows for batching without RTNL:

The pattern of enqueuing a work item per-entry is not very smart.
Instead, it is better to to add the notification info to a list
(protected by a spin lock) and scheduling a single work item whose
purpose is to dequeue entries from this list and batch process them.

I don't have hardware where FDB entries can be installed in bulk, so
this is new to me. Might make sense though where you are in fact talking
to firmware, and the firmware is in fact still committing to hardware
one by one, you are still reducing the number of round trips.

Inside the work item you would do something like:

spin_lock_bh()
list_splice_init()
spin_unlock_bh()

mutex_lock() // rtnl or preferably private lock
list_for_each_entry_safe()
	// process entry
	cond_resched()
mutex_unlock()

When is the work item scheduled in your proposal? I assume not only when
SWITCHDEV_FDB_FLUSH_TO_DEVICE is emitted. Is there some sort of timer to
allow for some batching to occur?

In del_nbp(), after br_fdb_delete_by_port(), the bridge will emit some
new blocking event (e.g., SWITCHDEV_FDB_FLUSH_TO_DEVICE) that will
instruct the driver to flush all its pending FDB notifications. You
don't strictly need this notification because of the
netdev_upper_dev_unlink() that follows, but it helps in making things
more structured.

Pros:

1. Solves your problem?
2. Pattern is not worse than what we currently have
3. Does not force RTNL
4. Allows for batching. For example, mlxsw has the ability to program up
to 64 entries in one transaction with the device. I assume other devices
in the same grade have similar capabilities

Cons:

1. Asynchronous
2. Pattern we will see in multiple drivers? Can consider migrating it
into switchdev itself at some point

I can already flush_workqueue(dsa_owq) in dsa_port_pre_bridge_leave()
and this will solve the problem in the same way, will it not?

It's not that I don't have driver-level solutions and hook points.
My concern is that there are way too many moving parts and the entrance
barrier for a new switchdev driver is getting higher and higher to
achieve even basic stuff.

For example, I need to maintain a DSA driver and a switchdev driver for
the exact same class of hardware (ocelot is switchdev, felix is DSA, but
the hardware is the same) and it is just so annoying that the interaction
with switchdev is so verbose and open-coded, it just leads to so much
duplication of basic patterns.
When I add support for SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE in ocelot I
really don't want to add a boatload of code, all copied from DSA.

3. Something I missed / overlooked

quoted

There is of course the option of going half-way too, just like for
SWITCHDEV_PORT_ATTR_SET. You notify it once, synchronously, on the
atomic chain, the switchdev throws as many errors as it can reasonably
can, then you defer the actual installation which means a hardware access.

Yes, the above proposal has the same property. You can throw errors
before enqueueing the notification info on your list.