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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 18, 2020 at 05:58:28PM +0200, David Hildenbrand wrote:
> On 18.06.20 17:14, Alexander Duyck wrote:
> > On Thu, Jun 18, 2020 at 5:54 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 13.06.20 22:07, Alexander Duyck wrote:
> >>> On Tue, May 26, 2020 at 9:14 PM Alexander Duyck
> >>> <alexander.duyck@gmail.com> 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.
> >>>>
> >>>> Acked-by: David Hildenbrand <david@redhat.com>
> >>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> ---
> >>>>  hw/virtio/virtio-balloon.c         |   78 ++++++++++++++++++------------------
> >>>>  include/hw/virtio/virtio-balloon.h |   20 +++++----
> >>>>  2 files changed, 49 insertions(+), 49 deletions(-)
> >>>>
> >>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>>> index 3e2ac1104b5f..dc15409b0bb6 100644
> >>>> --- a/hw/virtio/virtio-balloon.c
> >>>> +++ b/hw/virtio/virtio-balloon.c
> >>>
> >>> ...
> >>>
> >>>> @@ -817,14 +817,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()
> >>>>      }
> >>>>  };
> >>>
> >>> So I noticed this patch wasn't in the list of patches pulled, but that
> >>> is probably for the best since I believe the change above might have
> >>> broken migration as VMSTATE_UINT32 does a stringify on the first
> >>> parameter.
> >>
> >> Indeed, it's the name of the vmstate field. But I don't think it is
> >> relevant for migration. It's and indicator if a field is valid and it's
> >> used in traces/error messages.
> >>
> >> See git grep "field->name"
> >>
> >> I don't think renaming this is problematic. Can you rebase and resent?
> >> Thanks!
> > 
> > Okay, I will.
> > 
> >>> Any advice on how to address it, or should I just give up on renaming
> >>> free_page_report_cmd_id and free_page_report_status?
> >>>
> >>> Looking at this I wonder why we even need to migrate these values? It
> >>> seems like if we are completing a migration the cmd_id should always
> >>> be "DONE" shouldn't it? It isn't as if we are going to migrate the
> >>
> >> The *status* should be DONE IIUC. The cmd_id might be relevant, no? It's
> >> always incremented until it wraps.
> > 
> > The thing is, the cmd_id visible to the driver if the status is DONE
> > is the cmd_id value for DONE. So as long as the driver acknowledges
> > the value we could essentially start over the cmd_id without any
> > negative effect. The driver would have to put down a new descriptor to
> > start a block of hinting in order to begin reporting again so there
> > shouldn't be any risk of us falsely hinting pages that were in a
> > previous epoch.
> > 
> > Ugh, although now looking at it I think we might have a bug in the
> > QEMU code in that the driver could in theory force its way past a
> > "STOP" by just replaying the last command_id descriptor and then keep
> > going. Should be a pretty easy fix though as we should only allow a
> > transition to S_START if the status is S_REQUESTED/
> 
> Ugh, ...
> 
> @MST, you might have missed that in another discussion, what's your
> general opinion about removing free page hinting in QEMU (and Linux)? We
> keep finding issues in the QEMU implementation, including non-trivial
> ones, and have to speculate about the actual semantics. I can see that
> e.g., libvirt does not support it yet.

Not maintaining two similar features sounds attractive.

I'm still trying to get my head around the list of issues.  So far they
all look kind of minor to me.  Would you like to summarize them
somewhere?
The appeal of hinting is that it's 0 overhead outside migration,
and pains were taken to avoid keeping pages locked while
hypervisor is busy.

If we are to drop hinting completely we need to show that reporting
can be comparable, and we'll probably want to add a mode for
reporting that behaves somewhat similarly.

-- 
MST