[PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[Lorenzo Bianconi]

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(-)

-- 
2.38.1

[PATCH v3 net-next 1/2] net: ethernet: enetc: unlock XDP_REDIRECT for XDP non-linear buffers

[Lorenzo Bianconi]

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.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 25 ++++++++------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 8671591cb750..cd8f5f0c6b54 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -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]);
-- 
2.38.1

[PATCH v3 net-next 2/2] net: ethernet: enetc: get rid of xdp_redirect_sg counter

[Lorenzo Bianconi]

Remove xdp_redirect_sg counter and the related ethtool entry since it is
no longer used.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/freescale/enetc/enetc.h         | 1 -
 drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index c6d8cc15c270..416e4138dbaf 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -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;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index c8369e3752b0..d45f305eb03c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -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))
-- 
2.38.1

Re: [PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[Vladimir Oltean]

On Sat, Dec 10, 2022 at 02:53:09PM +0100, Lorenzo Bianconi wrote:
> 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(-)

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

Re: [PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5262 bytes --]

> On Sat, Dec 10, 2022 at 02:53:09PM +0100, Lorenzo Bianconi wrote:
> > 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(-)
> 
> NACK.
> 
> xdp_redirect_cpu works, but OOM is still there if we XDP_REDIRECT to
> 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

> 
> 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[Vladimir Oltean]

On Mon, Dec 12, 2022 at 10:15:31PM +0100, Lorenzo Bianconi wrote:
> 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?

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/20221213001908.2347046-1-vladimir.oltean@nxp.com/

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.

Re: [PATCH v3 net-next 0/2] enetc: unlock XDP_REDIRECT for XDP non-linear

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]

> On Mon, Dec 12, 2022 at 10:15:31PM +0100, Lorenzo Bianconi wrote:
> > 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?
> 
> 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/20221213001908.2347046-1-vladimir.oltean@nxp.com/
> 
> 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.

ack, cool now it is fixed. I will repost the series when net-next is open
adding your tested-by.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]