Re: [PATCH net-next 1/2] xsk: avoid using heavy lock when the pool is not shared

[Jason Xing <kerneljasonxing@gmail.com>]

On Wed, Oct 29, 2025 at 11:48 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Sat, Oct 25, 2025 at 02:53:09PM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The commit f09ced4053bc ("xsk: Fix race in SKB mode transmit with
> > shared cq") uses a heavy lock (spin_lock_irqsave) for the shared
> > pool scenario which is that multiple sockets share the same pool.
> >
> > It does harm to the case where the pool is only owned by one xsk.
> > The patch distinguishes those two cases through checking if the xsk
> > list only has one xsk. If so, that means the pool is exclusive and
> > we don't need to hold the lock and disable IRQ at all. The benefit
> > of this is to avoid those two operations being executed extremely
> > frequently.
>
> Even with a single CQ producer we need to have related code within
> critical section. One core can be in process context via sendmsg() and
> for some reason xmit failed and driver consumed skb (destructor called).
>
> Other core can be at same time calling the destructor on different skb
> that has been successfully xmitted, doing the Tx completion via driver's
> NAPI. This means that without locking the SPSC concept would be violated.
>
> So I'm afraid I have to nack this.

But that will not happen around cq->cached_prod. All the possible
places where cached_prod is modified are in the process context. I've
already pointed out the different subtle cases in patch [2/2].

SPSC is all about the global state of producer and consumer that can
affect both layers instead of local or cached ones. So that's why we
can apply a lockless policy in this patch when the pool is exclusive
and why we can use a smaller lock as patch [2/2] shows.

As to how to prevent the case like Jakub mentioned, so far I cannot
find a good solution unless introducing a new option that limits one
xsk binding to only one unique pool. But probably it's not worth it.
It's the reason why I will scrap this patch in V2.

Thanks,
Jason