On Tue, Mar 5, 2024 at 6:47 PM David Wei dw@davidwei.uk wrote:
On 2024-03-05 18:42, Mina Almasry wrote:
On Tue, Mar 5, 2024 at 6:28 PM David Wei dw@davidwei.uk wrote:
On 2024-03-04 18:01, Mina Almasry wrote:
if (pool->p.queue)binding = READ_ONCE(pool->p.queue->binding);if (binding) {pool->mp_ops = &dmabuf_devmem_ops;pool->mp_priv = binding;}This is specific to TCP devmem. For ZC Rx we will need something more generic to let us pass our own memory provider backend down to the page pool.
What about storing ops and priv void ptr in struct netdev_rx_queue instead? Then we can both use it.
Yes, this is dmabuf specific, I was thinking you'd define your own member of netdev_rx_queue, and then add something like this to page_pool_init:
if (pool->p.queue)io_uring_metadata = READ_ONCE(pool->p.queue->io_uring_metadata);/* We don't support rx-queues that are configured for bothio_uring & dmabuf binding */
BUG_ON(io_uring_metadata && binding);if (io_uring_metadata) {pool->mp_ops = &io_uring_ops;pool->mp_priv = io_uring_metadata;}I.e., we share the pool->mp_ops and the pool->mp_priv but we don't really need to share the same netdev_rx_queue member. For me it's a dma-buf specific data structure (netdev_dmabuf_binding) and for you it's something else.
This adds size to struct netdev_rx_queue and requires checks on whether both are set. There can be thousands of these structs at any one time so if we don't need to add size unnecessarily then that would be best.
We can disambiguate by comparing &mp_ops and then cast the void ptr to our impl specific objects.
What do you not like about this approach?
I was thinking it leaks page_pool specifics into a generic struct unrelated to the page pool like netdev_rx_queue. My mental model is that the rx-queue just says that it's bound to a dma-buf/io_uring unaware of page_pool internals, and the page pool internals figure out what to do from there.
Currently netdev_rx_queue.h doesn't include net/page_pool/types.h for example because there is no dependency between netdev_rx_queue & page_pool, I think this change would add a dependency.
But I concede it does not matter much AFAICT, I can certainly change the netdev_rx_queue to hold the mp_priv & mp_ops directly and include net/page_pool/types.h if you prefer that. I'll look into applying this change in the next iteration if there are no objections.
page_pool_init() probably needs to validate that the queue is configured for dma-buf or io_uring but not both. If it's configured for both then the user is doing something funky we shouldn't support.
Perhaps I can make the intention clearer by renaming 'binding' to something more specific to dma-buf like queue->dmabuf_binding, to make it clear that this is the dma-buf binding and not some other binding like io_uring?