On Tue, Aug 6, 2024 at 4:59 PM Jakub Kicinski kuba@kernel.org 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?