On Tue, Aug 6, 2024 at 4:59 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
...
> On Mon, 5 Aug 2024 21:25:20 +0000 Mina Almasry wrote:
> > + if (pool->p.queue) {
> > + /* We rely on rtnl_lock()ing to make sure netdev_rx_queue
> > + * configuration doesn't change while we're initializing the
> > + * page_pool.
> > + */
> > + ASSERT_RTNL();
> > + pool->mp_priv = pool->p.queue->mp_params.mp_priv;
>
Hi Jakub,
Sorry for the late reply, it took a bit of code reading to understand
what you mean with the deactivation request on the other patch, but I
think I got it down and have a patch on the way.
> 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?
> 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.
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?
--
Thanks,
Mina
