Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ

[Eugenio Perez Martin <eperezma@redhat.com>]

On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/2/17 下午4:22, Eugenio Perez Martin 写道:
> > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin
> >> <eperezma@redhat.com> wrote:
> >>> On Tue, Feb 8, 2022 at 9:25 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>>
> >>>> 在 2022/2/1 下午7:45, Eugenio Perez Martin 写道:
> >>>>> On Sun, Jan 30, 2022 at 7:50 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> 在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> >>>>>>> SVQ is able to log the dirty bits by itself, so let's use it to not
> >>>>>>> block migration.
> >>>>>>>
> >>>>>>> Also, ignore set and clear of VHOST_F_LOG_ALL on set_features if SVQ is
> >>>>>>> enabled. Even if the device supports it, the reports would be nonsense
> >>>>>>> because SVQ memory is in the qemu region.
> >>>>>>>
> >>>>>>> The log region is still allocated. Future changes might skip that, but
> >>>>>>> this series is already long enough.
> >>>>>>>
> >>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>>> ---
> >>>>>>>     hw/virtio/vhost-vdpa.c | 20 ++++++++++++++++++++
> >>>>>>>     1 file changed, 20 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>>>>>> index fb0a338baa..75090d65e8 100644
> >>>>>>> --- a/hw/virtio/vhost-vdpa.c
> >>>>>>> +++ b/hw/virtio/vhost-vdpa.c
> >>>>>>> @@ -1022,6 +1022,9 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, uint64_t *features)
> >>>>>>>         if (ret == 0 && v->shadow_vqs_enabled) {
> >>>>>>>             /* Filter only features that SVQ can offer to guest */
> >>>>>>>             vhost_svq_valid_guest_features(features);
> >>>>>>> +
> >>>>>>> +        /* Add SVQ logging capabilities */
> >>>>>>> +        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> >>>>>>>         }
> >>>>>>>
> >>>>>>>         return ret;
> >>>>>>> @@ -1039,8 +1042,25 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> >>>>>>>
> >>>>>>>         if (v->shadow_vqs_enabled) {
> >>>>>>>             uint64_t dev_features, svq_features, acked_features;
> >>>>>>> +        uint8_t status = 0;
> >>>>>>>             bool ok;
> >>>>>>>
> >>>>>>> +        ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> >>>>>>> +        if (unlikely(ret)) {
> >>>>>>> +            return ret;
> >>>>>>> +        }
> >>>>>>> +
> >>>>>>> +        if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> >>>>>>> +            /*
> >>>>>>> +             * vhost is trying to enable or disable _F_LOG, and the device
> >>>>>>> +             * would report wrong dirty pages. SVQ handles it.
> >>>>>>> +             */
> >>>>>> I fail to understand this comment, I'd think there's no way to disable
> >>>>>> dirty page tracking for SVQ.
> >>>>>>
> >>>>> vhost_log_global_{start,stop} are called at the beginning and end of
> >>>>> migration. To inform the device that it should start logging, they set
> >>>>> or clean VHOST_F_LOG_ALL at vhost_dev_set_log.
> >>>>
> >>>> Yes, but for SVQ, we can't disable dirty page tracking, isn't it? The
> >>>> only thing is to ignore or filter out the F_LOG_ALL and pretend to be
> >>>> enabled and disabled.
> >>>>
> >>> Yes, that's what this patch does.
> >>>
> >>>>> While SVQ does not use VHOST_F_LOG_ALL, it exports the feature bit so
> >>>>> vhost does not block migration. Maybe we need to look for another way
> >>>>> to do this?
> >>>>
> >>>> I'm fine with filtering since it's much more simpler, but I fail to
> >>>> understand why we need to check DRIVER_OK.
> >>>>
> >>> Ok maybe I can make that part more clear,
> >>>
> >>> Since both operations use vhost_vdpa_set_features we must just filter
> >>> the one that actually sets or removes VHOST_F_LOG_ALL, without
> >>> affecting other features.
> >>>
> >>> In practice, that means to not forward the set features after
> >>> DRIVER_OK. The device is not expecting them anymore.
> >> I wonder what happens if we don't do this.
> >>
> > If we simply delete the check vhost_dev_set_features will return an
> > error, failing the start of the migration. More on this below.
>
>
> Ok.
>
>
> >
> >> So kernel had this check:
> >>
> >>          /*
> >>           * It's not allowed to change the features after they have
> >>           * been negotiated.
> >>           */
> >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK)
> >>          return -EBUSY;
> >>
> >> So is it FEATURES_OK actually?
> >>
> > Yes, FEATURES_OK seems more appropriate actually so I will switch to
> > it for the next version.
> >
> > But it should be functionally equivalent, since
> > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot
> > be concurrent with it.
>
>
> Right.
>
>
> >
> >> For this patch, I wonder if the thing we need to do is to see whether
> >> it is a enable/disable F_LOG_ALL and simply return.
> >>
> > Yes, that's the intention of the patch.
> >
> > We have 4 cases here:
> > a) We're being called from vhost_dev_start, with enable_log = false
> > b) We're being called from vhost_dev_start, with enable_log = true
>
>
> And this case makes us can't simply return without calling vhost-vdpa.
>

It calls because {FEATURES,DRIVER}_OK is still not set at that point.

>
> > c) We're being called from vhost_dev_set_log, with enable_log = false
> > d) We're being called from vhost_dev_set_log, with enable_log = true
> >
> > The way to tell the difference between a/b and c/d is to check if
> > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails,
> > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty
> > memory through the memory unmapping, so we clear the bit
> > unconditionally if we detect that VHOST_SET_FEATURES will be called
> > (cases a and b).
> >
> > Another possibility is to track if features have been set with a bool
> > in vhost_vdpa or something like that. But it seems cleaner to me to
> > only store that in the actual device.
>
>
> So I suggest to make sure codes match the comment:
>
>          if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>              /*
>               * vhost is trying to enable or disable _F_LOG, and the device
>               * would report wrong dirty pages. SVQ handles it.
>               */
>              return 0;
>          }
>
> It would be better to check whether the caller is toggling _F_LOG_ALL in
> this case.
>

How to detect? We can save feature flags and compare, but ignoring all
set_features after FEATURES_OK seems simpler to me.

Would changing the comment work? Something like "set_features after
_S_FEATURES_OK means vhost is trying to enable or disable _F_LOG, and
the device would report wrong dirty pages. SVQ handles it."

Thanks!

> Thanks
>
>
> >
> >> Thanks
> >>
> >>> Does that make more sense?
> >>>
> >>> Thanks!
> >>>
> >>>> Thanks
> >>>>
> >>>>
> >>>>> Thanks!
> >>>>>
> >>>>>> Thanks
> >>>>>>
> >>>>>>
> >>>>>>> +            return 0;
> >>>>>>> +        }
> >>>>>>> +
> >>>>>>> +        /* We must not ack _F_LOG if SVQ is enabled */
> >>>>>>> +        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> >>>>>>> +
> >>>>>>>             ret = vhost_vdpa_get_dev_features(dev, &dev_features);
> >>>>>>>             if (ret != 0) {
> >>>>>>>                 error_report("Can't get vdpa device features, got (%d)", ret);
>