Re: [PATCH V2] vhost_vdpa: no need to fetch vring base when poweroff

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8253 bytes --]



On 7/12/2023 3:22 PM, Jason Wang wrote:
>
>
> On Wed, Jul 12, 2023 at 2:54 PM Zhu, Lingshan <lingshan.zhu@intel.com> 
> wrote:
>
>
>
>     On 7/11/2023 3:34 PM, Jason Wang wrote:
>>
>>
>>     On Tue, Jul 11, 2023 at 3:25 PM Eugenio Perez Martin
>>     <eperezma@redhat.com> wrote:
>>
>>         On Tue, Jul 11, 2023 at 9:05 AM Jason Wang
>>         <jasowang@redhat.com> wrote:
>>         >
>>         > On Tue, Jul 11, 2023 at 12:09 PM Zhu, Lingshan
>>         <lingshan.zhu@intel.com> wrote:
>>         > >
>>         > >
>>         > >
>>         > > On 7/11/2023 10:50 AM, Jason Wang wrote:
>>         > > > On Mon, Jul 10, 2023 at 4:53 PM Zhu Lingshan
>>         <lingshan.zhu@intel.com> wrote:
>>         > > >> In the poweroff routine, no need to fetch last
>>         available index.
>>         > > >>
>>         > > > This is because there's no concept of shutdown in the
>>         vhost layer, it
>>         > > > only knows start and stop.
>>         > > >
>>         > > >> This commit also provides a better debug message in
>>         the vhost
>>         > > >> caller vhost_virtqueue_stop,
>>         > > > A separate patch is better.
>>         > > OK
>>         > > >
>>         > > >> because if vhost does not fetch
>>         > > >> the last avail idx successfully, maybe the device does not
>>         > > >> suspend, vhost will sync last avail idx to vring used
>>         idx as a
>>         > > >> work around, not a failure.
>>         > > > This only happens if we return a negative value?
>>         > > Yes
>>         > > >
>>         > > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>         > > >> ---
>>         > > >>   hw/virtio/vhost-vdpa.c | 10 ++++++++++
>>         > > >>   hw/virtio/vhost.c      |  2 +-
>>         > > >>   2 files changed, 11 insertions(+), 1 deletion(-)
>>         > > >>
>>         > > >> diff --git a/hw/virtio/vhost-vdpa.c
>>         b/hw/virtio/vhost-vdpa.c
>>         > > >> index 3c575a9a6e..10b445f64e 100644
>>         > > >> --- a/hw/virtio/vhost-vdpa.c
>>         > > >> +++ b/hw/virtio/vhost-vdpa.c
>>         > > >> @@ -26,6 +26,7 @@
>>         > > >>   #include "cpu.h"
>>         > > >>   #include "trace.h"
>>         > > >>   #include "qapi/error.h"
>>         > > >> +#include "sysemu/runstate.h"
>>         > > >>
>>         > > >>   /*
>>         > > >>    * Return one past the end of the end of section. Be
>>         careful with uint64_t
>>         > > >> @@ -1391,6 +1392,15 @@ static int
>>         vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>>         > > >>       struct vhost_vdpa *v = dev->opaque;
>>         > > >>       int ret;
>>         > > >>
>>         > > >> +    if (runstate_check(RUN_STATE_SHUTDOWN)) {
>>         > > >> +        /*
>>         > > >> +         * Some devices do not support this call
>>         properly,
>>         > > >> +         * and we don't need to retrieve the indexes
>>         > > >> +         * if it is shutting down
>>         > > >> +         */
>>         > > >> +        return 0;
>>         > > > Checking runstate in the vhost code seems like a layer
>>         violation.
>>         > > >
>>         > > > What happens without this patch?
>>         > > vhost tries to fetch vring base,
>>         > > vhost_vdpa needs suspend the device before retrieving
>>         last_avail_idx.
>>         > > However not all devices can support .suspend properly so
>>         this call
>>         > > may fail.
>>         >
>>         > I think this is where I'm lost. If the device doesn't support
>>         > suspending, any reason we only try to fix the case of shutdown?
>>         >
>>         > Btw, the fail is intended:
>>         >
>>         >     if (!v->suspended) {
>>         >         /*
>>         >          * Cannot trust in value returned by device, let
>>         vhost recover used
>>         >          * idx from guest.
>>         >          */
>>         >         return -1;
>>         >     }
>>         >
>>
>>         The fail is intended, but to recover the last used idx,
>>         either from
>>         device or from the guest, is only useful in the case of
>>         migration.
>>
>>
>>     Note that we had userspace devices like VDUSE now, so it could be
>>     useful in the case of a VDUSE daemon crash or reconnect.
>     This code blcok is for vhost_vdpa backend, and I think vduse is
>     another code path.
>
>
> I'm not sure I understand here, I meant vhost_vdpa + vduse. It works 
> similar to vhost-user.
OK, so do you suggest we set vring state == 0 and return 0 if failed to 
suspend the device?
No matter shutdown or other cases.
>
>     Return a guest used idx may be a good idea but as Eugenio pointed
>     out that may duplicate the code.
>>
>>
>>         I think the main problem is the error message, actually. But
>>         I think
>>         it has no use either to recover last_avail_idx or print a debug
>>         message if we're not migrating. Another solution would be to
>>         recover
>>         it from the guest at vhost_vdpa_get_vring_base, but I don't
>>         like the
>>         duplication.
>>
>>         > And if we return to success here, will we go to set an
>>         uninitialized
>>         > last avail idx?
>>         >
>>
>>         It will be either the default value (is set to 0 at
>>         __virtio_queue_reset) or the one received from a migration (at
>>         virtio_load).
>>
>>
>>     0 is even sub-optimal than the index used. Anyhow, VHOST_DEBUG
>>     should not be enabled for production environments.
>     Returning 0 sounds like a queue reset, yes we can reset a queue if
>     failed to suspend it, I am not sure whther
>     0 is better than guest used idx.
>
>     I think we are not able to disable VHOST_DEBUG because customers
>     can build QEMU by their own.
>
>
> Well, disabling debug information is a common practice in any 
> distribution.
>
> Or if you worry about the default, let's have a patch to undef 
> VHOST_DEBUG by defualt.
>
I can do this in the next version

