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

[David Hildenbrand <david@redhat.com>]

On 24.04.20 16:56, Alexander Duyck wrote:
> On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 22.04.20 20:21, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> 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 <alexander.h.duyck@linux.intel.com>
>>> ---
>>>  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.

Oh, I missed that - forget what I said. I thought this would still be
stuck in -next.

Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb