| Message ID | 20210604183349.30040-1-mcroce@linux.microsoft.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | page_pool: recycle buffers | expand |
On Fri, Jun 04, 2021 at 08:33:44PM +0200, Matteo Croce wrote: > Please let this going in on a future -rc1 so to allow enough time > to have wider tests. > > Note that this series depends on the change "mm: fix struct page layout > on 32-bit systems"[2] which is not yet in master. is so! 9ddb3c14afba8bc5950ed297f02d4ae05ff35cd1
On Fri, Jun 04, 2021 at 08:33:45PM +0200, Matteo Croce wrote: > @@ -130,7 +137,10 @@ struct page { > }; > }; > struct { /* Tail pages of compound page */ > - unsigned long compound_head; /* Bit zero is set */ > + /* Bit zero is set > + * Bit one if pfmemalloc page > + */ > + unsigned long compound_head; I would drop this hunk. Bit 1 is not used for this purpose in tail pages; it's used for that purpose in head and base pages. I suppose we could do something like ... static inline void set_page_pfmemalloc(struct page *page) { - page->index = -1UL; + page->lru.next = (void *)2; } if it's causing confusion.
On Fri, Jun 04, 2021 at 08:33:47PM +0200, Matteo Croce wrote: > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 7fcfea7e7b21..057b40ad29bd 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -40,6 +40,9 @@ > #if IS_ENABLED(CONFIG_NF_CONNTRACK) > #include <linux/netfilter/nf_conntrack_common.h> > #endif > +#ifdef CONFIG_PAGE_POOL > +#include <net/page_pool.h> > +#endif I'm not a huge fan of conditional includes ... any reason to not include it always? > @@ -3088,7 +3095,13 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f) > */ > static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle) > { > - put_page(skb_frag_page(frag)); > + struct page *page = skb_frag_page(frag); > + > +#ifdef CONFIG_PAGE_POOL > + if (recycle && page_pool_return_skb_page(page_address(page))) > + return; It feels weird to have a page here, convert it back to an address, then convert it back to a head page in page_pool_return_skb_page(). How about passing 'page' here, calling compound_head() in page_pool_return_skb_page() and calling virt_to_page() in skb_free_head()? > @@ -251,4 +253,11 @@ static inline void page_pool_ring_unlock(struct page_pool *pool) > spin_unlock_bh(&pool->ring.producer_lock); > } > > +/* Store mem_info on struct page and use it while recycling skb frags */ > +static inline > +void page_pool_store_mem_info(struct page *page, struct page_pool *pp) > +{ > + page->pp = pp; I'm not sure this wrapper needs to exist. > +} > + > #endif /* _NET_PAGE_POOL_H */ > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index e1321bc9d316..a03f48f45696 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -628,3 +628,26 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid) > } > } > EXPORT_SYMBOL(page_pool_update_nid); > + > +bool page_pool_return_skb_page(void *data) > +{ > + struct page_pool *pp; > + struct page *page; > + > + page = virt_to_head_page(data); > + if (unlikely(page->pp_magic != PP_SIGNATURE)) > + return false; > + > + pp = (struct page_pool *)page->pp; You don't need the cast any more. > + /* Driver set this to memory recycling info. Reset it on recycle. > + * This will *not* work for NIC using a split-page memory model. > + * The page will be returned to the pool here regardless of the > + * 'flipped' fragment being in use or not. > + */ > + page->pp = NULL; > + page_pool_put_full_page(pp, page, false); > + > + return true; > +} > +EXPORT_SYMBOL(page_pool_return_skb_page); > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 12b7e90dd2b5..f769f08e7b32 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -70,6 +70,9 @@ > #include <net/xfrm.h> > #include <net/mpls.h> > #include <net/mptcp.h> > +#ifdef CONFIG_PAGE_POOL > +#include <net/page_pool.h> > +#endif > > #include <linux/uaccess.h> > #include <trace/events/skb.h> > @@ -645,10 +648,15 @@ static void skb_free_head(struct sk_buff *skb) > { > unsigned char *head = skb->head; > > - if (skb->head_frag) > + if (skb->head_frag) { > +#ifdef CONFIG_PAGE_POOL > + if (skb->pp_recycle && page_pool_return_skb_page(head)) > + return; > +#endif put this in a header file: static inline bool skb_pp_recycle(struct sk_buff *skb, void *data) { if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle) return false; return page_pool_return_skb_page(virt_to_page(data)); } then this becomes: if (skb->head_frag) { if (skb_pp_recycle(skb, head)) return; > skb_free_frag(head); > - else > + } else { > kfree(head); > + } > } > > static void skb_release_data(struct sk_buff *skb)
On Fri, Jun 4, 2021 at 9:08 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Jun 04, 2021 at 08:33:45PM +0200, Matteo Croce wrote: > > @@ -130,7 +137,10 @@ struct page { > > }; > > }; > > struct { /* Tail pages of compound page */ > > - unsigned long compound_head; /* Bit zero is set */ > > + /* Bit zero is set > > + * Bit one if pfmemalloc page > > + */ > > + unsigned long compound_head; > > I would drop this hunk. Bit 1 is not used for this purpose in tail > pages; it's used for that purpose in head and base pages. > > I suppose we could do something like ... > > static inline void set_page_pfmemalloc(struct page *page) > { > - page->index = -1UL; > + page->lru.next = (void *)2; > } > > if it's causing confusion. > If you prefer, ok for me. Why not "(void *)BIT(1)"? Just to remark that it's a single bit and not a magic like value?
On Sat, Jun 05, 2021 at 12:59:50AM +0200, Matteo Croce wrote: > On Fri, Jun 4, 2021 at 9:08 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Fri, Jun 04, 2021 at 08:33:45PM +0200, Matteo Croce wrote: > > > @@ -130,7 +137,10 @@ struct page { > > > }; > > > }; > > > struct { /* Tail pages of compound page */ > > > - unsigned long compound_head; /* Bit zero is set */ > > > + /* Bit zero is set > > > + * Bit one if pfmemalloc page > > > + */ > > > + unsigned long compound_head; > > > > I would drop this hunk. Bit 1 is not used for this purpose in tail > > pages; it's used for that purpose in head and base pages. > > > > I suppose we could do something like ... > > > > static inline void set_page_pfmemalloc(struct page *page) > > { > > - page->index = -1UL; > > + page->lru.next = (void *)2; > > } > > > > if it's causing confusion. > > > > If you prefer, ok for me. > Why not "(void *)BIT(1)"? Just to remark that it's a single bit and > not a magic like value? I don't have a strong preference. I'd use '2', but I wouldn't ask BIT(1) to be changed.
On Sat, Jun 5, 2021 at 4:32 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sat, Jun 05, 2021 at 12:59:50AM +0200, Matteo Croce wrote: > > On Fri, Jun 4, 2021 at 9:08 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Fri, Jun 04, 2021 at 08:33:45PM +0200, Matteo Croce wrote: > > > > @@ -130,7 +137,10 @@ struct page { > > > > }; > > > > }; > > > > struct { /* Tail pages of compound page */ > > > > - unsigned long compound_head; /* Bit zero is set */ > > > > + /* Bit zero is set > > > > + * Bit one if pfmemalloc page > > > > + */ > > > > + unsigned long compound_head; > > > > > > I would drop this hunk. Bit 1 is not used for this purpose in tail > > > pages; it's used for that purpose in head and base pages. > > > > > > I suppose we could do something like ... > > > > > > static inline void set_page_pfmemalloc(struct page *page) > > > { > > > - page->index = -1UL; > > > + page->lru.next = (void *)2; > > > } > > > > > > if it's causing confusion. > > > > > And change all the *_pfmemalloc functions to use page->lru.next like this? @@ -1668,10 +1668,12 @@ struct address_space *page_mapping(struct page *page); static inline bool page_is_pfmemalloc(const struct page *page) { /* - * Page index cannot be this large so this must be - * a pfmemalloc page. + * This is not a tail page; compound_head of a head page is unused + * at return from the page allocator, and will be overwritten + * by callers who do not care whether the page came from the + * reserves. */ - return page->index == -1UL; + return (uintptr_t)page->lru.next & BIT(1); } /* @@ -1680,12 +1682,12 @@ static inline bool page_is_pfmemalloc(const struct page *page) */ static inline void set_page_pfmemalloc(struct page *page) { - page->index = -1UL; + page->lru.next = (void *)BIT(1); } static inline void clear_page_pfmemalloc(struct page *page) { - page->index = 0; + page->lru.next = NULL; }
On Fri, Jun 04, 2021 at 08:41:52PM +0100, Matthew Wilcox wrote: > On Fri, Jun 04, 2021 at 08:33:47PM +0200, Matteo Croce wrote: > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index 7fcfea7e7b21..057b40ad29bd 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -40,6 +40,9 @@ > > #if IS_ENABLED(CONFIG_NF_CONNTRACK) > > #include <linux/netfilter/nf_conntrack_common.h> > > #endif > > +#ifdef CONFIG_PAGE_POOL > > +#include <net/page_pool.h> > > +#endif > > I'm not a huge fan of conditional includes ... any reason to not include > it always? I think we can. I'll check and change it. > > > @@ -3088,7 +3095,13 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f) > > */ > > static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle) > > { > > - put_page(skb_frag_page(frag)); > > + struct page *page = skb_frag_page(frag); > > + > > +#ifdef CONFIG_PAGE_POOL > > + if (recycle && page_pool_return_skb_page(page_address(page))) > > + return; > > It feels weird to have a page here, convert it back to an address, > then convert it back to a head page in page_pool_return_skb_page(). > How about passing 'page' here, calling compound_head() in > page_pool_return_skb_page() and calling virt_to_page() in skb_free_head()? > Sure, sounds reasonable. > > @@ -251,4 +253,11 @@ static inline void page_pool_ring_unlock(struct page_pool *pool) > > spin_unlock_bh(&pool->ring.producer_lock); > > } > > > > +/* Store mem_info on struct page and use it while recycling skb frags */ > > +static inline > > +void page_pool_store_mem_info(struct page *page, struct page_pool *pp) > > +{ > > + page->pp = pp; > > I'm not sure this wrapper needs to exist. > > > +} > > + > > #endif /* _NET_PAGE_POOL_H */ > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > index e1321bc9d316..a03f48f45696 100644 > > --- a/net/core/page_pool.c > > +++ b/net/core/page_pool.c > > @@ -628,3 +628,26 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid) > > } > > } > > EXPORT_SYMBOL(page_pool_update_nid); > > + > > +bool page_pool_return_skb_page(void *data) > > +{ > > + struct page_pool *pp; > > + struct page *page; > > + > > + page = virt_to_head_page(data); > > + if (unlikely(page->pp_magic != PP_SIGNATURE)) > > + return false; > > + > > + pp = (struct page_pool *)page->pp; > > You don't need the cast any more. > True > > + /* Driver set this to memory recycling info. Reset it on recycle. > > + * This will *not* work for NIC using a split-page memory model. > > + * The page will be returned to the pool here regardless of the > > + * 'flipped' fragment being in use or not. > > + */ > > + page->pp = NULL; > > + page_pool_put_full_page(pp, page, false); > > + > > + return true; > > +} > > +EXPORT_SYMBOL(page_pool_return_skb_page); > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 12b7e90dd2b5..f769f08e7b32 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -70,6 +70,9 @@ > > #include <net/xfrm.h> > > #include <net/mpls.h> > > #include <net/mptcp.h> > > +#ifdef CONFIG_PAGE_POOL > > +#include <net/page_pool.h> > > +#endif > > > > #include <linux/uaccess.h> > > #include <trace/events/skb.h> > > @@ -645,10 +648,15 @@ static void skb_free_head(struct sk_buff *skb) > > { > > unsigned char *head = skb->head; > > > > - if (skb->head_frag) > > + if (skb->head_frag) { > > +#ifdef CONFIG_PAGE_POOL > > + if (skb->pp_recycle && page_pool_return_skb_page(head)) > > + return; > > +#endif > > put this in a header file: > > static inline bool skb_pp_recycle(struct sk_buff *skb, void *data) > { > if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle) > return false; > return page_pool_return_skb_page(virt_to_page(data)); > } > > then this becomes: > > if (skb->head_frag) { > if (skb_pp_recycle(skb, head)) > return; > > skb_free_frag(head); > > - else > > + } else { > > kfree(head); > > + } > > } > > ok Thanks for having a look Cheers /Ilias
On Sun, Jun 06, 2021 at 03:50:54AM +0200, Matteo Croce wrote: > And change all the *_pfmemalloc functions to use page->lru.next like this? > > @@ -1668,10 +1668,12 @@ struct address_space *page_mapping(struct page *page); > static inline bool page_is_pfmemalloc(const struct page *page) > { > /* > - * Page index cannot be this large so this must be > - * a pfmemalloc page. > + * This is not a tail page; compound_head of a head page is unused > + * at return from the page allocator, and will be overwritten > + * by callers who do not care whether the page came from the > + * reserves. > */ The comment doesn't make a lot of sense if we're switching to use lru.next. How about: /* * lru.next has bit 1 set if the page is allocated from the * pfmemalloc reserves. Callers may simply overwrite it if * they do not need to preserve that information. */
On Mon, Jun 7, 2021 at 3:53 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Jun 06, 2021 at 03:50:54AM +0200, Matteo Croce wrote: > > And change all the *_pfmemalloc functions to use page->lru.next like this? > > > > @@ -1668,10 +1668,12 @@ struct address_space *page_mapping(struct page *page); > > static inline bool page_is_pfmemalloc(const struct page *page) > > { > > /* > > - * Page index cannot be this large so this must be > > - * a pfmemalloc page. > > + * This is not a tail page; compound_head of a head page is unused > > + * at return from the page allocator, and will be overwritten > > + * by callers who do not care whether the page came from the > > + * reserves. > > */ > > The comment doesn't make a lot of sense if we're switching to use > lru.next. How about: > > /* > * lru.next has bit 1 set if the page is allocated from the > * pfmemalloc reserves. Callers may simply overwrite it if > * they do not need to preserve that information. > */ Sounds good!
On Fri, Jun 4, 2021 at 9:42 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Jun 04, 2021 at 08:33:47PM +0200, Matteo Croce wrote: > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index 7fcfea7e7b21..057b40ad29bd 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -40,6 +40,9 @@ > > #if IS_ENABLED(CONFIG_NF_CONNTRACK) > > #include <linux/netfilter/nf_conntrack_common.h> > > #endif > > +#ifdef CONFIG_PAGE_POOL > > +#include <net/page_pool.h> > > +#endif > > I'm not a huge fan of conditional includes ... any reason to not include > it always? > Nope, I tried without the conditional on a system without CONFIG_PAGE_POOL and it compiles fine > > @@ -3088,7 +3095,13 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f) > > */ > > static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle) > > { > > - put_page(skb_frag_page(frag)); > > + struct page *page = skb_frag_page(frag); > > + > > +#ifdef CONFIG_PAGE_POOL > > + if (recycle && page_pool_return_skb_page(page_address(page))) > > + return; > > It feels weird to have a page here, convert it back to an address, > then convert it back to a head page in page_pool_return_skb_page(). > How about passing 'page' here, calling compound_head() in > page_pool_return_skb_page() and calling virt_to_page() in skb_free_head()? > I like it. > > @@ -251,4 +253,11 @@ static inline void page_pool_ring_unlock(struct page_pool *pool) > > spin_unlock_bh(&pool->ring.producer_lock); > > } > > > > +/* Store mem_info on struct page and use it while recycling skb frags */ > > +static inline > > +void page_pool_store_mem_info(struct page *page, struct page_pool *pp) > > +{ > > + page->pp = pp; > > I'm not sure this wrapper needs to exist. > I admit that this wrapper was bigger in the previous versions, but it's used by drivers which handle skb fragments (e.g. mvneta) to set the pointer for each frag. We can open code it, but it will be less straightforward. > > +} > > + > > #endif /* _NET_PAGE_POOL_H */ > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > index e1321bc9d316..a03f48f45696 100644 > > --- a/net/core/page_pool.c > > +++ b/net/core/page_pool.c > > @@ -628,3 +628,26 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid) > > } > > } > > EXPORT_SYMBOL(page_pool_update_nid); > > + > > +bool page_pool_return_skb_page(void *data) > > +{ > > + struct page_pool *pp; > > + struct page *page; > > + > > + page = virt_to_head_page(data); > > + if (unlikely(page->pp_magic != PP_SIGNATURE)) > > + return false; > > + > > + pp = (struct page_pool *)page->pp; > > You don't need the cast any more. > Right. > > + /* Driver set this to memory recycling info. Reset it on recycle. > > + * This will *not* work for NIC using a split-page memory model. > > + * The page will be returned to the pool here regardless of the > > + * 'flipped' fragment being in use or not. > > + */ > > + page->pp = NULL; > > + page_pool_put_full_page(pp, page, false); > > + > > + return true; > > +} > > +EXPORT_SYMBOL(page_pool_return_skb_page); > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 12b7e90dd2b5..f769f08e7b32 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -70,6 +70,9 @@ > > #include <net/xfrm.h> > > #include <net/mpls.h> > > #include <net/mptcp.h> > > +#ifdef CONFIG_PAGE_POOL > > +#include <net/page_pool.h> > > +#endif > > > > #include <linux/uaccess.h> > > #include <trace/events/skb.h> > > @@ -645,10 +648,15 @@ static void skb_free_head(struct sk_buff *skb) > > { > > unsigned char *head = skb->head; > > > > - if (skb->head_frag) > > + if (skb->head_frag) { > > +#ifdef CONFIG_PAGE_POOL > > + if (skb->pp_recycle && page_pool_return_skb_page(head)) > > + return; > > +#endif > > put this in a header file: > > static inline bool skb_pp_recycle(struct sk_buff *skb, void *data) > { > if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle) > return false; > return page_pool_return_skb_page(virt_to_page(data)); > } > > then this becomes: > > if (skb->head_frag) { > if (skb_pp_recycle(skb, head)) > return; > > skb_free_frag(head); > > - else > > + } else { > > kfree(head); > > + } > > } > > > > static void skb_release_data(struct sk_buff *skb) Done. I'll send a v8 soon.
From: Matteo Croce <mcroce@microsoft.com> This is a respin of [1] This patchset shows the plans for allowing page_pool to handle and maintain DMA map/unmap of the pages it serves to the driver. For this to work a return hook in the network core is introduced. The overall purpose is to simplify drivers, by providing a page allocation API that does recycling, such that each driver doesn't have to reinvent its own recycling scheme. Using page_pool in a driver does not require implementing XDP support, but it makes it trivially easy to do so. Instead of allocating buffers specifically for SKBs we now allocate a generic buffer and either wrap it on an SKB (via build_skb) or create an XDP frame. The recycling code leverages the XDP recycle APIs. The Marvell mvpp2 and mvneta drivers are used in this patchset to demonstrate how to use the API, and tested on a MacchiatoBIN and EspressoBIN boards respectively. Please let this going in on a future -rc1 so to allow enough time to have wider tests. Note that this series depends on the change "mm: fix struct page layout on 32-bit systems"[2] which is not yet in master. v6 -> v7: - refresh patches against net-next - remove a redundant call to virt_to_head_page() - update mvneta benchmarks v5 -> v6 - preserve pfmemalloc bit when setting signature - fix typo in mvneta - rebase on next-next with the new cache - don't clear the skb->pp_recycle in pskb_expand_head() v4 -> v5: - move the signature so it doesn't alias with page->mapping - use an invalid pointer as magic - incorporate Matthew Wilcox's changes for pfmemalloc pages - move the __skb_frag_unref() changes to a preliminary patch - refactor some cpp directives - only attempt recycling if skb->head_frag - clear skb->pp_recycle in pskb_expand_head() v3 -> v4: - store a pointer to page_pool instead of xdp_mem_info - drop a patch which reduces xdp_mem_info size - do the recycling in the page_pool code instead of xdp_return - remove some unused headers include - remove some useless forward declaration v2 -> v3: - added missing SOBs - CCed the MM people v1 -> v2: - fix a commit message - avoid setting pp_recycle multiple times on mvneta - squash two patches to avoid breaking bisect [1] https://lore.kernel.org/netdev/154413868810.21735.572808840657728172.stgit@firesoul/ [2] https://lore.kernel.org/linux-mm/20210510153211.1504886-1-willy@infradead.org/ Ilias Apalodimas (1): page_pool: Allow drivers to hint on SKB recycling Matteo Croce (4): mm: add a signature in struct page skbuff: add a parameter to __skb_frag_unref mvpp2: recycle buffers mvneta: recycle buffers drivers/net/ethernet/marvell/mvneta.c | 11 +++--- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +- drivers/net/ethernet/marvell/sky2.c | 2 +- drivers/net/ethernet/mellanox/mlx4/en_rx.c | 2 +- include/linux/mm.h | 12 ++++--- include/linux/mm_types.h | 12 ++++++- include/linux/poison.h | 3 ++ include/linux/skbuff.h | 34 ++++++++++++++++--- include/net/page_pool.h | 9 +++++ net/core/page_pool.c | 29 ++++++++++++++++ net/core/skbuff.c | 24 ++++++++++--- net/tls/tls_device.c | 2 +- 12 files changed, 119 insertions(+), 23 deletions(-)