Message-ID: <CAJaqyWcq73v_v6OoxWnwkOY5gxHV+kL+4eDDjor_rP=OzrBrvg@mail.gmail.com>
Date: Wed, 4 Feb 2026 09:53:19 +0100
From: Eugenio Perez Martin <eperezma@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: "Michael S . Tsirkin" <mst@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	Cindy Lu <lulu@...hat.com>, Laurent Vivier <lvivier@...hat.com>, 
	Stefano Garzarella <sgarzare@...hat.com>, linux-kernel@...r.kernel.org, 
	Maxime Coquelin <mcoqueli@...hat.com>, Yongji Xie <xieyongji@...edance.com>, 
	virtualization@...ts.linux.dev
Subject: Re: [PATCH 1/6] vduse: ensure vq->ready access is smp safe

On Wed, Feb 4, 2026 at 3:48 AM Jason Wang <jasowang@...hat.com> wrote:
>
> On Tue, Feb 3, 2026 at 6:35 PM Eugenio Perez Martin <eperezma@...hat.com> wrote:
> >
> > On Tue, Feb 3, 2026 at 5:05 AM Jason Wang <jasowang@...hat.com> wrote:
> > >
> > > On Fri, Jan 30, 2026 at 3:56 PM Eugenio Perez Martin
> > > <eperezma@...hat.com> wrote:
> > > >
> > > > On Fri, Jan 30, 2026 at 3:18 AM Jason Wang <jasowang@...hat.com> wrote:
> > > > >
> > > > > On Thu, Jan 29, 2026 at 2:21 PM Eugenio Perez Martin
> > > > > <eperezma@...hat.com> wrote:
> > > > > >
> > > > > > On Thu, Jan 29, 2026 at 2:17 AM Jason Wang <jasowang@...hat.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jan 28, 2026 at 8:45 PM Eugenio Pérez <eperezma@...hat.com> wrote:
> > > > > > > >
> > > > > > > > The vduse_vdpa_set_vq_ready can be called in the lifetime of the device
> > > > > > > > well after initial setup, and the device can read it afterwards.
> > > > > > > >
> > > > > > > > Ensure that reads and writes to vq->ready are SMP safe so that the
> > > > > > > > caller can trust that virtqueue kicks and calls behave as expected
> > > > > > > > immediately after the operation returns.
> > > > > > > >
> > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> > > > > > > > ---
> > > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 34 +++++++++++++++++++++++-------
> > > > > > > >  1 file changed, 26 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > index 73d1d517dc6c..a4963aaf9332 100644
> > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > @@ -460,6 +460,24 @@ static __poll_t vduse_dev_poll(struct file *file, poll_table *wait)
> > > > > > > >         return mask;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static bool vduse_vq_get_ready(const struct vduse_virtqueue *vq)
> > > > > > > > +{
> > > > > > > > +       /*
> > > > > > > > +        * Paired with vduse_vq_set_ready smp_store, as the driver may modify
> > > > > > > > +        * it while the VDUSE instance is reading it.
> > > > > > > > +        */
> > > > > > > > +       return smp_load_acquire(&vq->ready);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void vduse_vq_set_ready(struct vduse_virtqueue *vq, bool ready)
> > > > > > > > +{
> > > > > > > > +       /*
> > > > > > > > +        * Paired with vduse_vq_get_ready smp_load, as the driver may modify
> > > > > > > > +        * it while the VDUSE instance is reading it.
> > > > > > > > +        */
> > > > > > > > +       smp_store_release(&vq->ready, ready);
> > > > > > >
> > > > > > > Assuming this is not used in the datapath, I wonder if we can simply
> > > > > > > use vq_lock mutex.
> > > > > > >
> > > > > >
> > > > > > The function vduse_vq_set/get_ready are not in the datapath, but
> > > > > > vduse_vq_kick and vduse_vq_signal_irqfd are.  I'm ok if you want to
> > > > > > switch to vq_mutex if you want though, maybe it's even comparable with
> > > > > > the cost of the ioctls or eventfd signaling.
> > > > >
> > > > > I'd like to use mutex for simplicity.
> > > > >
> > > >
> > > > I cannot move it to a mutex, as we need to take it in the critical
> > > > sections of the kick_lock and irq_lock spinlocks.
> > > >
> > > > I can move it to a spinlock, but it seems more complicated to me. We
> > > > need to make sure we always take these kick_lock and irq_lock in the
> > > > same order as the ready_lock, to not create deadlocks, and they always
> > > > protect just the ready boolean. But sure, I'll send the spinlock
> > > > version for V2.
> > >
> > > Thinking about this, I'm not sure I understand the issue.
> > >
> > > Maybe you can give me an example of the race.
> > >
> >
> > It's the same issue as you describe with set_group_asid on cpu1 and
> > dma_map and dma_unmap in cpu1 in this mail [0], but now the functions
> > are set_vq_ready in the vdpa driver side and VDUSE_VQ_GET_INFO in the
> > VDUSE instance.
> >
> > To me, the barriers should be in the driver and the VDUSE instance,
> > and the VDUSE instance should use the VDUSE_SET_VQ_READY message to
> > know when a VQ is ready. But I don't expect all the VDUSE instances to
> > opt-in for that, so the flow without smp barriers is:
> >
> > [cpu0] .set_vq_ready(vq, true)
> > [cpu1] VDUSE_VQ_GET_INFO(vq)
> > [cpu1] check vq->ready, and it races with the set_vq_ready from cpu0.
> >
>
> Are there any side effects of this race? I meant the driver can do
> set_vq_ready() at any time. And there would be message for notifying
> the usersapce in this series.
>

Actually I'm moving to a semaphore for the moment. SMP barrier is not enough.

The message works for the driver to know that the device vq is ready
and it can send kicks. But there is a race for the device where it has
replied to the message but vq->ready is still not true:

static void vduse_vdpa_set_vq_ready(struct vdpa_device *vdpa,
                                        u16 idx, bool ready)
{
        [...]
        r = vduse_dev_msg_sync(dev, &msg);
        // The device could try to call() from here...

        [...]
out:
        // to here
        vduse_vq_set_ready(vq, ready);
}

To be honest, I'd prefer to just set ready = true at
vduse_dev_write_iter before returning success to the VDUSE userland.
This way the synchronization struct is omitted entirely. But let me
know if you prefer otherwise.

Thanks!

Powered by blists - more mailing lists