Re: [PATCH v22 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
On 27.04.20 17:57, Alexander Duyck wrote:
> On Mon, Apr 27, 2020 at 8:11 AM David Hildenbrand wrote:
>>
>> On 27.04.20 17:08, Alexander Duyck wrote:
>>> On Mon, Apr 27, 2020 at 1:15 AM David Hildenbrand wrote:
There is only one wrong comment remaining I think. Something like
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a1d6fb52c8..1b2127c04c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -554,8 +554,8 @@ static void
virtio_balloon_free_page_stop(VirtIOBalloon *s)
*/
qemu_mutex_lock(>free_page_lock);
/*
- * The guest hasn't done the reporting, so host sends a
notification
- * to the guest to actively stop the reporting.
+ * The guest isn't done with hinting, so the host sends a
notification
+ * to the guest to actively stop the hinting.
>>>
>>> I'll probably tweak it slightly and drop the "with". So the comment will
>>> read:
>>> /*
>>> * The guest isn't done hinting, so host sends a notification
>>
>> I always feel like "so host sends" sounds wrong ("the host"). But I am
>> not a native speaker.
>
> Actually it might read better to get rid of "the host" entirely to
> make it more of an imperative statement rather than a declarative one.
> Maybe something more like:
> /*
> * The guest isn't done hinting, so send a notification
> * to the guest to actively stop the hinting.
> */
>
Sounds good :)
--
Thanks,
David / dhildenb
Re: [PATCH v22 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
On Mon, Apr 27, 2020 at 8:11 AM David Hildenbrand wrote:
>
> On 27.04.20 17:08, Alexander Duyck wrote:
> > On Mon, Apr 27, 2020 at 1:15 AM David Hildenbrand wrote:
> >>
> >> There is only one wrong comment remaining I think. Something like
> >>
> >> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >> index a1d6fb52c8..1b2127c04c 100644
> >> --- a/hw/virtio/virtio-balloon.c
> >> +++ b/hw/virtio/virtio-balloon.c
> >> @@ -554,8 +554,8 @@ static void
> >> virtio_balloon_free_page_stop(VirtIOBalloon *s)
> >> */
> >> qemu_mutex_lock(>free_page_lock);
> >> /*
> >> - * The guest hasn't done the reporting, so host sends a
> >> notification
> >> - * to the guest to actively stop the reporting.
> >> + * The guest isn't done with hinting, so the host sends a
> >> notification
> >> + * to the guest to actively stop the hinting.
> >
> > I'll probably tweak it slightly and drop the "with". So the comment will
> > read:
> > /*
> > * The guest isn't done hinting, so host sends a notification
>
> I always feel like "so host sends" sounds wrong ("the host"). But I am
> not a native speaker.
Actually it might read better to get rid of "the host" entirely to
make it more of an imperative statement rather than a declarative one.
Maybe something more like:
/*
* The guest isn't done hinting, so send a notification
* to the guest to actively stop the hinting.
*/
Re: [PATCH v22 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
On 27.04.20 17:08, Alexander Duyck wrote:
> On Mon, Apr 27, 2020 at 1:15 AM David Hildenbrand wrote:
>>
>> There is only one wrong comment remaining I think. Something like
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index a1d6fb52c8..1b2127c04c 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -554,8 +554,8 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon
>> *s)
>> */
>> qemu_mutex_lock(>free_page_lock);
>> /*
>> - * The guest hasn't done the reporting, so host sends a notification
>> - * to the guest to actively stop the reporting.
>> + * The guest isn't done with hinting, so the host sends a
>> notification
>> + * to the guest to actively stop the hinting.
>
> I'll probably tweak it slightly and drop the "with". So the comment will read:
> /*
> * The guest isn't done hinting, so host sends a notification
I always feel like "so host sends" sounds wrong ("the host"). But I am
not a native speaker.
> * to the guest to actively stop the hinting.
> */
>
> There is one other spot left which is support for migration. The name
> for the VMStateDescription is
> "virtio-balloon-device/free-page-report". I am assuming I cannot
> rename that. Otherwise all other references to report on the balloon
> interface refer to reporting errors from what I can tell.
Yeah, that has to stay for migration to keep working.
--
Thanks,
David / dhildenb
Re: [PATCH v22 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
On Mon, Apr 27, 2020 at 1:15 AM David Hildenbrand wrote: > > There is only one wrong comment remaining I think. Something like > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index a1d6fb52c8..1b2127c04c 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -554,8 +554,8 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon > *s) > */ > qemu_mutex_lock(>free_page_lock); > /* > - * The guest hasn't done the reporting, so host sends a notification > - * to the guest to actively stop the reporting. > + * The guest isn't done with hinting, so the host sends a > notification > + * to the guest to actively stop the hinting. I'll probably tweak it slightly and drop the "with". So the comment will read: /* * The guest isn't done hinting, so host sends a notification * to the guest to actively stop the hinting. */ There is one other spot left which is support for migration. The name for the VMStateDescription is "virtio-balloon-device/free-page-report". I am assuming I cannot rename that. Otherwise all other references to report on the balloon interface refer to reporting errors from what I can tell.
Re: [PATCH v22 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
On 24.04.20 18:50, Alexander Duyck wrote:
> From: Alexander Duyck
>
> 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.
>
> Acked-by: David Hildenbrand
> Signed-off-by: Alexander Duyck
> ---
> 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(>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(>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(>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,
