| Message ID | 20230313215553.1045175-5-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d4e492338d11937c55841b1279287280d6e35894 |
| Delegated to: | BPF |
| Headers | show |
| Series | xdp: recycle Page Pool backed skbs built from XDP frames | expand |
On 2023/3/14 5:55, Alexander Lobakin wrote: > __xdp_build_skb_from_frame() was the last user of > {__,}xdp_release_frame(), which detaches pages from the page_pool. > All the consumers now recycle Page Pool skbs and page, except mlx5, > stmmac and tsnep drivers, which use page_pool_release_page() directly > (might change one day). It's safe to assume this functionality is not > needed anymore and can be removed (in favor of recycling). > > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > include/net/xdp.h | 29 ----------------------------- > net/core/xdp.c | 15 --------------- > 2 files changed, 44 deletions(-) > > diff --git a/include/net/xdp.h b/include/net/xdp.h > index d517bfac937b..5393b3ebe56e 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -317,35 +317,6 @@ void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq); > void xdp_return_frame_bulk(struct xdp_frame *xdpf, > struct xdp_frame_bulk *bq); > > -/* When sending xdp_frame into the network stack, then there is no > - * return point callback, which is needed to release e.g. DMA-mapping > - * resources with page_pool. Thus, have explicit function to release > - * frame resources. > - */ > -void __xdp_release_frame(void *data, struct xdp_mem_info *mem); > -static inline void xdp_release_frame(struct xdp_frame *xdpf) > -{ > - struct xdp_mem_info *mem = &xdpf->mem; > - struct skb_shared_info *sinfo; > - int i; > - > - /* Curr only page_pool needs this */ > - if (mem->type != MEM_TYPE_PAGE_POOL) > - return; > - > - if (likely(!xdp_frame_has_frags(xdpf))) > - goto out; > - > - sinfo = xdp_get_shared_info_from_frame(xdpf); > - for (i = 0; i < sinfo->nr_frags; i++) { > - struct page *page = skb_frag_page(&sinfo->frags[i]); > - > - __xdp_release_frame(page_address(page), mem); > - } > -out: > - __xdp_release_frame(xdpf->data, mem); > -} > - > static __always_inline unsigned int xdp_get_frame_len(struct xdp_frame *xdpf) > { > struct skb_shared_info *sinfo; > diff --git a/net/core/xdp.c b/net/core/xdp.c > index a2237cfca8e9..8d3ad315f18d 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -531,21 +531,6 @@ void xdp_return_buff(struct xdp_buff *xdp) > } > EXPORT_SYMBOL_GPL(xdp_return_buff); > > -/* Only called for MEM_TYPE_PAGE_POOL see xdp.h */ > -void __xdp_release_frame(void *data, struct xdp_mem_info *mem) > -{ > - struct xdp_mem_allocator *xa; > - struct page *page; > - > - rcu_read_lock(); > - xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params); > - page = virt_to_head_page(data); > - if (xa) > - page_pool_release_page(xa->page_pool, page); page_pool_release_page() is only call here when xa is not NULL and mem->type == MEM_TYPE_PAGE_POOL. But skb_mark_for_recycle() is call when mem->type == MEM_TYPE_PAGE_POOL without checking xa, it does not seems symmetric to patch 3, if this is intended? > - rcu_read_unlock(); > -} > -EXPORT_SYMBOL_GPL(__xdp_release_frame); > - > void xdp_attachment_setup(struct xdp_attachment_info *info, > struct netdev_bpf *bpf) > { >
From: Yunsheng Lin <linyunsheng@huawei.com> Date: Tue, 14 Mar 2023 19:37:23 +0800 > On 2023/3/14 5:55, Alexander Lobakin wrote: >> __xdp_build_skb_from_frame() was the last user of >> {__,}xdp_release_frame(), which detaches pages from the page_pool. [...] >> -/* Only called for MEM_TYPE_PAGE_POOL see xdp.h */ >> -void __xdp_release_frame(void *data, struct xdp_mem_info *mem) >> -{ >> - struct xdp_mem_allocator *xa; >> - struct page *page; >> - >> - rcu_read_lock(); >> - xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params); >> - page = virt_to_head_page(data); >> - if (xa) >> - page_pool_release_page(xa->page_pool, page); > > page_pool_release_page() is only call here when xa is not NULL > and mem->type == MEM_TYPE_PAGE_POOL. > > But skb_mark_for_recycle() is call when mem->type == MEM_TYPE_PAGE_POOL > without checking xa, it does not seems symmetric to patch 3, if this is > intended? Intended. page_pool_return_skb_page() checks for %PP_SIGNATURE and if a page doesn't belong to any PP, it will be returned to the MM layer. Moreover, cases `mem->type == MEM_TYPE_PAGE_POOL && xa == NULL` are more of an exception rather than regular -- this means the page was released from its PP before reaching the function and IIRC it's even impossible with our current drivers. Adding a hashtable lookup to {__,}xdp_build_skb_from_frame() would only add hotpath overhead with no positive impact. > >> - rcu_read_unlock(); >> -} >> -EXPORT_SYMBOL_GPL(__xdp_release_frame); >> - >> void xdp_attachment_setup(struct xdp_attachment_info *info, >> struct netdev_bpf *bpf) >> { >> Thanks, Olek
diff --git a/include/net/xdp.h b/include/net/xdp.h index d517bfac937b..5393b3ebe56e 100644 --- a/include/net/xdp.h +++ b/include/net/xdp.h @@ -317,35 +317,6 @@ void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq); void xdp_return_frame_bulk(struct xdp_frame *xdpf, struct xdp_frame_bulk *bq); -/* When sending xdp_frame into the network stack, then there is no - * return point callback, which is needed to release e.g. DMA-mapping - * resources with page_pool. Thus, have explicit function to release - * frame resources. - */ -void __xdp_release_frame(void *data, struct xdp_mem_info *mem); -static inline void xdp_release_frame(struct xdp_frame *xdpf) -{ - struct xdp_mem_info *mem = &xdpf->mem; - struct skb_shared_info *sinfo; - int i; - - /* Curr only page_pool needs this */ - if (mem->type != MEM_TYPE_PAGE_POOL) - return; - - if (likely(!xdp_frame_has_frags(xdpf))) - goto out; - - sinfo = xdp_get_shared_info_from_frame(xdpf); - for (i = 0; i < sinfo->nr_frags; i++) { - struct page *page = skb_frag_page(&sinfo->frags[i]); - - __xdp_release_frame(page_address(page), mem); - } -out: - __xdp_release_frame(xdpf->data, mem); -} - static __always_inline unsigned int xdp_get_frame_len(struct xdp_frame *xdpf) { struct skb_shared_info *sinfo; diff --git a/net/core/xdp.c b/net/core/xdp.c index a2237cfca8e9..8d3ad315f18d 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -531,21 +531,6 @@ void xdp_return_buff(struct xdp_buff *xdp) } EXPORT_SYMBOL_GPL(xdp_return_buff); -/* Only called for MEM_TYPE_PAGE_POOL see xdp.h */ -void __xdp_release_frame(void *data, struct xdp_mem_info *mem) -{ - struct xdp_mem_allocator *xa; - struct page *page; - - rcu_read_lock(); - xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params); - page = virt_to_head_page(data); - if (xa) - page_pool_release_page(xa->page_pool, page); - rcu_read_unlock(); -} -EXPORT_SYMBOL_GPL(__xdp_release_frame); - void xdp_attachment_setup(struct xdp_attachment_info *info, struct netdev_bpf *bpf) {
__xdp_build_skb_from_frame() was the last user of {__,}xdp_release_frame(), which detaches pages from the page_pool. All the consumers now recycle Page Pool skbs and page, except mlx5, stmmac and tsnep drivers, which use page_pool_release_page() directly (might change one day). It's safe to assume this functionality is not needed anymore and can be removed (in favor of recycling). Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- include/net/xdp.h | 29 ----------------------------- net/core/xdp.c | 15 --------------- 2 files changed, 44 deletions(-)