On Mon, Jun 17, 2024 at 7:17 AM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote: > > On 6/13/24 02:35, Mina Almasry wrote: > > Convert netmem to be a union of struct page and struct netmem. Overload > > the LSB of struct netmem* to indicate that it's a net_iov, otherwise > > it's a page. > > > > Currently these entries in struct page are rented by the page_pool and > > used exclusively by the net stack: > > > > struct { > > unsigned long pp_magic; > > struct page_pool *pp; > > unsigned long _pp_mapping_pad; > > unsigned long dma_addr; > > atomic_long_t pp_ref_count; > > }; > > > > Mirror these (and only these) entries into struct net_iov and implement > > netmem helpers that can access these common fields regardless of > > whether the underlying type is page or net_iov. > > > > Implement checks for net_iov in netmem helpers which delegate to mm > > APIs, to ensure net_iov are never passed to the mm stack. > > > > Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx> > > Apart from small comments below > > Reviewed-by: Pavel Begunkov <asml.silence@xxxxxxxxx> > > > > --- > > include/net/netmem.h | 137 ++++++++++++++++++++++++++++++-- > > include/net/page_pool/helpers.h | 25 +++--- > > net/core/devmem.c | 3 + > > net/core/page_pool.c | 26 +++--- > > net/core/skbuff.c | 22 +++-- > > 5 files changed, 168 insertions(+), 45 deletions(-) > > > > diff --git a/include/net/netmem.h b/include/net/netmem.h > > index 664df8325ece5..35ad237fdf29e 100644 > > --- a/include/net/netmem.h > > +++ b/include/net/netmem.h > ... > > -/* Converting from page to netmem is always safe, because a page can always be > > - * a netmem. > > - */ > > static inline netmem_ref page_to_netmem(struct page *page) > > { > > return (__force netmem_ref)page; > > @@ -68,17 +107,103 @@ static inline netmem_ref page_to_netmem(struct page *page) > > > > static inline int netmem_ref_count(netmem_ref netmem) > > { > > + /* The non-pp refcount of net_iov is always 1. On net_iov, we only > > + * support pp refcounting which uses the pp_ref_count field. > > + */ > > + if (netmem_is_net_iov(netmem)) > > + return 1; > > + > > return page_ref_count(netmem_to_page(netmem)); > > } > > > > static inline unsigned long netmem_to_pfn(netmem_ref netmem) > > { > > + if (netmem_is_net_iov(netmem)) > > + return 0; > > IIRC 0 is a valid pfn. Not much of a concern since it's > used only for tracing, but might make sense to pass some > invalid pfn if there is one > AFAIU all non-negative pfns are technically valid pfns if the machine is big enough. I could have this function return long long instead of unsigned long so I can return a negative number for errors, and then cast to unsigned long when I figure out it's actually a pfn. Seemed like such a hassle especially since the call site is just tracing that I figured it's not that worth it. > > + > > return page_to_pfn(netmem_to_page(netmem)); > > } > > > ... > > static inline netmem_ref netmem_compound_head(netmem_ref netmem) > > { > > + /* niov are never compounded */ > > + if (netmem_is_net_iov(netmem)) > > + return netmem; > > + > > return page_to_netmem(compound_head(netmem_to_page(netmem))); > > } > > > > +static inline void *netmem_address(netmem_ref netmem) > > I don't think it's used anywhere, do I miss it? > Ah, It's used by the GVE devmem implementation: https://github.com/mina/linux/commit/da89baa81873d457cbf7b49ee6b4f0d66855b205 I could leave it out of this patch, then add it with the follow up GVE devmem implementation, but I figured almost for sure drivers are going to need this eventually, and it's small, so just put it here. > > +{ > > + if (netmem_is_net_iov(netmem)) > > + return NULL; > > + > > + return page_address(netmem_to_page(netmem)); > > +} > > + > ... > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > index a5957d3359762..1152e3547795a 100644 > > --- a/net/core/page_pool.c > > +++ b/net/core/page_pool.c > > @@ -26,6 +26,8 @@ > ... > > > > /* If the page refcnt == 1, this will try to recycle the page. > > @@ -714,7 +713,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem, > > * refcnt == 1 means page_pool owns page, and can recycle it. > > * > > * page is NOT reusable when allocated when system is under > > - * some pressure. (page_is_pfmemalloc) > > + * some pressure. (page_pool_page_is_pfmemalloc) > > There is no page_pool_page_is_pfmemalloc() > Thanks done. I implemented most of your other comments on all the patches btw. I'm only responding to the ones I didn't apply for various reasons. Thanks for the review! -- Thanks, Mina