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

From: Vladimir Oltean <olteanv@gmail.com>
To: Ido Schimmel <idosch@idosch.org>
Cc: Nikolay Aleksandrov <nikolay@nvidia.com>,
	Vladimir Oltean <vladimir.oltean@nxp.com>,
	netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Roopa Prabhu <roopa@nvidia.com>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Vadym Kochan <vkochan@marvell.com>,
	Taras Chornyi <tchornyi@marvell.com>,
	Jiri Pirko <jiri@nvidia.com>, Ido Schimmel <idosch@nvidia.com>,
	UNGLinuxDriver@microchip.com,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Marek Behun <kabel@blackhole.sk>,
	DENG Qingfang <dqfext@gmail.com>,
	Kurt Kanzenbach <kurt@linutronix.de>,
	Hauke Mehrtens <hauke@hauke-m.de>,
	Woojung Huh <woojung.huh@microchip.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Landen Chao <Landen.Chao@mediatek.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	George McCollister <george.mccollister@gmail.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Leon Romanovsky <leon@kernel.org>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Julian Wiedmann <jwi@linux.ibm.com>,
	Karsten Graul <kgraul@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Ivan Vecera <ivecera@redhat.com>, Vlad Buslov <vladbu@nvidia.com>,
	Jianbo Liu <jianbol@nvidia.com>, Mark Bloch <mbloch@nvidia.com>,
	Roi Dayan <roid@nvidia.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>
Subject: Re: [PATCH v2 net-next 0/5] Make SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE blocking
Date: Mon, 23 Aug 2021 19:23:32 +0300	[thread overview]
Message-ID: <20210823162332.mj5gmjb4to3uacnq@skbuf> (raw)
In-Reply-To: <YSPGh4Fj3idApeFx@shredder>

On Mon, Aug 23, 2021 at 07:02:15PM +0300, Ido Schimmel wrote:
> > When is the work item scheduled in your proposal?
> 
> Calling queue_work() whenever you get a notification. The work item
> might already be queued, which is fine.
> 
> > 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?
> 
> You can add an hysteresis timer if you want, but I don't think it's
> necessary. Assuming user space is programming entries at a high rate,
> then by the time you finish a batch, you will have a new one enqueued.

I tried to do something similar in DSA. There we have .ndo_fdb_dump
because we don't sync the hardware FDB. We also have some drivers where
the FDB flush on a port is a very slow procedure, because the FDB needs
to be walked element by element to see what needs to be deleted.
So I wanted to defer the FDB flush to a background work queue, and let
the port leave the bridge quickly and not block the rtnl_mutex.
But it gets really nasty really quick. The FDB flush workqueue cannot
run concurrently with the ndo_fdb_dump, for reasons that have to do with
hardware access. Also, any fdb_add or fdb_del would need to flush the
FDB flush workqueue, for the same reasons. All these are currently
implicitly serialized by the rtnl_mutex now. Your hardware/firmware might
be smarter, but I think that if you drop the rtnl_mutex requirement, you
will be seriously surprised by the amount of extra concurrency you need
to handle.
In the end I scrapped everything and I'm happy with a synchronous FDB
flush even if it's slow. YMMV of course.