Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.
__xdp_build_skb_from_frame() missed the moment when the networking stack
became able to recycle skb pages backed by a page_pool. This was making
e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
also affected in some scenarios.
A lot of drivers use skb_mark_for_recycle() already, it's been almost
two years and seems like there are no issues in using it in the generic
code too. {__,}xdp_release_frame() can be then removed as it losts its
last user.
Page Pool becomes then zero-alloc (or almost) in the abovementioned
cases, too. Other memory type models (who needs them at this point)
have no changes.
Some numbers on 1 Xeon Platinum core bombed with 27 Mpps of 64-byte
IPv6 UDP, iavf w/XDP[0] (CONFIG_PAGE_POOL_STATS is enabled):
Plain %XDP_PASS on baseline, Page Pool driver:
src cpu Rx drops dst cpu Rx
2.1 Mpps N/A 2.1 Mpps
cpumap redirect (w/o leaving its node) on baseline:
6.8 Mpps 5.0 Mpps 1.8 Mpps
cpumap redirect with skb PP recycling:
7.9 Mpps 5.7 Mpps 2.2 Mpps
+22% (from cpumap redir on baseline)
[0] https://github.com/alobakin/linux/commits/iavf-xdp
Alexander Lobakin (3):
net: page_pool, skbuff: make skb_mark_for_recycle() always available
xdp: recycle Page Pool backed skbs built from XDP frames
xdp: remove unused {__,}xdp_release_frame()
include/linux/skbuff.h | 4 ++--
include/net/xdp.h | 29 -----------------------------
net/core/xdp.c | 19 ++-----------------
3 files changed, 4 insertions(+), 48 deletions(-)
---
From v1[1]:
* make skb_mark_for_recycle() always available, otherwise there are build
failures on non-PP systems (kbuild bot);
* 'Page Pool' -> 'page_pool' when it's about a page_pool instance, not
API (Jesper);
* expanded test system info a bit in the cover letter (Jesper).
[1] https://lore.kernel.org/bpf/[email protected]
--
2.39.2
__xdp_build_skb_from_frame() state(d):
/* Until page_pool get SKB return path, release DMA here */
Page Pool got skb pages recycling in April 2021, but missed this
function.
xdp_release_frame() is relevant only for Page Pool backed frames and it
detaches the page from the corresponding page_pool in order to make it
freeable via page_frag_free(). It can instead just mark the output skb
as eligible for recycling if the frame is backed by a pp. No change for
other memory model types (the same condition check as before).
cpumap redirect and veth on Page Pool drivers now become zero-alloc (or
almost).
Signed-off-by: Alexander Lobakin <[email protected]>
---
net/core/xdp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 8c92fc553317..a2237cfca8e9 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -658,8 +658,8 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
* - RX ring dev queue index (skb_record_rx_queue)
*/
- /* Until page_pool get SKB return path, release DMA here */
- xdp_release_frame(xdpf);
+ if (xdpf->mem.type == MEM_TYPE_PAGE_POOL)
+ skb_mark_for_recycle(skb);
/* Allow SKB to reuse area used by xdp_frame */
xdp_scrub_frame(xdpf);
--
2.39.2
__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 <[email protected]>
---
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);
- rcu_read_unlock();
-}
-EXPORT_SYMBOL_GPL(__xdp_release_frame);
-
void xdp_attachment_setup(struct xdp_attachment_info *info,
struct netdev_bpf *bpf)
{
--
2.39.2
From: Alexander Lobakin <[email protected]>
Date: Fri, 3 Mar 2023 14:32:29 +0100
> Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.
>
> __xdp_build_skb_from_frame() missed the moment when the networking stack
> became able to recycle skb pages backed by a page_pool. This was making
> e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
> also affected in some scenarios.
> A lot of drivers use skb_mark_for_recycle() already, it's been almost
> two years and seems like there are no issues in using it in the generic
> code too. {__,}xdp_release_frame() can be then removed as it losts its
> last user.
> Page Pool becomes then zero-alloc (or almost) in the abovementioned
> cases, too. Other memory type models (who needs them at this point)
> have no changes.
Ping?
The discussion in the v1 thread is unrelated to the patch subject :D
[...]
Thanks,
Olek
On 03/03/2023 14.32, Alexander Lobakin wrote:
> Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.
>
> __xdp_build_skb_from_frame() missed the moment when the networking stack
> became able to recycle skb pages backed by a page_pool. This was making
> e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
> also affected in some scenarios.
> A lot of drivers use skb_mark_for_recycle() already, it's been almost
> two years and seems like there are no issues in using it in the generic
> code too. {__,}xdp_release_frame() can be then removed as it losts its
> last user.
> Page Pool becomes then zero-alloc (or almost) in the abovementioned
> cases, too. Other memory type models (who needs them at this point)
> have no changes.
>
> Some numbers on 1 Xeon Platinum core bombed with 27 Mpps of 64-byte
> IPv6 UDP, iavf w/XDP[0] (CONFIG_PAGE_POOL_STATS is enabled):
>
> Plain %XDP_PASS on baseline, Page Pool driver:
>
> src cpu Rx drops dst cpu Rx
> 2.1 Mpps N/A 2.1 Mpps
>
> cpumap redirect (w/o leaving its node) on baseline:
What does it mean "without leaving its node" ?
I interpret this means BPF program CPU redirect to "same" CPU ?
Or does the "node" reference a NUMA node?
>
> 6.8 Mpps 5.0 Mpps 1.8 Mpps
>
> cpumap redirect with skb PP recycling:
Does this test use two CPUs?
>
> 7.9 Mpps 5.7 Mpps 2.2 Mpps
> +22% (from cpumap redir on baseline)
> [0] https://github.com/alobakin/linux/commits/iavf-xdp
>
> Alexander Lobakin (3):
> net: page_pool, skbuff: make skb_mark_for_recycle() always available
> xdp: recycle Page Pool backed skbs built from XDP frames
> xdp: remove unused {__,}xdp_release_frame()
>
> include/linux/skbuff.h | 4 ++--
> include/net/xdp.h | 29 -----------------------------
> net/core/xdp.c | 19 ++-----------------
> 3 files changed, 4 insertions(+), 48 deletions(-)
>
> ---
> From v1[1]:
> * make skb_mark_for_recycle() always available, otherwise there are build
> failures on non-PP systems (kbuild bot);
> * 'Page Pool' -> 'page_pool' when it's about a page_pool instance, not
> API (Jesper);
> * expanded test system info a bit in the cover letter (Jesper).
>
> [1] https://lore.kernel.org/bpf/[email protected]
From: Jesper Dangaard Brouer <[email protected]>
Date: Thu, 9 Mar 2023 17:43:51 +0100
[...]
>> Some numbers on 1 Xeon Platinum core bombed with 27 Mpps of 64-byte
>> IPv6 UDP, iavf w/XDP[0] (CONFIG_PAGE_POOL_STATS is enabled):
>>
>> Plain %XDP_PASS on baseline, Page Pool driver:
>>
>> src cpu Rx drops dst cpu Rx
>> 2.1 Mpps N/A 2.1 Mpps
>>
>> cpumap redirect (w/o leaving its node) on baseline:
>
> What does it mean "without leaving its node" ?
> I interpret this means BPF program CPU redirect to "same" CPU ?
> Or does the "node" reference a NUMA node?
Yes, NUMA node. It's a two-socket system. I redirect to a different
physical core, but within one NUMA node. When crossing nodes, results
usually are likely worse.
>
>>
>> 6.8 Mpps 5.0 Mpps 1.8 Mpps
>>
>> cpumap redirect with skb PP recycling:
>
> Does this test use two CPUs?
Yes, one serves interrupt / NAPI polling function and then redirects all
packets to a different core, which passes them up the stack.
These drops come from that the "source" CPU handles the queue much
faster than the "dest" one is able to process (no GRO on cpumap yet* +
software checksum computation + ...). Still faster than XDP_PASS when
one CPU does everything.
* well, there is cpumap GRO implementation in my repo, but without
hardware checksum status it's pretty useless and currently there's no
hints support in cpumap. So I didn't send it standalone (for now).
>
>>
>> 7.9 Mpps 5.7 Mpps 2.2 Mpps
>> +22% (from cpumap redir on baseline)
>> [0] https://github.com/alobakin/linux/commits/iavf-xdp
[...]
Thanks,
Olek
FYI
test_xdp_do_redirect:FAIL:pkt_count_zero unexpected pkt_count_zero:
actual 9936 != expected 2
see CI results.
It's a submitter job to monitor test results.
From: Alexei Starovoitov <[email protected]>
Date: Fri, 10 Mar 2023 10:33:39 -0800
> FYI
>
> test_xdp_do_redirect:FAIL:pkt_count_zero unexpected pkt_count_zero:
> actual 9936 != expected 2
>
> see CI results.
> It's a submitter job to monitor test results.
Yeah I saw it. Just for some reason I thought it's some CI problems,
like "what could possibly go wrong?" :clownface: Sorry >_<
The test assumes that only dropped pages get recycled, while this series
actually implements recycling for redirected ones as well. I'll dig into
this and adjust it on Monday. The code itself is fine :D
Thanks,
Olek