[PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[Wang Liang]

When the cq reservation is failed, the error code is not set which is
initialized to zero in __xsk_generic_xmit(). That means the packet is not
send successfully but sendto() return ok.

Set the error code and make xskq_prod_reserve_addr()/xskq_prod_reserve()
return values more meaningful when the queue is full.

Signed-off-by: Wang Liang <wangliang74@huawei.com>
---
 net/xdp/xsk.c       | 3 ++-
 net/xdp/xsk_queue.h | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 89d2bef96469..7d0d2f40ca57 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -802,7 +802,8 @@ static int __xsk_generic_xmit(struct sock *sk)
 		 * if there is space in it. This avoids having to implement
 		 * any buffering in the Tx path.
 		 */
-		if (xsk_cq_reserve_addr_locked(xs->pool, desc.addr))
+		err = xsk_cq_reserve_addr_locked(xs->pool, desc.addr);
+		if (err)
 			goto out;
 
 		skb = xsk_build_skb(xs, &desc);
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 46d87e961ad6..ac90b7fcc027 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -371,7 +371,7 @@ static inline void xskq_prod_cancel_n(struct xsk_queue *q, u32 cnt)
 static inline int xskq_prod_reserve(struct xsk_queue *q)
 {
 	if (xskq_prod_is_full(q))
-		return -ENOSPC;
+		return -ENOBUFS;
 
 	/* A, matches D */
 	q->cached_prod++;
@@ -383,7 +383,7 @@ static inline int xskq_prod_reserve_addr(struct xsk_queue *q, u64 addr)
 	struct xdp_umem_ring *ring = (struct xdp_umem_ring *)q->ring;
 
 	if (xskq_prod_is_full(q))
-		return -ENOSPC;
+		return -ENOBUFS;
 
 	/* A, matches D */
 	ring->desc[q->cached_prod++ & q->ring_mask] = addr;
-- 
2.34.1

Re: [PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[Magnus Karlsson]

On Sat, 22 Feb 2025 at 10:18, Wang Liang <wangliang74@huawei.com> wrote:
>
> When the cq reservation is failed, the error code is not set which is
> initialized to zero in __xsk_generic_xmit(). That means the packet is not
> send successfully but sendto() return ok.
>
> Set the error code and make xskq_prod_reserve_addr()/xskq_prod_reserve()
> return values more meaningful when the queue is full.

Hi Wang,

I agree that this would have been a really good idea if it was
implemented from day one, but now I do not dare to change this since
it would be changing the uapi. Let us say you have the following quite
common code snippet for sending a packet with AF_XDP in skb mode:

err = sendmsg();
if (err && err != -EAGAIN && err != -EBUSY)
    goto die_due_to_error;
continue with code

This code would with your change go and die suddenly when the
completion ring is full instead of working. Maybe there is a piece of
code that cleans the completion ring after these lines of code and
next time sendmsg() is called, the packet will get sent, so the
application used to work.

So I say: let us not do this. But if anyone has another opinion, please share.

Thanks for the report: Magnus

> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
>  net/xdp/xsk.c       | 3 ++-
>  net/xdp/xsk_queue.h | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 89d2bef96469..7d0d2f40ca57 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -802,7 +802,8 @@ static int __xsk_generic_xmit(struct sock *sk)
>                  * if there is space in it. This avoids having to implement
>                  * any buffering in the Tx path.
>                  */
> -               if (xsk_cq_reserve_addr_locked(xs->pool, desc.addr))
> +               err = xsk_cq_reserve_addr_locked(xs->pool, desc.addr);
> +               if (err)
>                         goto out;
>
>                 skb = xsk_build_skb(xs, &desc);
> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 46d87e961ad6..ac90b7fcc027 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -371,7 +371,7 @@ static inline void xskq_prod_cancel_n(struct xsk_queue *q, u32 cnt)
>  static inline int xskq_prod_reserve(struct xsk_queue *q)
>  {
>         if (xskq_prod_is_full(q))
> -               return -ENOSPC;
> +               return -ENOBUFS;
>
>         /* A, matches D */
>         q->cached_prod++;
> @@ -383,7 +383,7 @@ static inline int xskq_prod_reserve_addr(struct xsk_queue *q, u64 addr)
>         struct xdp_umem_ring *ring = (struct xdp_umem_ring *)q->ring;
>
>         if (xskq_prod_is_full(q))
> -               return -ENOSPC;
> +               return -ENOBUFS;
>
>         /* A, matches D */
>         ring->desc[q->cached_prod++ & q->ring_mask] = addr;
> --
> 2.34.1
>
>

Re: [PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[Stanislav Fomichev]

On 02/24, Magnus Karlsson wrote:
> On Sat, 22 Feb 2025 at 10:18, Wang Liang <wangliang74@huawei.com> wrote:
> >
> > When the cq reservation is failed, the error code is not set which is
> > initialized to zero in __xsk_generic_xmit(). That means the packet is not
> > send successfully but sendto() return ok.
> >
> > Set the error code and make xskq_prod_reserve_addr()/xskq_prod_reserve()
> > return values more meaningful when the queue is full.
> 
> Hi Wang,
> 
> I agree that this would have been a really good idea if it was
> implemented from day one, but now I do not dare to change this since
> it would be changing the uapi. Let us say you have the following quite
> common code snippet for sending a packet with AF_XDP in skb mode:
> 
> err = sendmsg();
> if (err && err != -EAGAIN && err != -EBUSY)
>     goto die_due_to_error;
> continue with code
> 
> This code would with your change go and die suddenly when the
> completion ring is full instead of working. Maybe there is a piece of
> code that cleans the completion ring after these lines of code and
> next time sendmsg() is called, the packet will get sent, so the
> application used to work.
> 
> So I say: let us not do this. But if anyone has another opinion, please share.

Can we return -EBUSY from this 'if (xsk_cq_reserve_addr_locked())' case as
well?

Re: [PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[Magnus Karlsson]

On Mon, 24 Feb 2025 at 17:00, Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 02/24, Magnus Karlsson wrote:
> > On Sat, 22 Feb 2025 at 10:18, Wang Liang <wangliang74@huawei.com> wrote:
> > >
> > > When the cq reservation is failed, the error code is not set which is
> > > initialized to zero in __xsk_generic_xmit(). That means the packet is not
> > > send successfully but sendto() return ok.
> > >
> > > Set the error code and make xskq_prod_reserve_addr()/xskq_prod_reserve()
> > > return values more meaningful when the queue is full.
> >
> > Hi Wang,
> >
> > I agree that this would have been a really good idea if it was
> > implemented from day one, but now I do not dare to change this since
> > it would be changing the uapi. Let us say you have the following quite
> > common code snippet for sending a packet with AF_XDP in skb mode:
> >
> > err = sendmsg();
> > if (err && err != -EAGAIN && err != -EBUSY)
> >     goto die_due_to_error;
> > continue with code
> >
> > This code would with your change go and die suddenly when the
> > completion ring is full instead of working. Maybe there is a piece of
> > code that cleans the completion ring after these lines of code and
> > next time sendmsg() is called, the packet will get sent, so the
> > application used to work.
> >
> > So I say: let us not do this. But if anyone has another opinion, please share.
>
> Can we return -EBUSY from this 'if (xsk_cq_reserve_addr_locked())' case as
> well?

That is a good idea! Though I would return -EAGAIN. When -EBUSY is
returned, the buffer was consumed but not sent. But -EAGAIN means that
the user just has to perform then sendmsg() again and that is exactly
what the user has to do here too.

Re: [PATCH net] xsk: fix __xsk_generic_xmit() error code when cq is full

[Wang Liang]

在 2025/2/25 1:14, Magnus Karlsson 写道:
> On Mon, 24 Feb 2025 at 17:00, Stanislav Fomichev <stfomichev@gmail.com> wrote:
>> On 02/24, Magnus Karlsson wrote:
>>> On Sat, 22 Feb 2025 at 10:18, Wang Liang <wangliang74@huawei.com> wrote:
>>>> When the cq reservation is failed, the error code is not set which is
>>>> initialized to zero in __xsk_generic_xmit(). That means the packet is not
>>>> send successfully but sendto() return ok.
>>>>
>>>> Set the error code and make xskq_prod_reserve_addr()/xskq_prod_reserve()
>>>> return values more meaningful when the queue is full.
>>> Hi Wang,
>>>
>>> I agree that this would have been a really good idea if it was
>>> implemented from day one, but now I do not dare to change this since
>>> it would be changing the uapi. Let us say you have the following quite
>>> common code snippet for sending a packet with AF_XDP in skb mode:
>>>
>>> err = sendmsg();
>>> if (err && err != -EAGAIN && err != -EBUSY)
>>>      goto die_due_to_error;
>>> continue with code
>>>
>>> This code would with your change go and die suddenly when the
>>> completion ring is full instead of working. Maybe there is a piece of
>>> code that cleans the completion ring after these lines of code and
>>> next time sendmsg() is called, the packet will get sent, so the
>>> application used to work.
>>>
>>> So I say: let us not do this. But if anyone has another opinion, please share.
>> Can we return -EBUSY from this 'if (xsk_cq_reserve_addr_locked())' case as
>> well?
> That is a good idea! Though I would return -EAGAIN. When -EBUSY is
> returned, the buffer was consumed but not sent. But -EAGAIN means that
> the user just has to perform then sendmsg() again and that is exactly
> what the user has to do here too.


Thank you for the suggestion!
Changing the uapi is indeed a high-risk act. Return -EAGAIN is a much 
better choice.
The cq is full usually because it is not released in time, try to send 
msg again is appropriate.
I will send a new patch later, and look forward to getting more advice. 
Thanks.