enetc: unlock XDP_REDIRECT for XDP non-linear
Subsystems (3)
v3 cover enetc: unlock XDP_REDIRECT for XDP non-linear
Unlock XDP_REDIRECT for S/G XDP buffer and rely on XDP stack to properly take care of the frames.
Remove xdp_redirect_sg counter and the related ethtool entry since it is no longer used.
Changes since v2:
- remove xdp_redirect_sg ethtool counter
Changes since v1:
- drop Fixes tag
- unlock XDP_REDIRECT
- populate missing XDP metadata
Please note this patch is just compile tested
Lorenzo Bianconi (2):
net: ethernet: enetc: unlock XDP_REDIRECT for XDP non-linear buffers
net: ethernet: enetc: get rid of xdp_redirect_sg counter
drivers/net/ethernet/freescale/enetc/enetc.c | 25 ++++++++----------- drivers/net/ethernet/freescale/enetc/enetc.h | 1 - .../ethernet/freescale/enetc/enetc_ethtool.c | 2 -- 3 files changed, 10 insertions(+), 18 deletions(-)
Signature · 1 line
2.38.1
PATCH v3 1/2 net: ethernet: enetc: unlock XDP_REDIRECT for XDP non-linear buffers
Even if full XDP_REDIRECT is not supported yet for non-linear XDP buffers since we allow redirecting just into CPUMAPs, unlock XDP_REDIRECT for S/G XDP buffer and rely on XDP stack to properly take care of the frames.
--- drivers/net/ethernet/freescale/enetc/enetc.c | 25 ++++++++------------ 1 file changed, 10 insertions(+), 15 deletions(-)
drivers/net/ethernet/freescale/enetc/enetc.c +10 -15
@@ -1412,6 +1412,16 @@ static void enetc_add_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,/* To be used for XDP_TX */ rx_swbd->len = size; +if (!xdp_buff_has_frags (xdp_buff)) { +xdp_buff_set_frags_flag (xdp_buff); + shinfo->xdp_frags_size = size; + }else { + shinfo->xdp_frags_size += size; + } + +if (page_is_pfmemalloc (rx_swbd->page)) +xdp_buff_set_frag_pfmemalloc (xdp_buff); +skb_frag_off_set (frag, rx_swbd->page_offset);skb_frag_size_set (frag, size);__skb_frag_set_page (frag, rx_swbd->page);
@@ -1601,22 +1611,7 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, }break ;case XDP_REDIRECT : -/* xdp_return_frame does not support S/G in the sense -* that it leaks the fragments (__xdp_return should not -* call page_frag_free only for the initial buffer). -* Until XDP_REDIRECT gains support for S/G let's keep -* the code structure in place, but dead. We drop the -* S/G frames ourselves to avoid memory leaks which -* would otherwise leave the kernel OOM. -*/ -if (unlikely (cleaned_cnt - orig_cleaned_cnt !=1 )) { -enetc_xdp_drop (rx_ring, orig_i, i); - rx_ring->stats.xdp_redirect_sg++; -break ; - } - tmp_orig_i = orig_i; -while (orig_i != i) {enetc_flip_rx_buff (rx_ring, &rx_ring->rx_swbd[orig_i]);
Signature · 1 line
2.38.1
PATCH v3 2/2 net: ethernet: enetc: get rid of xdp_redirect_sg counter
In reply to Lorenzo BianconiRemove xdp_redirect_sg counter and the related ethtool entry since it is no longer used.
--- drivers/net/ethernet/freescale/enetc/enetc.h | 1 - drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 2 -- 2 files changed, 3 deletions(-)
drivers/net/ethernet/freescale/enetc/enetc.h +0 -1
@@ -70,7 +70,6 @@ struct enetc_ring_stats {unsigned int xdp_tx_drops;unsigned int xdp_redirect;unsigned int xdp_redirect_failures; -unsigned int xdp_redirect_sg;unsigned int recycles;unsigned int recycle_failures;unsigned int win_drop;
drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +0 -2
@@ -197,7 +197,6 @@ static const char rx_ring_stats[][ETH_GSTRING_LEN] = {"Rx ring %2d recycle failures" ,"Rx ring %2d redirects" , "Rx ring %2d redirect failures", - "Rx ring %2d redirect S/G", }; static const char tx_ring_stats[][ETH_GSTRING_LEN] = {
@@ -291,7 +290,6 @@ static void enetc_get_ethtool_stats(struct net_device *ndev, data[o++] = priv->rx_ring[i]->stats.recycle_failures; data[o++] = priv->rx_ring[i]->stats.xdp_redirect; data[o++] = priv->rx_ring[i]->stats.xdp_redirect_failures; - data[o++] = priv->rx_ring[i]->stats.xdp_redirect_sg; }if (!enetc_si_is_pf (priv->si))
Signature · 1 line
2.38.1
Quoted 14 lines · Lorenzo Bianconi
Unlock
XDP_REDIRECTfor S/G XDP buffer and rely on XDP stack to properly take care of the frames.
Removexdp_redirect_sgcounter and the related ethtool entry since it is no longer used.Changes since v2:
- removexdp_redirect_sgethtool counter
Changes since v1:
- drop Fixes tag
- unlockXDP_REDIRECT
- populate missing XDP metadataPlease note this patch is just compile tested
Quoted 8 lines
Lorenzo Bianconi (2):
net: ethernet: enetc: unlockXDP_REDIRECTfor XDP non-linear buffers
net: ethernet: enetc: get rid ofxdp_redirect_sgcounterdrivers/net/ethernet/freescale/enetc/enetc.c | 25 ++++++++----------- drivers/net/ethernet/freescale/enetc/enetc.h | 1 - .../ethernet/freescale/enetc/enetc_ethtool.c | 2 -- 3 files changed, 10 insertions(+), 18 deletions(-)
NACK.
xdp_redirect_cpu works, but OOM is still there if we XDP_REDIRECT to another interface. That needs to be solved first.
root@debian:~# ./bpf/xdp_redirect eno0 eno2 [ 313.613983] fsl_enetc 0000:00:00.0 eno0: Link is Down [ 313.699861] fsl_enetc 0000:00:00.0 eno0: PHY [0000:00:00.3:02] driver [Qualcomm Atheros AR8031/AR8033] (irq=POLL) [ 313.735530] fsl_enetc 0000:00:00.0 eno0: configuring for inband/sgmii link mode [ 313.754024] fsl_enetc 0000:00:00.2 eno2: Link is Down [ 313.798565] fsl_enetc 0000:00:00.2 eno2: configuring for fixed/internal link mode [ 313.806252] fsl_enetc 0000:00:00.2 eno2: Link is Up - 2.5Gbps/Full - flow control rx/tx Redirecting from eno0 (ifindex 6; driver fsl_enetc) to eno2 (ifindex 7; driver fsl_enetc) [ 315.791491] fsl_enetc 0000:00:00.0 eno0: Link is Up - 1Gbps/Full - flow control rx/tx [ 315.799451] IPv6: ADDRCONF(NETDEV_CHANGE): eno0: link becomes ready eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 19806 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81275 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 75733 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 1562 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s ^Z [1]+ Stopped ./nxp_board_rootfs/bpf/xdp_redirect eno0 eno2 [ 347.901643] bash invoked oom-killer: gfp_mask=0x40cc0(GFP_KERNEL|__GFP_COMP), order=0, oom_score_adj=0 [ 347.911254] CPU: 1 PID: 412 Comm: bash Not tainted 6.1.0-rc8-07010-ga9b9500ffaac-dirty #754 [ 347.919676] Hardware name: LS1028A RDB Board (DT) [ 347.924423] Call trace: [ 347.926901] dump_backtrace.part.0+0xe8/0xf4 [ 347.931223] show_stack+0x20/0x50 [ 347.934579] dump_stack_lvl+0x8c/0xb8 [ 347.938288] dump_stack+0x18/0x34 [ 347.941644] dump_header+0x50/0x2ec [ 347.945182] oom_kill_process+0x384/0x390 [ 347.949243] out_of_memory+0x218/0x670 [ 347.953039] __alloc_pages+0xf28/0x1080 [ 347.956919] cache_grow_begin+0x98/0x390 [ 347.960887] fallback_alloc+0x1f8/0x2bc [ 347.964765] ____cache_alloc_node+0x17c/0x194 [ 347.969168] kmem_cache_alloc+0x214/0x2d0 [ 347.973222] getname_flags.part.0+0x3c/0x1a4 [ 347.977536] getname_flags+0x4c/0x7c [ 347.981151] vfs_fstatat+0x4c/0x90 [ 347.984595] __do_sys_newfstatat+0x2c/0x70 [ 347.988737] __arm64_sys_newfstatat+0x28/0x34 [ 347.993140] invoke_syscall+0x50/0x120 [ 347.996939] el0_svc_common.constprop.0+0x68/0x124
Quoted 24 lines · Lorenzo Bianconi
Quoted 14 lines
Unlock
XDP_REDIRECTfor S/G XDP buffer and rely on XDP stack to properly take care of the frames.
Removexdp_redirect_sgcounter and the related ethtool entry since it is no longer used.Changes since v2:
- removexdp_redirect_sgethtool counter
Changes since v1:
- drop Fixes tag
- unlockXDP_REDIRECT
- populate missing XDP metadataPlease note this patch is just compile tested
Quoted 8 lines
Lorenzo Bianconi (2):
net: ethernet: enetc: unlockXDP_REDIRECTfor XDP non-linear buffers
net: ethernet: enetc: get rid ofxdp_redirect_sgcounterdrivers/net/ethernet/freescale/enetc/enetc.c | 25 ++++++++----------- drivers/net/ethernet/freescale/enetc/enetc.h | 1 - .../ethernet/freescale/enetc/enetc_ethtool.c | 2 -- 3 files changed, 10 insertions(+), 18 deletions(-)
Quoted 4 lines
NACK.
xdp_redirect_cpuworks, but OOM is still there if weXDP_REDIRECTto another interface. That needs to be solved first.
Hi Vladimir,
thx for testing. If we perform XDP_REDIRECT with SG XDP frames, the devmap code will always return an error and the driver is responsible to free the pending pages. Looking at the code, can the issue be the following?
- enetc_flip_rx_buff() will unmap the page and set rx_swbd->page = NULL if the page is not reusable.
- enetc_xdp_free() will not be able to free the page since rx_swbd->page is NULL.
What do you think? I am wondering if we have a similar issue for 'linear' XDP buffer as well when xdp_do_redirect() returns an error. What do you think?
Regards,
Lorenzo
Quoted 53 lines
root@debian:~# ./bpf/xdp_redirect eno0 eno2 [ 313.613983] fsl_enetc 0000:00:00.0 eno0: Link is Down [ 313.699861] fsl_enetc 0000:00:00.0 eno0: PHY [0000:00:00.3:02] driver [Qualcomm Atheros AR8031/AR8033] (irq=POLL) [ 313.735530] fsl_enetc 0000:00:00.0 eno0: configuring for inband/sgmii link mode [ 313.754024] fsl_enetc 0000:00:00.2 eno2: Link is Down [ 313.798565] fsl_enetc 0000:00:00.2 eno2: configuring for fixed/internal link mode [ 313.806252] fsl_enetc 0000:00:00.2 eno2: Link is Up - 2.5Gbps/Full - flow control rx/tx Redirecting from eno0 (ifindex 6; driver fsl_enetc) to eno2 (ifindex 7; driver fsl_enetc) [ 315.791491] fsl_enetc 0000:00:00.0 eno0: Link is Up - 1Gbps/Full - flow control rx/tx [ 315.799451] IPv6: ADDRCONF(NETDEV_CHANGE): eno0: link becomes ready eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 19806 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81275 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 81274 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 75733 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 1562 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s eno0->eno2 0 rx/s 0 err,drop/s 0 xmit/s ^Z [1]+ Stopped ./nxp_board_rootfs/bpf/xdp_redirect eno0 eno2 [ 347.901643] bash invoked oom-killer: gfp_mask=0x40cc0(GFP_KERNEL|__GFP_COMP), order=0, oom_score_adj=0 [ 347.911254] CPU: 1 PID: 412 Comm: bash Not tainted 6.1.0-rc8-07010-ga9b9500ffaac-dirty #754 [ 347.919676] Hardware name: LS1028A RDB Board (DT) [ 347.924423] Call trace: [ 347.926901] dump_backtrace.part.0+0xe8/0xf4 [ 347.931223] show_stack+0x20/0x50 [ 347.934579] dump_stack_lvl+0x8c/0xb8 [ 347.938288] dump_stack+0x18/0x34 [ 347.941644] dump_header+0x50/0x2ec [ 347.945182] oom_kill_process+0x384/0x390 [ 347.949243] out_of_memory+0x218/0x670 [ 347.953039] __alloc_pages+0xf28/0x1080 [ 347.956919] cache_grow_begin+0x98/0x390 [ 347.960887] fallback_alloc+0x1f8/0x2bc [ 347.964765] ____cache_alloc_node+0x17c/0x194 [ 347.969168] kmem_cache_alloc+0x214/0x2d0 [ 347.973222] getname_flags.part.0+0x3c/0x1a4 [ 347.977536] getname_flags+0x4c/0x7c [ 347.981151] vfs_fstatat+0x4c/0x90 [ 347.984595] __do_sys_newfstatat+0x2c/0x70 [ 347.988737] __arm64_sys_newfstatat+0x28/0x34 [ 347.993140] invoke_syscall+0x50/0x120 [ 347.996939] el0_svc_common.constprop.0+0x68/0x124
Vladimir Oltean
maintainerQuoted 6 lines · Lorenzo Bianconi
Hi Vladimir,
thx for testing. If we perform
XDP_REDIRECTwith SG XDP frames, the devmap code will always return an error and the driver is responsible to free the pending pages. Looking at the code, can the issue be the following?
Quoted 7 lines
-
enetc_flip_rx_buff()will unmap the page and setrx_swbd->page= NULL if the page is not reusable.
-enetc_xdp_free()will not be able to free the page sincerx_swbd->pageis NULL.What do you think? I am wondering if we have a similar issue for 'linear' XDP buffer as well when
xdp_do_redirect()returns an error. What do you think?
A bit more complicated, but that's the gist, yes. Thanks for the hint. I was quite sure that this situation does not lead to a leak, because even though rx_swbd->page becomes NULL, the reference to it is not lost. But wrong I was. Not sure if you pointed out the condition where the page is not reusable because that's the only part that's problematic, or because you simply didn't notice that enetc_put_rx_buff() makes rx_swbd->page = NULL too. In any case, it's normally quite rare for a page to not be reusable, yet in this case, the way in which the page becomes non reusable is the key to the bug.
Anyway, I've tested your patch set again with that fixed, and also submitted the fix here:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
It works as it should now. And yes, the issue should also be reproducible with single buffer XDP, if we redirect to a devmap which doesn't implement ndo_xdp_xmit or is down, for example.
Quoted 25 lines · Lorenzo Bianconi
Quoted 6 lines
Hi Vladimir,
thx for testing. If we perform
XDP_REDIRECTwith SG XDP frames, the devmap code will always return an error and the driver is responsible to free the pending pages. Looking at the code, can the issue be the following?Quoted 7 lines
-
enetc_flip_rx_buff()will unmap the page and setrx_swbd->page= NULL if the page is not reusable.
-enetc_xdp_free()will not be able to free the page sincerx_swbd->pageis NULL.What do you think? I am wondering if we have a similar issue for 'linear' XDP buffer as well when
xdp_do_redirect()returns an error. What do you think?A bit more complicated, but that's the gist, yes. Thanks for the hint. I was quite sure that this situation does not lead to a leak, because even though
rx_swbd->pagebecomes NULL, the reference to it is not lost. But wrong I was. Not sure if you pointed out the condition where the page is not reusable because that's the only part that's problematic, or because you simply didn't notice thatenetc_put_rx_buff()makesrx_swbd->page= NULL too. In any case, it's normally quite rare for a page to not be reusable, yet in this case, the way in which the page becomes non reusable is the key to the bug.
Quoted 7 lines
Anyway, I've tested your patch set again with that fixed, and also submitted the fix here:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/It works as it should now. And yes, the issue should also be reproducible with single buffer XDP, if we redirect to a devmap which doesn't implement
ndo_xdp_xmitor is down, for example.
ack, cool now it is fixed. I will repost the series when net-next is open adding your tested-by.
Regards,
Lorenzo