[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <85cb2625-7bac-45cd-af31-0d114253e6e4@gmail.com>
Date: Thu, 2 May 2024 16:37:16 -0400
From: Joseph Huang <joseph.huang.2024@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Joseph Huang <Joseph.Huang@...min.com>, netdev@...r.kernel.org,
Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Roopa Prabhu <roopa@...dia.com>, Nikolay Aleksandrov <razor@...ckwall.org>,
Linus Lüssing <linus.luessing@...3.blue>,
linux-kernel@...r.kernel.org, bridge@...ts.linux.dev
Subject: Re: [PATCH RFC net-next 07/10] net: dsa: mv88e6xxx: Track bridge mdb
objects
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
Powered by blists - more mailing lists