Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <address@hidden> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <address@hidden>
> >
> > In an upcoming patch a feature named Free Page Reporting is about to be
> > added. In order to avoid any confusion we should drop the use of the word
> > 'report' when referring to Free Page Hinting. So what this patch does is go
> > through and replace all instances of 'report' with 'hint" when we are
> > referring to free page hinting.
> >
> > Signed-off-by: Alexander Duyck <address@hidden>
> > ---
> >  hw/virtio/virtio-balloon.c         |   74 
> > ++++++++++++++++++------------------
> >  include/hw/virtio/virtio-balloon.h |   20 +++++-----
> >  2 files changed, 47 insertions(+), 47 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index a4729f7fc930..a1d6fb52c876 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >              ret = false;
> >              goto out;
> >          }
> > -        if (id == dev->free_page_report_cmd_id) {
> > -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > +        if (id == dev->free_page_hint_cmd_id) {
> > +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
> >          } else {
> >              /*
> >               * Stop the optimization only when it has started. This
> >               * avoids a stale stop sign for the previous command.
> >               */
> > -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> > +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >              }
> >          }
> >      }
> >
> >      if (elem->in_num) {
> > -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> >              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> >                                        elem->in_sg[0].iov_len);
> >          }
> > @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void 
> > *opaque)
> >          qemu_mutex_unlock(&dev->free_page_lock);
> >          virtio_notify(vdev, vq);
> >        /*
> > -       * Start to poll the vq once the reporting started. Otherwise, 
> > continue
> > +       * Start to poll the vq once the hinting started. Otherwise, continue
> >         * only when there are entries on the vq, which need to be given 
> > back.
> >         */
> >      } while (continue_to_get_hints ||
> > -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
> > +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
> >      virtio_queue_set_notification(vq, 1);
> >  }
> >
> > @@ -531,14 +531,14 @@ static void 
> > virtio_balloon_free_page_start(VirtIOBalloon *s)
> >          return;
> >      }
> >
> > -    if (s->free_page_report_cmd_id == UINT_MAX) {
> > -        s->free_page_report_cmd_id =
> > -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > +    if (s->free_page_hint_cmd_id == UINT_MAX) {
> > +        s->free_page_hint_cmd_id =
> > +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> >      } else {
> > -        s->free_page_report_cmd_id++;
> > +        s->free_page_hint_cmd_id++;
> >      }
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
> >      virtio_notify_config(vdev);
> >  }
> >
> > @@ -546,18 +546,18 @@ static void 
> > virtio_balloon_free_page_stop(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
> > +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
> >          /*
> >           * The lock also guarantees us that the
> >           * virtio_ballloon_get_free_page_hints exits after the
> > -         * free_page_report_status is set to S_STOP.
> > +         * free_page_hint_status is set to S_STOP.
> >           */
> >          qemu_mutex_lock(&s->free_page_lock);
> >          /*
> >           * The guest hasn't done the reporting, so host sends a 
> > notification
> >           * to the guest to actively stop the reporting.
> >           */
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >          qemu_mutex_unlock(&s->free_page_lock);
> >          virtio_notify_config(vdev);
> >      }
> > @@ -567,15 +567,15 @@ static void 
> > virtio_balloon_free_page_done(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
> >      virtio_notify_config(vdev);
> >  }
> >
> >  static int
> > -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> > +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
> >  {
> >      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
> > -                                      free_page_report_notify);
> > +                                      free_page_hint_notify);
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      PrecopyNotifyData *pnd = data;
> >
> > @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon 
> > *s)
> >      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          return offsetof(struct virtio_balloon_config, poison_val);
> >      }
> > -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> > +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
> >  }
> >
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t 
> > *config_data)
> > @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice 
> > *vdev, uint8_t *config_data)
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> >
> > -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
> > -        config.free_page_report_cmd_id =
> > -                       cpu_to_le32(dev->free_page_report_cmd_id);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
> > -        config.free_page_report_cmd_id =
> > +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> > +        config.free_page_hint_cmd_id =
> > +                       cpu_to_le32(dev->free_page_hint_cmd_id);
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
> > -        config.free_page_report_cmd_id =
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
> >      }
> >
> > @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void 
> > *opaque, int version_id)
> >      return 0;
> >  }
> >
> > -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
> > +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
> >      .name = "virtio-balloon-device/free-page-report",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = virtio_balloon_free_page_support,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> > -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > @@ -766,7 +766,7 @@ static const VMStateDescription 
> > vmstate_virtio_balloon_device = {
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription * []) {
> > -        &vmstate_virtio_balloon_free_page_report,
> > +        &vmstate_virtio_balloon_free_page_hint,
> >          NULL
> >      }
> >  };
> > @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState 
> > *dev, Error **errp)
> >                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> >                                             
> > virtio_balloon_handle_free_page_vq);
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > -        s->free_page_report_cmd_id =
> > -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > -        s->free_page_report_notify.notify =
> > -                                       
> > virtio_balloon_free_page_report_notify;
> > -        precopy_add_notifier(&s->free_page_report_notify);
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> > +        s->free_page_hint_cmd_id =
> > +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> > +        s->free_page_hint_notify.notify =
> > +                                       
> > virtio_balloon_free_page_hint_notify;
> > +        precopy_add_notifier(&s->free_page_hint_notify);
> >          if (s->iothread) {
> >              object_ref(OBJECT(s->iothread));
> >              s->free_page_bh = 
> > aio_bh_new(iothread_get_aio_context(s->iothread),
> > @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState 
> > *dev, Error **errp)
> >      if (virtio_balloon_free_page_support(s)) {
> >          qemu_bh_delete(s->free_page_bh);
> >          virtio_balloon_free_page_stop(s);
> > -        precopy_remove_notifier(&s->free_page_report_notify);
> > +        precopy_remove_notifier(&s->free_page_hint_notify);
> >      }
> >      balloon_stats_destroy_timer(s);
> >      qemu_remove_balloon_handler(s);
> > diff --git a/include/hw/virtio/virtio-balloon.h 
> > b/include/hw/virtio/virtio-balloon.h
> > index d1c968d2376e..108cff97e71a 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -23,7 +23,7 @@
> >  #define VIRTIO_BALLOON(obj) \
> >          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
> >
> > -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
> > +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
> >
> >  typedef struct virtio_balloon_stat VirtIOBalloonStat;
> >
> > @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
> >         uint64_t val;
> >  } VirtIOBalloonStatModern;
> >
> > -enum virtio_balloon_free_page_report_status {
> > -    FREE_PAGE_REPORT_S_STOP = 0,
> > -    FREE_PAGE_REPORT_S_REQUESTED = 1,
> > -    FREE_PAGE_REPORT_S_START = 2,
> > -    FREE_PAGE_REPORT_S_DONE = 3,
> > +enum virtio_balloon_free_page_hint_status {
> > +    FREE_PAGE_HINT_S_STOP = 0,
> > +    FREE_PAGE_HINT_S_REQUESTED = 1,
> > +    FREE_PAGE_HINT_S_START = 2,
> > +    FREE_PAGE_HINT_S_DONE = 3,
> >  };
> >
> >  typedef struct VirtIOBalloon {
> >      VirtIODevice parent_obj;
> >      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> > -    uint32_t free_page_report_status;
> > +    uint32_t free_page_hint_status;
> >      uint32_t num_pages;
> >      uint32_t actual;
> > -    uint32_t free_page_report_cmd_id;
> > +    uint32_t free_page_hint_cmd_id;
> >      uint64_t stats[VIRTIO_BALLOON_S_NR];
> >      VirtQueueElement *stats_vq_elem;
> >      size_t stats_vq_offset;
> > @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
> >      QEMUBH *free_page_bh;
> >      /*
> >       * Lock to synchronize threads to access the free page reporting 
> > related
> > -     * fields (e.g. free_page_report_status).
> > +     * fields (e.g. free_page_hint_status).
> >       */
> >      QemuMutex free_page_lock;
> >      QemuCond  free_page_cond;
> > @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
> >       * stopped.
> >       */
> >      bool block_iothread;
> > -    NotifierWithReturn free_page_report_notify;
> > +    NotifierWithReturn free_page_hint_notify;
> >      int64_t stats_last_update;
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> >
>
> Maybe split out the change to "free_page_report_cmd_id" in the uapi
> (meaning: move that to a separate patch). So you can move forward with
> most of this series (moving patch #1 and the free_page_report_cmd_id
> change to the very last of this series) without depending on that change
> to go upstream.

Okay. I can split it if that is needed. Any specific reason for
splitting it I should cite in the patch? From what I can tell Michael
has already accepted the renamed and pushed it to Linus.

Thanks.

- Alex