On Sat, Dec 16, 2023 at 2:06 PM Mina Almasry <almasrymina@xxxxxxxxxx> wrote: > > On Sat, Dec 16, 2023 at 11:47 AM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > > > > On Fri, Dec 15, 2023 at 7:01 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > > > > > On Fri, 15 Dec 2023 02:11:14 +0000 Shakeel Butt wrote: > > > > > 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. > > > "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. > > > > > > > > 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
