On 4/30/2024 12:27 PM, Joseph Huang wrote:
On 4/29/2024 8:59 PM, Vladimir Oltean wrote:
On Mon, Apr 29, 2024 at 06:07:25PM -0400, Joseph Huang wrote:
Something like this (some layers omitted for brevity)?
+br_iterator
| for each mdb
| _br_switchdev_mdb_notify
rtnl_lock | without F_DEFER flag
| | |
+switchdev_port_attr_set_deferred | +switchdev_port_obj_notify
| | |
+dsa_port_mrouter | +dsa_user_port_obj_a/d
| | |
+mv88e6xxx_port_mrouter----------+
+mv88e6xxx_port_obj_a/d
|
+--------------------------------------+
|
rtnl_unlock
At a _very_ superficial glance, I don't think you are properly
accounting for the fact that even with rtnl_lock() held, there are still
SWITCHDEV_OBJ_ID_PORT_MDB events which may be pending on the switchdev
chain. Without a switchdev_deferred_process() flush call, you won't be
getting rid of them, so when you rtnl_unlock(), they will still run.
Even worse, holding rtnl_lock() will not stop the bridge multicast layer
from modifying its br->mdb_list; only br->multicast_lock will.
So you may be better off also acquiring br->multicast_lock, and
notifying the MDB entries to the switchdev chain _with_the F_DEFER flag.
Like this?
+br_iterator(dsa_cb)
| lock br->multicask_lock
| for each mdb
| br_switchdev_mdb_notify
rtnl_lock | |
| | +switchdev_port_obj_._defer
+switchdev_port_attr_set_deferred | unlock br->multicast_lock
| |
+dsa_port_mrouter |
| |
+mv88e6xxx_port_mrouter----------+
|
+--------------------------------------+
|
rtnl_unlock
(potential task change)
rtnl_lock
|
+switchdev_deferred_process
| flush all queued dfitems in queuing order
|
rtnl_unlock
I'm not that familiar with the bridge code, but is there any concern
with potential deadlock here (between rtnl_lock and br->multicast_lock)?
Hi Nik,
Do you know if it's safe to acquire rtnl_lock and br->multicast_lock in
the following sequence? Is there any potential possibility for a deadlock?
rtnl_lock
spin_lock(br->multicast_lock)
spin_unlock(br->multicast_lock)
rtnl_unlock
If the operation is safe, the next question is should it be spin_lock or
spin_lock_bh?
Thanks,
Joseph
