Re: [RFC PATCH net-next v1 4/4] net: page_pool: use netmem_t instead of struct page in API

On Sat, Dec 16, 2023 at 2:06 PM Mina Almasry [off-list ref] wrote:

On Sat, Dec 16, 2023 at 11:47 AM Shakeel Butt [off-list ref] wrote:

quoted

On Fri, Dec 15, 2023 at 7:01 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Fri, 15 Dec 2023 02:11:14 +0000 Shakeel Butt wrote:

quoted

quoted

From my POV it has to be the first one. We want to abstract the memory
type from the drivers as much as possible, not introduce N new memory
types and ask the driver to implement new code for each of them
separately.

Agree with Mina's point. Let's aim to decouple memory types from
drivers.

What does "decouple" mean? Drivers should never convert netmem
to pages. Either a path in the driver can deal with netmem,
i.e. never touch the payload, or it needs pages.

I'm guessing the paths in the driver that need pages will have to be
disabled for non-paged netmem, which is fine.

One example that I ran into with GVE is that it calls page_address()
to copy small packets instead of adding them as a frag. I can add a
netmem_address() that returns page_address() for pages, and NULL for
non-pages (never passing non-pages to mm code). The driver can detect
that the netmem has no address, and disable the optimization for
non-paged netmem.

quoted

"Decouple" might not be the right word. What I wanted to say was to
avoid too much specialization such that we have to have a new API for
every new fancy thing.

quoted

Perhaps we should aim to not export netmem_to_page(),
prevent modules from accessing it directly.

+1.

I looked into this, but it turns out it's a slightly bigger change
that needs some refactoring to make it work. There are few places
where I believe I need to add netmem_to_page() that are exposed to the
drivers via inline helpers, these are:

- skb_frag_page(), which returns NULL if the netmem is not a page, but
needs to do a netmem_to_page() to return the page otherwise.
- The helpers inside skb_add_rx_frag(), which needs to do a
netmem_to_page() to set skb->pfmemalloc.
- Some of the page_pool APIs are exposed to the drivers as static
inline helpers, and if I want the page_pool to use netmem internally
the page_pool needs to do a netmem_to_page() in these helpers.

The refactor is not an issue, but I was wondering if not exporting
netmem_to_page() was worth moving the code around. I was thinking in
the interim until netmem is adopted and has actual driver users we may
prefer to just add a comment on the netmem_to_page() helper that says
'try not to use this directly and use the netmem helpers instead'.

This is an aggressive approach and I like it. I'll try to make it work
(should be fine).


--
Thanks,
Mina

--
Thanks,
Mina