Thanks
> Thanks
>
>
>     Thanks
>>
>>     Thanks
>>
>>
>>         Thanks!
>>
>>         >     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
>>         >     if (r < 0) {
>>         >     ...
>>         >     }.else {
>>         >  virtio_queue_set_last_avail_idx(vdev, idx, state.num);
>>         >     }
>>         >
>>         > Thanks
>>         >
>>         > > Then vhost will print an error shows something failed.
>>         > >
>>         > > The error msg is confused, as stated in the commit log,
>>         restoring
>>         > > last_avail_idx with guest used idx
>>         > > is a workaround rather than a failure. And no needs to
>>         fetch last_avail_idx
>>         > > when power off.
>>         > >
>>         > > Thanks
>>         > > >
>>         > > > Thanks
>>         > > >
>>         > > >> +    }
>>         > > >> +
>>         > > >>       if (v->shadow_vqs_enabled) {
>>         > > >>           ring->num =
>>         virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
>>         > > >>           return 0;
>>         > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>         > > >> index 82394331bf..7dd90cff3a 100644
>>         > > >> --- a/hw/virtio/vhost.c
>>         > > >> +++ b/hw/virtio/vhost.c
>>         > > >> @@ -1262,7 +1262,7 @@ void vhost_virtqueue_stop(struct
>>         vhost_dev *dev,
>>         > > >>
>>         > > >>       r = dev->vhost_ops->vhost_get_vring_base(dev,
>>         &state);
>>         > > >>       if (r < 0) {
>>         > > >> -        VHOST_OPS_DEBUG(r, "vhost VQ %u ring restore
>>         failed: %d", idx, r);
>>         > > >> +        VHOST_OPS_DEBUG(r, "sync last avail idx to
>>         the guest used idx for vhost VQ %u", idx);
>>         > > >>           /* Connection to the backend is broken, so
>>         let's sync internal
>>         > > >>            * last avail idx to the device used idx.
>>         > > >>            */
>>         > > >> --
>>         > > >> 2.39.3
>>         > > >>
>>         > >
>>         >
>>
>

[-- Attachment #2: Type: text/html, Size: 19742 bytes --]