netdev - Re: [PATCH net-next v2 2/3] xsk: use atomic operations around cached

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoCy9vkAmreAvtm2FhgL0bfjZ_kJm2p9JxyaCd1aTSiHew@mail.gmail.com>
Date: Thu, 27 Nov 2025 21:55:39 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	bjorn@...nel.org, magnus.karlsson@...el.com, maciej.fijalkowski@...el.com, 
	jonathan.lemon@...il.com, sdf@...ichev.me, ast@...nel.org, 
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com, 
	bpf@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 2/3] xsk: use atomic operations around
 cached_prod for copy mode

On Thu, Nov 27, 2025 at 7:35 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 11/25/25 9:54 AM, Jason Xing wrote:
> > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> > index 44cc01555c0b..3a023791b273 100644
> > --- a/net/xdp/xsk_queue.h
> > +++ b/net/xdp/xsk_queue.h
> > @@ -402,13 +402,28 @@ static inline void xskq_prod_cancel_n(struct xsk_queue *q, u32 cnt)
> >       q->cached_prod -= cnt;
> >  }
> >
> > -static inline int xskq_prod_reserve(struct xsk_queue *q)
> > +static inline bool xsk_cq_cached_prod_nb_free(struct xsk_queue *q)
> >  {
> > -     if (xskq_prod_is_full(q))
> > +     u32 cached_prod = atomic_read(&q->cached_prod_atomic);
> > +     u32 free_entries = q->nentries - (cached_prod - q->cached_cons);
> > +
> > +     if (free_entries)
> > +             return true;
> > +
> > +     /* Refresh the local tail pointer */
> > +     q->cached_cons = READ_ONCE(q->ring->consumer);
> > +     free_entries = q->nentries - (cached_prod - q->cached_cons);
> > +
> > +     return free_entries ? true : false;
> > +}
> _If_ different CPUs can call xsk_cq_cached_prod_reserve() simultaneously
> (as the spinlock existence suggests) the above change introduce a race:
>
> xsk_cq_cached_prod_nb_free() can return true when num_free == 1  on
> CPU1, and xsk_cq_cached_prod_reserve increment cached_prod_atomic on
> CPU2 before CPU1 completed xsk_cq_cached_prod_reserve().

I think you're right... I will give it more thought tomorrow morning.

I presume using try_cmpxchg() should work as it can detect if another
process changes @cached_prod simultaneously. They both work similarly.
But does it make any difference compared to spin lock? I don't have
any handy benchmark to stably measure two xsk sharing the same umem,
probably going to implement one.

Or like what you suggested in another thread, move that lock to struct
xsk_queue?

Thanks,
Jason