Re: [PATCH net] xsk: correct tx_ring_empty_descs count statistics

From: Magnus Karlsson
Date: Tue Apr 01 2025 - 02:58:08 EST


On Tue, 1 Apr 2025 at 04:36, Wang Liang <wangliang74@xxxxxxxxxx> wrote:
>
>
> 在 2025/4/1 6:03, Stanislav Fomichev 写道:
> > On 03/31, Stanislav Fomichev wrote:
> >> On 03/29, Wang Liang wrote:
> >>> The tx_ring_empty_descs count may be incorrect, when set the XDP_TX_RING
> >>> option but do not reserve tx ring. Because xsk_poll() try to wakeup the
> >>> driver by calling xsk_generic_xmit() for non-zero-copy mode. So the
> >>> tx_ring_empty_descs count increases once the xsk_poll()is called:
> >>>
> >>> xsk_poll
> >>> xsk_generic_xmit
> >>> __xsk_generic_xmit
> >>> xskq_cons_peek_desc
> >>> xskq_cons_read_desc
> >>> q->queue_empty_descs++;

Sorry, but I do not understand how to reproduce this error. So you
first issue a setsockopt with the XDP_TX_RING option and then you do
not "reserve tx ring". What does that last "not reserve tx ring" mean?
No mmap() of that ring, or something else? I guess you have bound the
socket with a bind()? Some pseudo code on how to reproduce this would
be helpful. Just want to understand so I can help. Thank you.

> >>>
> >>> To avoid this count error, add check for tx descs before send msg in poll.
> >>>
> >>> Fixes: df551058f7a3 ("xsk: Fix crash in poll when device does not support ndo_xsk_wakeup")
> >>> Signed-off-by: Wang Liang <wangliang74@xxxxxxxxxx>
> >> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> > Hmm, wait, I stumbled upon xskq_has_descs again and it looks only at
> > cached prod/cons. How is it supposed to work when the actual tx
> > descriptor is posted? Is there anything besides xskq_cons_peek_desc from
> > __xsk_generic_xmit that refreshes cached_prod?
>
>
> Yes, you are right!
>
> How about using xskq_cons_nb_entries() to check free descriptors?
>
> Like this:
>
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index e5d104ce7b82..babb7928d335 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -993,7 +993,7 @@ static __poll_t xsk_poll(struct file *file, struct
> socket *sock,
> if (pool->cached_need_wakeup) {
> if (xs->zc)
> xsk_wakeup(xs, pool->cached_need_wakeup);
> - else if (xs->tx)
> + else if (xs->tx && xskq_cons_nb_entries(xs->tx, 1))
> /* Poll needs to drive Tx also in copy mode */
> xsk_generic_xmit(sk);
> }
>
>