Re: [PATCH v2] vhost: always lock all QPs if handlers request it

[Maxime Coquelin <mcoqueli@redhat.com>]

On Tue, Oct 14, 2025 at 2:54 PM Maxime Coquelin <mcoqueli@redhat.com> wrote:
>
> Hi Serghii,
>
> On Tue, Oct 7, 2025 at 5:06 PM Serhii Iliushyk <sil-plv@napatech.com> wrote:
> >
> > The check for the VIRTIO_DEV_VDPA_CONFIGURED flag is not needed
> > since each handler has its own lock_all_qps flag
> > to indicate whether it needs all queue pairs.
>
> Hmm, I think this is needed because lock_all_qps is only effective
> with the SW datapath.
> With vDPA, once the device is configured, we would end-up with
> virtqueues modifications
> without the HW being stopped or even notified.
>
> > Checking the VIRTIO_DEV_VDPA_CONFIGURED flag caused certain handlers
> > (for example, SET_MEM_TABLE for memory hot plug) to run without holding
> > the per-vq access_lock and trigger vq_assert_lock failures.
>
> In this specific memory hotplug case, we do the right thing in the handler:
>
>     if (dev->mem) {
>         if (dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) {
>             struct rte_vdpa_device *vdpa_dev = dev->vdpa_dev;
>
>             if (vdpa_dev && vdpa_dev->ops->dev_close)
>                 vdpa_dev->ops->dev_close(dev->vid);
>             dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED;
>         }
>
> Maybe we should also lock all the queue pairs once the vDPA is closed.
> The problem is to handle the unlock properly.

Maybe a generic solution would be in vhost_user_msg_handler() to close
the vDPA device,
clear CONFIGURED flag and lock all queue pairs as we do in
vhost_user_set_mem_table?

I have no hardware to test, could you have a try if you think it makes sense?

Thanks,
Maxime
> Maxime
>
> > Signed-off-by: Serhii Iliushyk <sil-plv@napatech.com>
> > ---
> > v2
> > * fix typo in commit message
> > ---
> >  lib/vhost/vhost_user.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> > index 4bfb13fb98..9288da9322 100644
> > --- a/lib/vhost/vhost_user.c
> > +++ b/lib/vhost/vhost_user.c
> > @@ -3171,10 +3171,8 @@ vhost_user_msg_handler(int vid, int fd)
> >          * would cause a dead lock.
> >          */
> >         if (msg_handler != NULL && msg_handler->lock_all_qps) {
> > -               if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> > -                       vhost_user_lock_all_queue_pairs(dev);
> > -                       unlock_required = 1;
> > -               }
> > +               vhost_user_lock_all_queue_pairs(dev);
> > +               unlock_required = 1;
> >         }
> >
> >         handled = false;
> > --
> > 2.47.3
> >