[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240430005949.44lcegwhmcetpddm@skbuf>
Date: Tue, 30 Apr 2024 03:59:49 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Joseph Huang <joseph.huang.2024@...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 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.
> Note that on the system I tested, each register read/write takes about 100us
> to complete. For 100's of mdb groups, this would mean that we will be
> holding rtnl lock for 10's of ms. I don't know if it's considered too long.
Not sure how this is going to be any better if the iteration over MDB
entries is done 100% in the driver, though - since its hook,
dsa_port_mrouter(), runs entirely under rtnl_lock().
Anyway, with the SWITCHDEV_F_DEFER flag, maybe the mdb object
notifications can be made to run by switchdev only a few at a time, to
give the network stack time to do other things as well.
Powered by blists - more mailing lists