Message-ID: <20240808192410.37a49724@kernel.org>
Date: Thu, 8 Aug 2024 19:24:10 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-alpha@...r.kernel.org,
 linux-mips@...r.kernel.org, linux-parisc@...r.kernel.org,
 sparclinux@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-arch@...r.kernel.org, linux-kselftest@...r.kernel.org,
 bpf@...r.kernel.org, linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, Donald Hunter <donald.hunter@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet
 <corbet@....net>, Richard Henderson <richard.henderson@...aro.org>, Ivan
 Kokshaysky <ink@...assic.park.msu.ru>, Matt Turner <mattst88@...il.com>,
 Thomas Bogendoerfer <tsbogend@...ha.franken.de>, "James E.J. Bottomley"
 <James.Bottomley@...senpartnership.com>, Helge Deller <deller@....de>,
 Andreas Larsson <andreas@...sler.com>, Jesper Dangaard Brouer
 <hawk@...nel.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>, Steven
 Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Arnd Bergmann
 <arnd@...db.de>, Steffen Klassert <steffen.klassert@...unet.com>, Herbert
 Xu <herbert@...dor.apana.org.au>, David Ahern <dsahern@...nel.org>, Willem
 de Bruijn <willemdebruijn.kernel@...il.com>, Shuah Khan <shuah@...nel.org>,
 Sumit Semwal <sumit.semwal@...aro.org>, Christian König
 <christian.koenig@....com>, Bagas Sanjaya <bagasdotme@...il.com>, Christoph
 Hellwig <hch@...radead.org>, Nikolay Aleksandrov <razor@...ckwall.org>,
 Taehee Yoo <ap420073@...il.com>, Pavel Begunkov <asml.silence@...il.com>,
 David Wei <dw@...idwei.uk>, Jason Gunthorpe <jgg@...pe.ca>, Yunsheng Lin
 <linyunsheng@...wei.com>, Shailend Chand <shailend@...gle.com>, Harshitha
 Ramamurthy <hramamurthy@...gle.com>, Shakeel Butt <shakeel.butt@...ux.dev>,
 Jeroen de Borst <jeroendb@...gle.com>, Praveen Kaligineedi
 <pkaligineedi@...gle.com>, Willem de Bruijn <willemb@...gle.com>, Kaiyuan
 Zhang <kaiyuanz@...gle.com>
Subject: Re: [PATCH net-next v18 07/14] memory-provider: dmabuf devmem
 memory provider

On Thu, 8 Aug 2024 16:36:24 -0400 Mina Almasry wrote:
> > How do you know that the driver:
> >  - supports net_iov at all (let's not make implicit assumptions based
> >    on presence of queue API);
> >  - supports net_iov in current configuration (eg header-data split is
> >    enabled)
> >  - supports net_iov for _this_ pool (all drivers must have separate
> >    buffer pools for headers and data for this to work, some will use
> >    page pool for both)
> >
> > What comes to mind is adding an "I can gobble up net_iovs from this
> > pool" flag in page pool params (the struct that comes from the driver),  
> 
> This already sorta exists in the current iteration, although maybe in
> an implicit way. As written, drivers need to set params.queue,
> otherwise core will not attempt to grab the mp information from
> params.queue. A driver can set params.queue for its data pages pool
> and not set it for the headers pool. AFAICT that deals with all 3
> issues you present above.
> 
> The awkward part is if params.queue starts getting used for other
> reasons rather than passing mp configuration, but as of today that's
> not the case so I didn't add the secondary flag. If you want a second
> flag to be added preemptively, I can do that, no problem. Can you
> confirm params.queue is not good enough?

I'd prefer a flag. The setting queue in a param struct is not a good
API for conveying that the page pool is for netmem payloads only.

> > and then on the installation path we can check if after queue reset
> > the refcount of the binding has increased. If it did - driver has
> > created a pool as we expected, otherwise - fail, something must be off.
> > Maybe that's a bit hacky?  
> 
> What's missing is for core to check at binding time that the driver
> supports net_iov. I had relied on the implicit presence of the
> queue-API.
> 
> What you're proposing works, but AFAICT it's quite hacky, yes. I
> basically need to ASSERT_RTNL in net_devmem_binding_get() to ensure
> nothing can increment the refcount while the binding is happening so
> that the refcount check is valid.

True. Shooting from the hip, but we could walk the page pools of the
netdev and find the one that has the right mp installed, and matches
queue? The page pools are on a list hooked up to the netdev, trivial
to walk.

> I think a less hacky approach is to add a function to the queue-API
> like ndo_queue_supported_features(), which lets the driver declare
> that it supports net_iov at a given rx queue. However I'm open to both
> approaches. What do you prefer?

I kinda like trying to query the page pools more, because it's both
fewer driver changes, and it actually validates that the driver did 
the right thing based on outcomes. Driver callback may have bugs.

If you prefer strongly - fine, but hm.

Powered by blists - more mailing lists