[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ
From: |
Jason Wang |
Subject: |
Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ |
Date: |
Thu, 17 Feb 2022 14:02:05 +0800 |
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.
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?
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.
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);
> >
>
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Eugenio Perez Martin, 2022/02/01
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Jason Wang, 2022/02/08
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Eugenio Perez Martin, 2022/02/16
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ,
Jason Wang <=
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Eugenio Perez Martin, 2022/02/17
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Jason Wang, 2022/02/22
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Eugenio Perez Martin, 2022/02/22
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Jason Wang, 2022/02/22
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Eugenio Perez Martin, 2022/02/23
- Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ, Jason Wang, 2022/02/23