[PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
packets are corrupted for the second and any further sockets bound to
the same umem. In other words, this does not affect the first socket
bound to the umem. The culprit for this bug is that the initialization
of the DMA addresses for the pre-populated xsk buffer pool entries was
not performed for any socket but the first one bound to the umem. Only
the linear array of DMA addresses was populated. Fix this by
populating the DMA addresses in the xsk buffer pool for every socket
bound to the same umem.

Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 net/xdp/xsk_buff_pool.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index f70112176b7c..9b09da63a7c3 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -379,6 +379,14 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
 
 static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
 {
+	u32 i;
+
+	for (i = 0; i < pool->heads_cnt; i++) {
+		struct xdp_buff_xsk *xskb = &pool->heads[i];
+
+		xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
+	}
+
 	pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
 	if (!pool->dma_pages)
 		return -ENOMEM;
@@ -428,12 +436,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
 
 	if (pool->unaligned)
 		xp_check_dma_contiguity(dma_map);
-	else
-		for (i = 0; i < pool->heads_cnt; i++) {
-			struct xdp_buff_xsk *xskb = &pool->heads[i];
-
-			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
-		}
 
 	err = xp_init_dma_info(pool, dma_map);
 	if (err) {

base-commit: 46c8229c4317ba8576a206d285a34783390ba6ab
-- 
2.34.1

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Alasdair McWilliam]

Thanks Magnus,

Results on E810 is mixed.

In terms of RSS:

Tested with 8 channels, 4 channels and 2 channels, processing a low volume of packets first (e.g. 8kpps, 4kpps, 2kpps per setup).

* On an 8 channel setup, it is usually channels 0-3 see traffic, but 4-7 do not. (I’ve since observed a test where queues 0-3 and queue 5 see packets, whereas queues 4 and 6-7 do not.

* On a 4 channel setup, I’ve had it where channels 0 and 3 see traffic, but 1 and 2 do not.

* On a 2 channel setup I can’t replicate the behaviour of only 1 channel getting traffic. Both always seem to work.

If a queue does not see any packets on a test run, I see what I think is a proportional increase on ethtool rx_dropped counter.

In a very low PPS test (e.g. 8 channels, 800pps) I can actually observe broadly the right proportion of traffic, to the queues that work. E.G. ~100pps up to the 4096 limit before stall. I don’t know if that’s good enough data to infer that RSS itself is working.

In terms of queue stalls:

Each queue seems to be able to process at most 4096 packets before stalling, and the ethtool rx_dropped counter starts incrementing. (It may have already started incrementing if the queue fails to work at all in the RSS notes above).

Interestingly, sometimes they stall slightly below this - I’ve observed 4091, 4093, 4095 etc. I’d have to do more repeated testing to try find the threshold before it stops servicing the queue, but once it reaches around 4096 or just below, it stops.

I can reproduce this even with a single queue.

Also worth noting this behaviour is seen with and without --poll and --busy-poll. It’s also observed if I don’t specify --zero-copy, but goes away if I force copy mode with --copy. Off the top of my head I’m not sure if the XDP infrastructure will default to XDP_ZEROCOPY if capable and XDP_COPY is not explicitly set on the bind flags though. But, putting it here anyway.


Results on MLX5 (hw is ConnectX-4 Lx)

The patch seems to have resolved issues on MLX and I can reliably rx/tx 2Mpps per queue with xdpsock_multi which is good news. (Same parameters as per testing on E810: poll, busy-poll, zero-copy).

I unfortunately cannot test beyond 2 queues in my current rig as I have to manually program flow-steering rules for 2 source MAC addresses into 2 different channels, and I only have 2 source ports on my load generator!

I could see if someone can generate a test pattern that comes from 8x source IPs, and flow steer each IP into its own queue, but I’m not sure this is useful re. the comment on RSS above. Let me know?

I will do some more thorough testing on the E810/ICE setup tomorrow to see if I can get a change in behaviour or observe what the exact stall threshold is.

Thanks
Alasdair



> On 11 Aug 2022, at 12:55, Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> 
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
> packets are corrupted for the second and any further sockets bound to
> the same umem. In other words, this does not affect the first socket
> bound to the umem. The culprit for this bug is that the initialization
> of the DMA addresses for the pre-populated xsk buffer pool entries was
> not performed for any socket but the first one bound to the umem. Only
> the linear array of DMA addresses was populated. Fix this by
> populating the DMA addresses in the xsk buffer pool for every socket
> bound to the same umem.
> 
> Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
> Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
> net/xdp/xsk_buff_pool.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index f70112176b7c..9b09da63a7c3 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -379,6 +379,14 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
> 
> static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
> {
> +	u32 i;
> +
> +	for (i = 0; i < pool->heads_cnt; i++) {
> +		struct xdp_buff_xsk *xskb = &pool->heads[i];
> +
> +		xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> +	}
> +
> 	pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
> 	if (!pool->dma_pages)
> 		return -ENOMEM;
> @@ -428,12 +436,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
> 
> 	if (pool->unaligned)
> 		xp_check_dma_contiguity(dma_map);
> -	else
> -		for (i = 0; i < pool->heads_cnt; i++) {
> -			struct xdp_buff_xsk *xskb = &pool->heads[i];
> -
> -			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> -		}
> 
> 	err = xp_init_dma_info(pool, dma_map);
> 	if (err) {
> 
> base-commit: 46c8229c4317ba8576a206d285a34783390ba6ab
> -- 
> 2.34.1
> 

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Maciej Fijalkowski]

On Thu, Aug 11, 2022 at 05:20:30PM +0000, Alasdair McWilliam wrote:
> Thanks Magnus,
> 
> Results on E810 is mixed.
> 
> In terms of RSS:
> 
> Tested with 8 channels, 4 channels and 2 channels, processing a low volume of packets first (e.g. 8kpps, 4kpps, 2kpps per setup).
> 
> * On an 8 channel setup, it is usually channels 0-3 see traffic, but 4-7 do not. (I’ve since observed a test where queues 0-3 and queue 5 see packets, whereas queues 4 and 6-7 do not.
> 
> * On a 4 channel setup, I’ve had it where channels 0 and 3 see traffic, but 1 and 2 do not.
> 
> * On a 2 channel setup I can’t replicate the behaviour of only 1 channel getting traffic. Both always seem to work.
> 
> If a queue does not see any packets on a test run, I see what I think is a proportional increase on ethtool rx_dropped counter.

Alasdair,

I have just sent patches and CCed you on them which should address the
issue you are observing. I will be grateful if you could test them on your
side and get back to us with results. If this won't help then we'll need
to dig this more.

> 
> In a very low PPS test (e.g. 8 channels, 800pps) I can actually observe broadly the right proportion of traffic, to the queues that work. E.G. ~100pps up to the 4096 limit before stall. I don’t know if that’s good enough data to infer that RSS itself is working.
> 
> In terms of queue stalls:
> 
> Each queue seems to be able to process at most 4096 packets before stalling, and the ethtool rx_dropped counter starts incrementing. (It may have already started incrementing if the queue fails to work at all in the RSS notes above).
> 
> Interestingly, sometimes they stall slightly below this - I’ve observed 4091, 4093, 4095 etc. I’d have to do more repeated testing to try find the threshold before it stops servicing the queue, but once it reaches around 4096 or just below, it stops.
> 
> I can reproduce this even with a single queue.
> 
> Also worth noting this behaviour is seen with and without --poll and --busy-poll. It’s also observed if I don’t specify --zero-copy, but goes away if I force copy mode with --copy. Off the top of my head I’m not sure if the XDP infrastructure will default to XDP_ZEROCOPY if capable and XDP_COPY is not explicitly set on the bind flags though. But, putting it here anyway.
> 

What ring size you're using?

Thanks,
Maciej

> 
> Results on MLX5 (hw is ConnectX-4 Lx)
> 
> The patch seems to have resolved issues on MLX and I can reliably rx/tx 2Mpps per queue with xdpsock_multi which is good news. (Same parameters as per testing on E810: poll, busy-poll, zero-copy).
> 
> I unfortunately cannot test beyond 2 queues in my current rig as I have to manually program flow-steering rules for 2 source MAC addresses into 2 different channels, and I only have 2 source ports on my load generator!
> 
> I could see if someone can generate a test pattern that comes from 8x source IPs, and flow steer each IP into its own queue, but I’m not sure this is useful re. the comment on RSS above. Let me know?
> 
> I will do some more thorough testing on the E810/ICE setup tomorrow to see if I can get a change in behaviour or observe what the exact stall threshold is.
> 
> Thanks
> Alasdair
> 
> 
> 
> > On 11 Aug 2022, at 12:55, Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> > 
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > 
> > Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
> > packets are corrupted for the second and any further sockets bound to
> > the same umem. In other words, this does not affect the first socket
> > bound to the umem. The culprit for this bug is that the initialization
> > of the DMA addresses for the pre-populated xsk buffer pool entries was
> > not performed for any socket but the first one bound to the umem. Only
> > the linear array of DMA addresses was populated. Fix this by
> > populating the DMA addresses in the xsk buffer pool for every socket
> > bound to the same umem.
> > 
> > Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
> > Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> > Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> > Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> > net/xdp/xsk_buff_pool.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > index f70112176b7c..9b09da63a7c3 100644
> > --- a/net/xdp/xsk_buff_pool.c
> > +++ b/net/xdp/xsk_buff_pool.c
> > @@ -379,6 +379,14 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
> > 
> > static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
> > {
> > +	u32 i;
> > +
> > +	for (i = 0; i < pool->heads_cnt; i++) {
> > +		struct xdp_buff_xsk *xskb = &pool->heads[i];
> > +
> > +		xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> > +	}
> > +
> > 	pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
> > 	if (!pool->dma_pages)
> > 		return -ENOMEM;
> > @@ -428,12 +436,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
> > 
> > 	if (pool->unaligned)
> > 		xp_check_dma_contiguity(dma_map);
> > -	else
> > -		for (i = 0; i < pool->heads_cnt; i++) {
> > -			struct xdp_buff_xsk *xskb = &pool->heads[i];
> > -
> > -			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> > -		}
> > 
> > 	err = xp_init_dma_info(pool, dma_map);
> > 	if (err) {
> > 
> > base-commit: 46c8229c4317ba8576a206d285a34783390ba6ab
> > -- 
> > 2.34.1
> > 
> 

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Magnus Karlsson]

On Thu, Aug 11, 2022 at 8:22 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Thu, Aug 11, 2022 at 05:20:30PM +0000, Alasdair McWilliam wrote:
> > Thanks Magnus,
> >
> > Results on E810 is mixed.
> >
> > In terms of RSS:
> >
> > Tested with 8 channels, 4 channels and 2 channels, processing a low volume of packets first (e.g. 8kpps, 4kpps, 2kpps per setup).
> >
> > * On an 8 channel setup, it is usually channels 0-3 see traffic, but 4-7 do not. (I’ve since observed a test where queues 0-3 and queue 5 see packets, whereas queues 4 and 6-7 do not.
> >
> > * On a 4 channel setup, I’ve had it where channels 0 and 3 see traffic, but 1 and 2 do not.
> >
> > * On a 2 channel setup I can’t replicate the behaviour of only 1 channel getting traffic. Both always seem to work.
> >
> > If a queue does not see any packets on a test run, I see what I think is a proportional increase on ethtool rx_dropped counter.
>
> Alasdair,
>
> I have just sent patches and CCed you on them which should address the
> issue you are observing. I will be grateful if you could test them on your
> side and get back to us with results. If this won't help then we'll need
> to dig this more.
>
> >
> > In a very low PPS test (e.g. 8 channels, 800pps) I can actually observe broadly the right proportion of traffic, to the queues that work. E.G. ~100pps up to the 4096 limit before stall. I don’t know if that’s good enough data to infer that RSS itself is working.
> >
> > In terms of queue stalls:
> >
> > Each queue seems to be able to process at most 4096 packets before stalling, and the ethtool rx_dropped counter starts incrementing. (It may have already started incrementing if the queue fails to work at all in the RSS notes above).
> >
> > Interestingly, sometimes they stall slightly below this - I’ve observed 4091, 4093, 4095 etc. I’d have to do more repeated testing to try find the threshold before it stops servicing the queue, but once it reaches around 4096 or just below, it stops.
> >
> > I can reproduce this even with a single queue.
> >
> > Also worth noting this behaviour is seen with and without --poll and --busy-poll. It’s also observed if I don’t specify --zero-copy, but goes away if I force copy mode with --copy. Off the top of my head I’m not sure if the XDP infrastructure will default to XDP_ZEROCOPY if capable and XDP_COPY is not explicitly set on the bind flags though. But, putting it here anyway.
> >
>
> What ring size you're using?
>
> Thanks,
> Maciej
>
> >
> > Results on MLX5 (hw is ConnectX-4 Lx)
> >
> > The patch seems to have resolved issues on MLX and I can reliably rx/tx 2Mpps per queue with xdpsock_multi which is good news. (Same parameters as per testing on E810: poll, busy-poll, zero-copy).
> >
> > I unfortunately cannot test beyond 2 queues in my current rig as I have to manually program flow-steering rules for 2 source MAC addresses into 2 different channels, and I only have 2 source ports on my load generator!
> >
> > I could see if someone can generate a test pattern that comes from 8x source IPs, and flow steer each IP into its own queue, but I’m not sure this is useful re. the comment on RSS above. Let me know?
> >
> > I will do some more thorough testing on the E810/ICE setup tomorrow to see if I can get a change in behaviour or observe what the exact stall threshold is.
> >
> > Thanks
> > Alasdair

Thank you so much for testing Alasdair. I believe your tests are
successful in terms of not getting corrupted packets anymore, which is
the only thing the patch I sent will fix. So we still have two
problems to fix for you:

1: RSS not working correctly for E810, which is what Maciej is
addressing in his patch set
2: The queue stalls after 4K packets.

I can take a look at #2 using the application you sent, but after
applying my patch and Maciej's.

> >
> >
> >
> > > On 11 Aug 2022, at 12:55, Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> > >
> > > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > >
> > > Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
> > > packets are corrupted for the second and any further sockets bound to
> > > the same umem. In other words, this does not affect the first socket
> > > bound to the umem. The culprit for this bug is that the initialization
> > > of the DMA addresses for the pre-populated xsk buffer pool entries was
> > > not performed for any socket but the first one bound to the umem. Only
> > > the linear array of DMA addresses was populated. Fix this by
> > > populating the DMA addresses in the xsk buffer pool for every socket
> > > bound to the same umem.
> > >
> > > Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
> > > Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> > > Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> > > Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > ---
> > > net/xdp/xsk_buff_pool.c | 14 ++++++++------
> > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > > index f70112176b7c..9b09da63a7c3 100644
> > > --- a/net/xdp/xsk_buff_pool.c
> > > +++ b/net/xdp/xsk_buff_pool.c
> > > @@ -379,6 +379,14 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
> > >
> > > static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
> > > {
> > > +   u32 i;
> > > +
> > > +   for (i = 0; i < pool->heads_cnt; i++) {
> > > +           struct xdp_buff_xsk *xskb = &pool->heads[i];
> > > +
> > > +           xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> > > +   }
> > > +

This for loop needs to be protected with an if (!pool->unaligned), but
I will not send out a new version here. It will be in the version sent
to the netdev mailing list.

> > >     pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
> > >     if (!pool->dma_pages)
> > >             return -ENOMEM;
> > > @@ -428,12 +436,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
> > >
> > >     if (pool->unaligned)
> > >             xp_check_dma_contiguity(dma_map);
> > > -   else
> > > -           for (i = 0; i < pool->heads_cnt; i++) {
> > > -                   struct xdp_buff_xsk *xskb = &pool->heads[i];
> > > -
> > > -                   xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> > > -           }
> > >
> > >     err = xp_init_dma_info(pool, dma_map);
> > >     if (err) {
> > >
> > > base-commit: 46c8229c4317ba8576a206d285a34783390ba6ab
> > > --
> > > 2.34.1
> > >
> >

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Alasdair McWilliam]

Maciej, Magnus,

>> I have just sent patches and CCed you on them which should address the
>> issue you are observing. I will be grateful if you could test them on your
>> side and get back to us with results. If this won't help then we'll need
>> to dig this more.

>> What ring size you're using?

I’ve applied both of those patches on top of the patch from Magnus the other day (including the if (!pool->unaligned) tweak below).

With both NIC tx and rx rings at 4096, using xdpsock_multi, all queues now seem to see process packets, which is good, but only up to ~4096 packets before stalling.

However, your additional question about ring size got me to try a tx/rx ring size of 2048 each, and it springs to life! 2Mpps l2fwd per queue on 8 queues with xdpsock_multi. Repeated with 4 queues, 2 queues etc and all looking good.

Seems ICE doesn’t like a 4096 ring size atm. :-)


> Thank you so much for testing Alasdair. I believe your tests are
> successful in terms of not getting corrupted packets anymore, which is
> the only thing the patch I sent will fix. So we still have two
> problems to fix for you:
> 
> 1: RSS not working correctly for E810, which is what Maciej is
> addressing in his patch set
> 2: The queue stalls after 4K packets.
> 
> I can take a look at #2 using the application you sent, but after
> applying my patch and Maciej's.


>>>> static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
>>>> {
>>>> +   u32 i;
>>>> +
>>>> +   for (i = 0; i < pool->heads_cnt; i++) {
>>>> +           struct xdp_buff_xsk *xskb = &pool->heads[i];
>>>> +
>>>> +           xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
>>>> +   }
>>>> +
> 
> This for loop needs to be protected with an if (!pool->unaligned), but
> I will not send out a new version here. It will be in the version sent
> to the netdev mailing list.

To confirm I applied this as well before applying Maciej’s patches.

Now we can now run xdpsock_multi with ring sizes at 2048 okay, I’ll spin up our software stack on this patched kernel to validate that side of things too.

Thank you both!!
Alasdair

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Maciej Fijalkowski]

On Fri, Aug 12, 2022 at 10:29:28AM +0000, Alasdair McWilliam wrote:
> Maciej, Magnus,
> 
> >> I have just sent patches and CCed you on them which should address the
> >> issue you are observing. I will be grateful if you could test them on your
> >> side and get back to us with results. If this won't help then we'll need
> >> to dig this more.
> 
> >> What ring size you're using?
> 
> I’ve applied both of those patches on top of the patch from Magnus the
> other day (including the if (!pool->unaligned) tweak below).
> 
> With both NIC tx and rx rings at 4096, using xdpsock_multi, all queues
> now seem to see process packets, which is good, but only up to ~4096
> packets before stalling.
> 
> However, your additional question about ring size got me to try a tx/rx
> ring size of 2048 each, and it springs to life! 2Mpps l2fwd per queue on
> 8 queues with xdpsock_multi. Repeated with 4 queues, 2 queues etc and
> all looking good.

Awesome!

> 
> Seems ICE doesn’t like a 4096 ring size atm. :-)

I have a fix for that, but please give me few days to clean this up. I
will be able to share it with you next week and I would really appreciate
if you could test this as well.

> 
> 
> > Thank you so much for testing Alasdair. I believe your tests are
> > successful in terms of not getting corrupted packets anymore, which is
> > the only thing the patch I sent will fix. So we still have two
> > problems to fix for you:
> > 
> > 1: RSS not working correctly for E810, which is what Maciej is
> > addressing in his patch set
> > 2: The queue stalls after 4K packets.
> > 
> > I can take a look at #2 using the application you sent, but after
> > applying my patch and Maciej's.
> 
> 
> >>>> static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
> >>>> {
> >>>> +   u32 i;
> >>>> +
> >>>> +   for (i = 0; i < pool->heads_cnt; i++) {
> >>>> +           struct xdp_buff_xsk *xskb = &pool->heads[i];
> >>>> +
> >>>> +           xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> >>>> +   }
> >>>> +
> > 
> > This for loop needs to be protected with an if (!pool->unaligned), but
> > I will not send out a new version here. It will be in the version sent
> > to the netdev mailing list.
> 
> To confirm I applied this as well before applying Maciej’s patches.
> 
> Now we can now run xdpsock_multi with ring sizes at 2048 okay, I’ll spin
> up our software stack on this patched kernel to validate that side of
> things too.

Glad to hear that! I'll ping you once I have a fix for 4k rings.

Thanks,
Maciej

> 
> Thank you both!!
> Alasdair
> 

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Alasdair McWilliam]

Hi,

Just to confirm the two patches provided also fixed our internal software as well.

I’ve seen the patches have made their way to the mainline code so thank you again.

Would you like us to test the 4k ring size fix still?

KR
Alasdair



> On 12 Aug 2022, at 11:37, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:
> 
> On Fri, Aug 12, 2022 at 10:29:28AM +0000, Alasdair McWilliam wrote:
>> Maciej, Magnus,
>> 
>>>> I have just sent patches and CCed you on them which should address the
>>>> issue you are observing. I will be grateful if you could test them on your
>>>> side and get back to us with results. If this won't help then we'll need
>>>> to dig this more.
>> 
>>>> What ring size you're using?
>> 
>> I’ve applied both of those patches on top of the patch from Magnus the
>> other day (including the if (!pool->unaligned) tweak below).
>> 
>> With both NIC tx and rx rings at 4096, using xdpsock_multi, all queues
>> now seem to see process packets, which is good, but only up to ~4096
>> packets before stalling.
>> 
>> However, your additional question about ring size got me to try a tx/rx
>> ring size of 2048 each, and it springs to life! 2Mpps l2fwd per queue on
>> 8 queues with xdpsock_multi. Repeated with 4 queues, 2 queues etc and
>> all looking good.
> 
> Awesome!
> 
>> 
>> Seems ICE doesn’t like a 4096 ring size atm. :-)
> 
> I have a fix for that, but please give me few days to clean this up. I
> will be able to share it with you next week and I would really appreciate
> if you could test this as well.
> 
>> 
>> 
>>> Thank you so much for testing Alasdair. I believe your tests are
>>> successful in terms of not getting corrupted packets anymore, which is
>>> the only thing the patch I sent will fix. So we still have two
>>> problems to fix for you:
>>> 
>>> 1: RSS not working correctly for E810, which is what Maciej is
>>> addressing in his patch set
>>> 2: The queue stalls after 4K packets.
>>> 
>>> I can take a look at #2 using the application you sent, but after
>>> applying my patch and Maciej's.
>> 
>> 
>>>>>> static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
>>>>>> {
>>>>>> +   u32 i;
>>>>>> +
>>>>>> +   for (i = 0; i < pool->heads_cnt; i++) {
>>>>>> +           struct xdp_buff_xsk *xskb = &pool->heads[i];
>>>>>> +
>>>>>> +           xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
>>>>>> +   }
>>>>>> +
>>> 
>>> This for loop needs to be protected with an if (!pool->unaligned), but
>>> I will not send out a new version here. It will be in the version sent
>>> to the netdev mailing list.
>> 
>> To confirm I applied this as well before applying Maciej’s patches.
>> 
>> Now we can now run xdpsock_multi with ring sizes at 2048 okay, I’ll spin
>> up our software stack on this patched kernel to validate that side of
>> things too.
> 
> Glad to hear that! I'll ping you once I have a fix for 4k rings.
> 
> Thanks,
> Maciej
> 
>> 
>> Thank you both!!
>> Alasdair

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Maciej Fijalkowski]

On Fri, Sep 09, 2022 at 10:15:22AM +0000, Alasdair McWilliam wrote:
> Hi,
> 
> Just to confirm the two patches provided also fixed our internal software as well.
> 
> I’ve seen the patches have made their way to the mainline code so thank you again.

Hey Alasdair,
Great!

> 
> Would you like us to test the 4k ring size fix still?

Yes, this would be much appreciated.
To do so, could you pick up the following:

https://lore.kernel.org/intel-wired-lan/20220901104040.15723-1-maciej.fijalkowski@intel.com/

Let us know of the result.
Maciej

> 
> KR
> Alasdair
> 
> 
> 
> > On 12 Aug 2022, at 11:37, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:
> > 
> > On Fri, Aug 12, 2022 at 10:29:28AM +0000, Alasdair McWilliam wrote:
> >> Maciej, Magnus,
> >> 
> >>>> I have just sent patches and CCed you on them which should address the
> >>>> issue you are observing. I will be grateful if you could test them on your
> >>>> side and get back to us with results. If this won't help then we'll need
> >>>> to dig this more.
> >> 
> >>>> What ring size you're using?
> >> 
> >> I’ve applied both of those patches on top of the patch from Magnus the
> >> other day (including the if (!pool->unaligned) tweak below).
> >> 
> >> With both NIC tx and rx rings at 4096, using xdpsock_multi, all queues
> >> now seem to see process packets, which is good, but only up to ~4096
> >> packets before stalling.
> >> 
> >> However, your additional question about ring size got me to try a tx/rx
> >> ring size of 2048 each, and it springs to life! 2Mpps l2fwd per queue on
> >> 8 queues with xdpsock_multi. Repeated with 4 queues, 2 queues etc and
> >> all looking good.
> > 
> > Awesome!
> > 
> >> 
> >> Seems ICE doesn’t like a 4096 ring size atm. :-)
> > 
> > I have a fix for that, but please give me few days to clean this up. I
> > will be able to share it with you next week and I would really appreciate
> > if you could test this as well.
> > 
> >> 
> >> 
> >>> Thank you so much for testing Alasdair. I believe your tests are
> >>> successful in terms of not getting corrupted packets anymore, which is
> >>> the only thing the patch I sent will fix. So we still have two
> >>> problems to fix for you:
> >>> 
> >>> 1: RSS not working correctly for E810, which is what Maciej is
> >>> addressing in his patch set
> >>> 2: The queue stalls after 4K packets.
> >>> 
> >>> I can take a look at #2 using the application you sent, but after
> >>> applying my patch and Maciej's.
> >> 
> >> 
> >>>>>> static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
> >>>>>> {
> >>>>>> +   u32 i;
> >>>>>> +
> >>>>>> +   for (i = 0; i < pool->heads_cnt; i++) {
> >>>>>> +           struct xdp_buff_xsk *xskb = &pool->heads[i];
> >>>>>> +
> >>>>>> +           xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> >>>>>> +   }
> >>>>>> +
> >>> 
> >>> This for loop needs to be protected with an if (!pool->unaligned), but
> >>> I will not send out a new version here. It will be in the version sent
> >>> to the netdev mailing list.
> >> 
> >> To confirm I applied this as well before applying Maciej’s patches.
> >> 
> >> Now we can now run xdpsock_multi with ring sizes at 2048 okay, I’ll spin
> >> up our software stack on this patched kernel to validate that side of
> >> things too.
> > 
> > Glad to hear that! I'll ping you once I have a fix for 4k rings.
> > 
> > Thanks,
> > Maciej
> > 
> >> 
> >> Thank you both!!
> >> Alasdair
> 

[PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
packets are corrupted for the second and any further sockets bound to
the same umem. In other words, this does not affect the first socket
bound to the umem. The culprit for this bug is that the initialization
of the DMA addresses for the pre-populated xsk buffer pool entries was
not performed for any socket but the first one bound to the umem. Only
the linear array of DMA addresses was populated. Fix this by
populating the DMA addresses in the xsk buffer pool for every socket
bound to the same umem.

Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
Reported-by: Intrusion Shield Team <dnevil@intrusion.com>
Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 net/xdp/xsk_buff_pool.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index f70112176b7c..a71a8c6edf55 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -379,6 +379,16 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
 
 static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
 {
+	if (!pool->unaligned) {
+		u32 i;
+
+		for (i = 0; i < pool->heads_cnt; i++) {
+			struct xdp_buff_xsk *xskb = &pool->heads[i];
+
+			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
+		}
+	}
+
 	pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
 	if (!pool->dma_pages)
 		return -ENOMEM;
@@ -428,12 +438,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
 
 	if (pool->unaligned)
 		xp_check_dma_contiguity(dma_map);
-	else
-		for (i = 0; i < pool->heads_cnt; i++) {
-			struct xdp_buff_xsk *xskb = &pool->heads[i];
-
-			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
-		}
 
 	err = xp_init_dma_info(pool, dma_map);
 	if (err) {

base-commit: 4e4588f1c4d2e67c993208f0550ef3fae33abce4
-- 
2.34.1

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[Maciej Fijalkowski]

On Fri, Aug 12, 2022 at 01:32:59PM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were

s/were/where

> packets are corrupted for the second and any further sockets bound to
> the same umem. In other words, this does not affect the first socket
> bound to the umem. The culprit for this bug is that the initialization
> of the DMA addresses for the pre-populated xsk buffer pool entries was
> not performed for any socket but the first one bound to the umem. Only
> the linear array of DMA addresses was populated. Fix this by
> populating the DMA addresses in the xsk buffer pool for every socket
> bound to the same umem.
> 
> Fixes: 94033cd8e73b8 ("xsk: Optimize for aligned case")
> Reported-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> Reported-by: Intrusion Shield Team <dnevil@intrusion.com>
> Tested-by: Alasdair McWilliam <alasdair.mcwilliam@outlook.com>
> Link: https://lore.kernel.org/xdp-newbies/6205E10C-292E-4995-9D10-409649354226@outlook.com/
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  net/xdp/xsk_buff_pool.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index f70112176b7c..a71a8c6edf55 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -379,6 +379,16 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
>  
>  static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
>  {
> +	if (!pool->unaligned) {
> +		u32 i;
> +
> +		for (i = 0; i < pool->heads_cnt; i++) {
> +			struct xdp_buff_xsk *xskb = &pool->heads[i];
> +
> +			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);

I wondered if it would be better to move it to the end of func and use
pool->dma_pages, but it probably doesn't matter in the end.

Great catch!
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> +		}
> +	}
> +
>  	pool->dma_pages = kvcalloc(dma_map->dma_pages_cnt, sizeof(*pool->dma_pages), GFP_KERNEL);
>  	if (!pool->dma_pages)
>  		return -ENOMEM;
> @@ -428,12 +438,6 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
>  
>  	if (pool->unaligned)
>  		xp_check_dma_contiguity(dma_map);
> -	else
> -		for (i = 0; i < pool->heads_cnt; i++) {
> -			struct xdp_buff_xsk *xskb = &pool->heads[i];
> -
> -			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
> -		}
>  
>  	err = xp_init_dma_info(pool, dma_map);
>  	if (err) {
> 
> base-commit: 4e4588f1c4d2e67c993208f0550ef3fae33abce4
> -- 
> 2.34.1
> 

Re: [PATCH bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 12 Aug 2022 13:32:59 +0200 you wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Fix an issue in XDP_SHARED_UMEM mode together with aligned mode were
> packets are corrupted for the second and any further sockets bound to
> the same umem. In other words, this does not affect the first socket
> bound to the umem. The culprit for this bug is that the initialization
> of the DMA addresses for the pre-populated xsk buffer pool entries was
> not performed for any socket but the first one bound to the umem. Only
> the linear array of DMA addresses was populated. Fix this by
> populating the DMA addresses in the xsk buffer pool for every socket
> bound to the same umem.
> 
> [...]

Here is the summary with links:
  - [bpf] xsk: fix corrupted packets for XDP_SHARED_UMEM
    https://git.kernel.org/bpf/bpf/c/58ca14ed98c8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html