[bpf] xsk: fix batch alloc API on non-coherent systems

Commit Message

Fijalkowski, Maciej Sept. 11, 2024, 7:10 p.m. UTC

In cases when synchronizing DMA operations is necessary,
xsk_buff_alloc_batch() returns a single buffer instead of the requested
count. This puts the pressure on drivers that use batch API as they have
to check for this corner case on their side and take care of allocations
by themselves, which feels counter productive. Let us improve the core
by looping over xp_alloc() @max times when slow path needs to be taken.

Another issue with current interface, as spotted and fixed by Dries, was
that when driver called xsk_buff_alloc_batch() with @max == 0, for slow
path case it still allocated and returned a single buffer, which should
not happen. By introducing the logic from first paragraph we kill two
birds with one stone and address this problem as well.

Fixes: 47e4075df300 ("xsk: Batched buffer allocation for the pool")
Reported-and-tested-by: Dries De Winter <[e-mail]>
Co-developed-by: Dries De Winter <[e-mail]>
Signed-off-by: Dries De Winter <[e-mail]>
Signed-off-by: Maciej Fijalkowski <[e-mail]>
---
 net/xdp/xsk_buff_pool.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Magnus Karlsson Sept. 12, 2024, 11:04 a.m. UTC | #1

On Wed, 11 Sept 2024 at 21:10, Maciej Fijalkowski
<[e-mail]> wrote:
>
> In cases when synchronizing DMA operations is necessary,
> xsk_buff_alloc_batch() returns a single buffer instead of the requested
> count. This puts the pressure on drivers that use batch API as they have
> to check for this corner case on their side and take care of allocations
> by themselves, which feels counter productive. Let us improve the core
> by looping over xp_alloc() @max times when slow path needs to be taken.
>
> Another issue with current interface, as spotted and fixed by Dries, was
> that when driver called xsk_buff_alloc_batch() with @max == 0, for slow
> path case it still allocated and returned a single buffer, which should
> not happen. By introducing the logic from first paragraph we kill two
> birds with one stone and address this problem as well.

Thanks Maciej and Dries for finding and fixing this.

Acked-by: Magnus Karlsson <[e-mail]>

> Fixes: 47e4075df300 ("xsk: Batched buffer allocation for the pool")
> Reported-and-tested-by: Dries De Winter <[e-mail]>
> Co-developed-by: Dries De Winter <[e-mail]>
> Signed-off-by: Dries De Winter <[e-mail]>
> Signed-off-by: Maciej Fijalkowski <[e-mail]>
> ---
>  net/xdp/xsk_buff_pool.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index 29afa880ffa0..5e2e03042ef3 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -623,20 +623,31 @@ static u32 xp_alloc_reused(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u3
>         return nb_entries;
>  }
>
> -u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max)
> +static u32 xp_alloc_slow(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> +                        u32 max)
>  {
> -       u32 nb_entries1 = 0, nb_entries2;
> +       int i;
>
> -       if (unlikely(pool->dev && dma_dev_need_sync(pool->dev))) {
> +       for (i = 0; i < max; i++) {
>                 struct xdp_buff *buff;
>
> -               /* Slow path */
>                 buff = xp_alloc(pool);
> -               if (buff)
> -                       *xdp = buff;
> -               return !!buff;
> +               if (unlikely(!buff))
> +                       return i;
> +               *xdp = buff;
> +               xdp++;
>         }
>
> +       return max;
> +}
> +
> +u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max)
> +{
> +       u32 nb_entries1 = 0, nb_entries2;
> +
> +       if (unlikely(pool->dev && dma_dev_need_sync(pool->dev)))
> +               return xp_alloc_slow(pool, xdp, max);
> +
>         if (unlikely(pool->free_list_cnt)) {
>                 nb_entries1 = xp_alloc_reused(pool, xdp, max);
>                 if (nb_entries1 == max)
> --
> 2.34.1
>
>

Alexei Starovoitov Sept. 13, 2024, 7:48 p.m. UTC | #2

On Thu, Sep 12, 2024 at 4:04 AM Magnus Karlsson
<[e-mail]> wrote:
>
> On Wed, 11 Sept 2024 at 21:10, Maciej Fijalkowski
> <[e-mail]> wrote:
> >
> > In cases when synchronizing DMA operations is necessary,
> > xsk_buff_alloc_batch() returns a single buffer instead of the requested
> > count. This puts the pressure on drivers that use batch API as they have
> > to check for this corner case on their side and take care of allocations
> > by themselves, which feels counter productive. Let us improve the core
> > by looping over xp_alloc() @max times when slow path needs to be taken.
> >
> > Another issue with current interface, as spotted and fixed by Dries, was
> > that when driver called xsk_buff_alloc_batch() with @max == 0, for slow
> > path case it still allocated and returned a single buffer, which should
> > not happen. By introducing the logic from first paragraph we kill two
> > birds with one stone and address this problem as well.
>
> Thanks Maciej and Dries for finding and fixing this.
>
> Acked-by: Magnus Karlsson <[e-mail]>

We already did the last bpf and bpf-next/net PRs before the merge window,
so reassigning to netdev.

Acked-by: Alexei Starovoitov <[e-mail]>

[e-mail] Sept. 14, 2024, 1:20 a.m. UTC | #3

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[e-mail]>:

On Wed, 11 Sep 2024 21:10:19 +0200 you wrote:
> In cases when synchronizing DMA operations is necessary,
> xsk_buff_alloc_batch() returns a single buffer instead of the requested
> count. This puts the pressure on drivers that use batch API as they have
> to check for this corner case on their side and take care of allocations
> by themselves, which feels counter productive. Let us improve the core
> by looping over xp_alloc() @max times when slow path needs to be taken.
> 
> [...]

Here is the summary with links:
  - [bpf] xsk: fix batch alloc API on non-coherent systems
    https://git.kernel.org/netdev/net/c/4144a1059b47

You are awesome, thank you!

Patch

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 29afa880ffa0..5e2e03042ef3 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -623,20 +623,31 @@  static u32 xp_alloc_reused(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u3
 	return nb_entries;
 }
 
-u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max)
+static u32 xp_alloc_slow(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
+			 u32 max)
 {
-	u32 nb_entries1 = 0, nb_entries2;
+	int i;
 
-	if (unlikely(pool->dev && dma_dev_need_sync(pool->dev))) {
+	for (i = 0; i < max; i++) {
 		struct xdp_buff *buff;
 
-		/* Slow path */
 		buff = xp_alloc(pool);
-		if (buff)
-			*xdp = buff;
-		return !!buff;
+		if (unlikely(!buff))
+			return i;
+		*xdp = buff;
+		xdp++;
 	}
 
+	return max;
+}
+
+u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max)
+{
+	u32 nb_entries1 = 0, nb_entries2;
+
+	if (unlikely(pool->dev && dma_dev_need_sync(pool->dev)))
+		return xp_alloc_slow(pool, xdp, max);
+
 	if (unlikely(pool->free_list_cnt)) {
 		nb_entries1 = xp_alloc_reused(pool, xdp, max);
 		if (nb_entries1 == max)