Message-ID: <20250410132706.GR199604@unreal>
Date: Thu, 10 Apr 2025 16:27:06 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Larysa Zaremba <larysa.zaremba@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Tatyana Nikolova <tatyana.e.nikolova@...el.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	Lee Trager <lee@...ger.us>,
	Madhavan Srinivasan <maddy@...ux.ibm.com>,
	Sridhar Samudrala <sridhar.samudrala@...el.com>,
	Jacob Keller <jacob.e.keller@...el.com>,
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
	Mateusz Polchlopek <mateusz.polchlopek@...el.com>,
	Ahmed Zaki <ahmed.zaki@...el.com>, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Karlsson, Magnus" <magnus.karlsson@...el.com>,
	Emil Tantilov <emil.s.tantilov@...el.com>,
	Madhu Chittim <madhu.chittim@...el.com>,
	Josh Hay <joshua.a.hay@...el.com>,
	Milena Olech <milena.olech@...el.com>, pavan.kumar.linga@...el.com,
	"Singhai, Anjali" <anjali.singhai@...el.com>,
	Phani R Burra <phani.r.burra@...el.com>
Subject: Re: [PATCH iwl-next 05/14] libeth: add control queue support

On Thu, Apr 10, 2025 at 02:58:28PM +0200, Larysa Zaremba wrote:
> On Thu, Apr 10, 2025 at 02:23:49PM +0300, Leon Romanovsky wrote:
> > On Thu, Apr 10, 2025 at 12:44:33PM +0200, Larysa Zaremba wrote:
> > > On Thu, Apr 10, 2025 at 11:21:37AM +0300, Leon Romanovsky wrote:
> > > > On Tue, Apr 08, 2025 at 02:47:51PM +0200, Larysa Zaremba wrote:
> > > > > From: Phani R Burra <phani.r.burra@...el.com>
> > > > > 
> > > > > Libeth will now support control queue setup and configuration APIs.
> > > > > These are mainly used for mailbox communication between drivers and
> > > > > control plane.
> > > > > 
> > > > > Make use of the page pool support for managing controlq buffers.
> > > > 
> > > > <...>
> > > > 
> > > > >  libeth-y			:= rx.o
> > > > >  
> > > > > +obj-$(CONFIG_LIBETH_CP)		+= libeth_cp.o
> > > > > +
> > > > > +libeth_cp-y			:= controlq.o
> > > > 
> > > > So why did you create separate module for it?
> > > > Now you have pci -> libeth -> libeth_cp -> ixd, with the potential races between ixd and libeth, am I right?
> > > >
> > > 
> > > I am not sure what kind of races do you mean, all libeth modules themselves are 
> > > stateless and will stay this way [0], all used data is owned by drivers.
> > 
> > Somehow such separation doesn't truly work. There are multiple syzkaller
> > reports per-cycle where module A tries to access module C, which already
> > doesn't exist because it was proxied through module B.
> 
> Are there similar reports for libeth and libie modules when iavf is enabled?

To get such report, syzkaller should run on physical iavf, it looks like it doesn't.
Did I miss it here?
https://syzkaller.appspot.com/upstream/s/net

> It is basically the same hierarchy. (iavf uses both libeth and libie, libie 
> depends on libeth).
> 
> I am just trying to understand, is this a regular situation or did I just mess 
> smth up?

My review comment was general one. It is almost impossible to review
this newly proposed architecture split for correctness.

> 
> > 
> > > 
> > > As for the module separation, I think there is no harm in keeping it modular. 
> > 
> > Syzkaller reports disagree with you. 
> >
> 
> Could you please share them?

It is not an easy question to answer, because all these reports are complaining
about some wrong locking order or NULL-pointer access. You will never know if
it is because of programming or design error.

As an approximate example, see commits a27c6f46dcec ("RDMA/bnxt_re: Fix an issue in bnxt_re_async_notifier")
and f0df225d12fc ("RDMA/bnxt_re: Add sanity checks on rdev validity").
At the first glance, they look unrelated to our discussion, however
they can serve as an example or races between deinit/disable paths in
parent module vs. child.

>  
> > > We intend to use basic libeth (libeth_rx) in drivers that for sure have no use 
> > > for libeth_cp. libeth_pci and libeth_cp separation is more arbitral, as we have 
> > > no plans for now to use them separately.
> > 
> > So let's not over-engineer it.
> > 
> > > 
> > > Module dependencies are as follows:
> > > 
> > > libeth_rx and libeth_pci do not depend on other modules.
> > > libeth_cp depends on both libeth_rx and libeth_pci.
> > > idpf directly uses libeth_pci, libeth_rx and libeth_cp.
> > > ixd directly uses libeth_cp and libeth_pci.
> > 
> 
> I need to amend this: libeth_cp does not depend on libeth_pci in terms of 
> module namespace, it only uses the header to access struct device that is 
> stored in libeth_pci-specific mmio_info.

So why did you add SELECT in kconfig?

> 
> > You can do whatever module architecture for netdev devices, but if you
> > plan to expose it to RDMA devices, I will vote against any deep layered
> > module architecture for the drivers.
> > 
> > BTW, please add some Intel prefix to the modules names, they shouldn't
> > be called in generic names like libeth, e.t.c
> >
> 
> We did not think this would be a problem, intel has a tradition of calling the 
> modules pretty ambiguously.

I know and it is worth to be changed.

> 
> > Thanks
> > 
> > > 
> > > [0] https://lore.kernel.org/netdev/61bfa880-6a88-4eac-bab7-040bf72a11ef@intel.com/
> > > 
> > > > Thanks
> 

Powered by blists - more mailing lists