| Message ID | 20210521161527.34607-4-mcroce@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | page_pool: recycle buffers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
On Fri, May 21, 2021 at 6:16 PM Matteo Croce <mcroce@linux.microsoft.com> wrote: > +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; > + > + /* 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, virt_to_head_page(data), false); Here I could just use the cached "page" instead of calling virt_to_head_page() once again.
On 2021/5/22 0:15, Matteo Croce wrote: > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Up to now several high speed NICs have custom mechanisms of recycling > the allocated memory they use for their payloads. > Our page_pool API already has recycling capabilities that are always > used when we are running in 'XDP mode'. So let's tweak the API and the > kernel network stack slightly and allow the recycling to happen even > during the standard operation. > The API doesn't take into account 'split page' policies used by those > drivers currently, but can be extended once we have users for that. > > The idea is to be able to intercept the packet on skb_release_data(). > If it's a buffer coming from our page_pool API recycle it back to the > pool for further usage or just release the packet entirely. > > To achieve that we introduce a bit in struct sk_buff (pp_recycle:1) and > a field in struct page (page->pp) to store the page_pool pointer. > Storing the information in page->pp allows us to recycle both SKBs and > their fragments. > We could have skipped the skb bit entirely, since identical information > can bederived from struct page. However, in an effort to affect the free path > as less as possible, reading a single bit in the skb which is already > in cache, is better that trying to derive identical information for the > page stored data. > > The driver or page_pool has to take care of the sync operations on it's own > during the buffer recycling since the buffer is, after opting-in to the > recycling, never unmapped. > > Since the gain on the drivers depends on the architecture, we are not > enabling recycling by default if the page_pool API is used on a driver. > In order to enable recycling the driver must call skb_mark_for_recycle() > to store the information we need for recycling in page->pp and > enabling the recycling bit, or page_pool_store_mem_info() for a fragment. The state of this patch in patchwork is "Not Applicable", so you may need to respin it again. Some minor comment below: > > Co-developed-by: Jesper Dangaard Brouer <brouer@redhat.com> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > Co-developed-by: Matteo Croce <mcroce@microsoft.com> > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > include/linux/skbuff.h | 28 +++++++++++++++++++++++++--- > include/net/page_pool.h | 9 +++++++++ > net/core/page_pool.c | 23 +++++++++++++++++++++++ > net/core/skbuff.c | 24 ++++++++++++++++++++---- > 4 files changed, 77 insertions(+), 7 deletions(-) > > 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 > > /* The interface for checksum offload between the stack and networking drivers > * is as follows... > @@ -667,6 +670,8 @@ typedef unsigned char *sk_buff_data_t; > * @head_frag: skb was allocated from page fragments, > * not allocated by kmalloc() or vmalloc(). > * @pfmemalloc: skbuff was allocated from PFMEMALLOC reserves > + * @pp_recycle: mark the packet for recycling instead of freeing (implies > + * page_pool support on driver) > * @active_extensions: active extensions (skb_ext_id types) > * @ndisc_nodetype: router type (from link layer) > * @ooo_okay: allow the mapping of a socket to a queue to be changed > @@ -791,10 +796,12 @@ struct sk_buff { > fclone:2, > peeked:1, > head_frag:1, > - pfmemalloc:1; > + pfmemalloc:1, > + pp_recycle:1; /* page_pool recycle indicator */ The about comment seems unnecessary, for there is comment added above in this patch to explain that. > #ifdef CONFIG_SKB_EXTENSIONS > __u8 active_extensions; > #endif > + Unnecessary change? > /* fields enclosed in headers_start/headers_end are copied > * using a single memcpy() in __copy_skb_header() > */ > @@ -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; > +#endif > + put_page(page); > } > > /** > @@ -3100,7 +3113,7 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle) > */ > static inline void skb_frag_unref(struct sk_buff *skb, int f) > { > - __skb_frag_unref(&skb_shinfo(skb)->frags[f], false); > + __skb_frag_unref(&skb_shinfo(skb)->frags[f], skb->pp_recycle); > } > > /** > @@ -4699,5 +4712,14 @@ static inline u64 skb_get_kcov_handle(struct sk_buff *skb) > #endif > } > > +#ifdef CONFIG_PAGE_POOL > +static inline void skb_mark_for_recycle(struct sk_buff *skb, struct page *page, > + struct page_pool *pp) > +{ > + skb->pp_recycle = 1; > + page_pool_store_mem_info(page, pp); > +} > +#endif > + > #endif /* __KERNEL__ */ > #endif /* _LINUX_SKBUFF_H */ > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index b4b6de909c93..7b9b6a1c61f5 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -146,6 +146,8 @@ inline enum dma_data_direction page_pool_get_dma_dir(struct page_pool *pool) > return pool->p.dma_dir; > } > > +bool page_pool_return_skb_page(void *data); > + > struct page_pool *page_pool_create(const struct page_pool_params *params); > > #ifdef CONFIG_PAGE_POOL > @@ -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) The conventional practice seems to put "struct page_pool" before other parameter in page_pool.h. > +{ > + page->pp = pp; > +} > + > #endif /* _NET_PAGE_POOL_H */ > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index e1321bc9d316..2a020cca489f 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; > + > + /* 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. > + */ I am not sure I understand how does the last part of comment related to the code below, as there is no driver using split-page memory model will reach here because those driver will not call skb_mark_for_recycle(), right? > + page->pp = NULL; > + page_pool_put_full_page(pp, virt_to_head_page(data), false); > + > + return true; > +} > +EXPORT_SYMBOL(page_pool_return_skb_page);
[...] > > + /* 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. > > + */ > > I am not sure I understand how does the last part of comment related > to the code below, as there is no driver using split-page memory model > will reach here because those driver will not call skb_mark_for_recycle(), > right? > Yes the comment is there to prohibit people (mlx5 only actually) to add the recycling bit on their driver. Because if they do it will *probably* work but they might get random corrupted packets which will be hard to debug. > > + page->pp = NULL; > > + page_pool_put_full_page(pp, virt_to_head_page(data), false); > > + > > + return true; > > +} > > +EXPORT_SYMBOL(page_pool_return_skb_page); >
On 6/4/21 2:42 AM, Ilias Apalodimas wrote: > [...] >>> + /* 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. >>> + */ >> >> I am not sure I understand how does the last part of comment related >> to the code below, as there is no driver using split-page memory model >> will reach here because those driver will not call skb_mark_for_recycle(), >> right? >> > > Yes the comment is there to prohibit people (mlx5 only actually) to add the > recycling bit on their driver. Because if they do it will *probably* work > but they might get random corrupted packets which will be hard to debug. > What's the complexity for getting it to work with split page model? Since 1500 is the default MTU, requiring a page per packet means a lot of wasted memory.
On Sat, Jun 5, 2021 at 6:06 PM David Ahern <dsahern@gmail.com> wrote: > > On 6/4/21 2:42 AM, Ilias Apalodimas wrote: > > [...] > >>> + /* 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. > >>> + */ > >> > >> I am not sure I understand how does the last part of comment related > >> to the code below, as there is no driver using split-page memory model > >> will reach here because those driver will not call skb_mark_for_recycle(), > >> right? > >> > > > > Yes the comment is there to prohibit people (mlx5 only actually) to add the > > recycling bit on their driver. Because if they do it will *probably* work > > but they might get random corrupted packets which will be hard to debug. > > > > What's the complexity for getting it to work with split page model? > Since 1500 is the default MTU, requiring a page per packet means a lot > of wasted memory. We could create a new memory model, e.g. MEM_TYPE_PAGE_SPLIT, and restore the behavior present in the previous versions of this serie, which is, save xdp_mem_info in struct page. As this could slightly impact the performances, this can be added in a future change when the drivers which are doing it want to use this recycling api.
On 6/5/2021 7:34 PM, Matteo Croce wrote: > On Sat, Jun 5, 2021 at 6:06 PM David Ahern <dsahern@gmail.com> wrote: >> >> On 6/4/21 2:42 AM, Ilias Apalodimas wrote: >>> [...] >>>>> + /* 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. >>>>> + */ >>>> >>>> I am not sure I understand how does the last part of comment related >>>> to the code below, as there is no driver using split-page memory model >>>> will reach here because those driver will not call skb_mark_for_recycle(), >>>> right? >>>> >>> >>> Yes the comment is there to prohibit people (mlx5 only actually) to add the >>> recycling bit on their driver. Because if they do it will *probably* work >>> but they might get random corrupted packets which will be hard to debug. >>> >> >> What's the complexity for getting it to work with split page model? >> Since 1500 is the default MTU, requiring a page per packet means a lot >> of wasted memory. > > We could create a new memory model, e.g. MEM_TYPE_PAGE_SPLIT, and > restore the behavior present in the previous versions of this serie, > which is, save xdp_mem_info in struct page. > As this could slightly impact the performances, this can be added in a > future change when the drivers which are doing it want to use this > recycling api. > page-split model doesn't only help reduce memory waste, but increase cache-locality, especially for aggregated GRO SKBs. I'm looking forward to integrating the page-pool SKB recycling API into mlx5e datapath. For this we need it to support the page-split model. Let's see what's missing and how we can help making this happen. Regards, Tariq
Hi David, On Sat, Jun 05, 2021 at 10:06:30AM -0600, David Ahern wrote: > On 6/4/21 2:42 AM, Ilias Apalodimas wrote: > > [...] > >>> + /* 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. > >>> + */ > >> > >> I am not sure I understand how does the last part of comment related > >> to the code below, as there is no driver using split-page memory model > >> will reach here because those driver will not call skb_mark_for_recycle(), > >> right? > >> > > > > Yes the comment is there to prohibit people (mlx5 only actually) to add the > > recycling bit on their driver. Because if they do it will *probably* work > > but they might get random corrupted packets which will be hard to debug. > > > > What's the complexity for getting it to work with split page model? > Since 1500 is the default MTU, requiring a page per packet means a lot > of wasted memory. It boils down to 'can we re-use the page or is someone using it'. Yunsheng sent a patch in earlier series that implements this with ref counters. As Matteo mentions we can also add another page pool type. In theory none of those sound too hard, but we'll have to code it and see. /Ilias
Hi Tariq, > > > > > > > > Yes the comment is there to prohibit people (mlx5 only actually) to add the > > > > recycling bit on their driver. Because if they do it will *probably* work > > > > but they might get random corrupted packets which will be hard to debug. > > > > > > > > > > What's the complexity for getting it to work with split page model? > > > Since 1500 is the default MTU, requiring a page per packet means a lot > > > of wasted memory. > > > > We could create a new memory model, e.g. MEM_TYPE_PAGE_SPLIT, and > > restore the behavior present in the previous versions of this serie, > > which is, save xdp_mem_info in struct page. > > As this could slightly impact the performances, this can be added in a > > future change when the drivers which are doing it want to use this > > recycling api. > > > > page-split model doesn't only help reduce memory waste, but increase > cache-locality, especially for aggregated GRO SKBs. > > I'm looking forward to integrating the page-pool SKB recycling API into > mlx5e datapath. For this we need it to support the page-split model. > > Let's see what's missing and how we can help making this happen. Yes that's the final goal. As I said I don't think adding the page split model will fundamentally change the current patchset. So imho we should get this in first, make sure that everything is fine, and then add code for the mlx cards. Regards /Ilias
On 6/7/2021 7:38 AM, Ilias Apalodimas wrote: > Hi Tariq, > >>>>> >>>>> Yes the comment is there to prohibit people (mlx5 only actually) to add the >>>>> recycling bit on their driver. Because if they do it will *probably* work >>>>> but they might get random corrupted packets which will be hard to debug. >>>>> >>>> >>>> What's the complexity for getting it to work with split page model? >>>> Since 1500 is the default MTU, requiring a page per packet means a lot >>>> of wasted memory. >>> >>> We could create a new memory model, e.g. MEM_TYPE_PAGE_SPLIT, and >>> restore the behavior present in the previous versions of this serie, >>> which is, save xdp_mem_info in struct page. >>> As this could slightly impact the performances, this can be added in a >>> future change when the drivers which are doing it want to use this >>> recycling api. >>> >> >> page-split model doesn't only help reduce memory waste, but increase >> cache-locality, especially for aggregated GRO SKBs. >> >> I'm looking forward to integrating the page-pool SKB recycling API into >> mlx5e datapath. For this we need it to support the page-split model. >> >> Let's see what's missing and how we can help making this happen. > > Yes that's the final goal. As I said I don't think adding the page split > model will fundamentally change the current patchset. So imho we should > get this in first, make sure that everything is fine, and then add code for > the mlx cards. > Sounds good
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 /* The interface for checksum offload between the stack and networking drivers * is as follows... @@ -667,6 +670,8 @@ typedef unsigned char *sk_buff_data_t; * @head_frag: skb was allocated from page fragments, * not allocated by kmalloc() or vmalloc(). * @pfmemalloc: skbuff was allocated from PFMEMALLOC reserves + * @pp_recycle: mark the packet for recycling instead of freeing (implies + * page_pool support on driver) * @active_extensions: active extensions (skb_ext_id types) * @ndisc_nodetype: router type (from link layer) * @ooo_okay: allow the mapping of a socket to a queue to be changed @@ -791,10 +796,12 @@ struct sk_buff { fclone:2, peeked:1, head_frag:1, - pfmemalloc:1; + pfmemalloc:1, + pp_recycle:1; /* page_pool recycle indicator */ #ifdef CONFIG_SKB_EXTENSIONS __u8 active_extensions; #endif + /* fields enclosed in headers_start/headers_end are copied * using a single memcpy() in __copy_skb_header() */ @@ -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; +#endif + put_page(page); } /** @@ -3100,7 +3113,7 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle) */ static inline void skb_frag_unref(struct sk_buff *skb, int f) { - __skb_frag_unref(&skb_shinfo(skb)->frags[f], false); + __skb_frag_unref(&skb_shinfo(skb)->frags[f], skb->pp_recycle); } /** @@ -4699,5 +4712,14 @@ static inline u64 skb_get_kcov_handle(struct sk_buff *skb) #endif } +#ifdef CONFIG_PAGE_POOL +static inline void skb_mark_for_recycle(struct sk_buff *skb, struct page *page, + struct page_pool *pp) +{ + skb->pp_recycle = 1; + page_pool_store_mem_info(page, pp); +} +#endif + #endif /* __KERNEL__ */ #endif /* _LINUX_SKBUFF_H */ diff --git a/include/net/page_pool.h b/include/net/page_pool.h index b4b6de909c93..7b9b6a1c61f5 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -146,6 +146,8 @@ inline enum dma_data_direction page_pool_get_dma_dir(struct page_pool *pool) return pool->p.dma_dir; } +bool page_pool_return_skb_page(void *data); + struct page_pool *page_pool_create(const struct page_pool_params *params); #ifdef CONFIG_PAGE_POOL @@ -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; +} + #endif /* _NET_PAGE_POOL_H */ diff --git a/net/core/page_pool.c b/net/core/page_pool.c index e1321bc9d316..2a020cca489f 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; + + /* 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, virt_to_head_page(data), 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 skb_free_frag(head); - else + } else { kfree(head); + } } static void skb_release_data(struct sk_buff *skb) @@ -664,7 +672,7 @@ static void skb_release_data(struct sk_buff *skb) skb_zcopy_clear(skb, true); for (i = 0; i < shinfo->nr_frags; i++) - __skb_frag_unref(&shinfo->frags[i], false); + __skb_frag_unref(&shinfo->frags[i], skb->pp_recycle); if (shinfo->frag_list) kfree_skb_list(shinfo->frag_list); @@ -1046,6 +1054,7 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb) n->nohdr = 0; n->peeked = 0; C(pfmemalloc); + C(pp_recycle); n->destructor = NULL; C(tail); C(end); @@ -3495,7 +3504,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen) fragto = &skb_shinfo(tgt)->frags[merge]; skb_frag_size_add(fragto, skb_frag_size(fragfrom)); - __skb_frag_unref(fragfrom, false); + __skb_frag_unref(fragfrom, skb->pp_recycle); } /* Reposition in the original skb */ @@ -5285,6 +5294,13 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from, if (skb_cloned(to)) return false; + /* The page pool signature of struct page will eventually figure out + * which pages can be recycled or not but for now let's prohibit slab + * allocated and page_pool allocated SKBs from being coalesced. + */ + if (to->pp_recycle != from->pp_recycle) + return false; + if (len <= skb_tailroom(to)) { if (len) BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));