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

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

On Wed, Jun 24, 2020 at 05:28:59PM +0200, David Hildenbrand wrote:
> > So at the high level the idea was simple, we just clear the dirty bit
> > when page is hinted, unless we sent a new command since. Implementation
> > was reviewed by migration maintainers. If there's a consensus the code
> > is written so badly we can't maintain it, maybe we should remove it.
> > Which parts are unmaintainable in your eyes - migration or virtio ones?
> 
> QEMU implementation without a propert virtio specification. I hope that
> we can *at least* finally document the expected behavior. Alex gave it a
> shot, and I was hoping that Wei could jump in to clarify, help move this
> forward ... after all he implemented (+designed?) the feature and the
> virtio interface.
> 
> > Or maybe it's the general thing that interface was never specced
> > properly.
> 
> Yes, a spec would be definitely a good starter ...
> 
> [...]
> 
> >>
> >> 1. If migration fails during RAM precopy, the guest will never receive a
> >> DONE notification. Probably easy to fix.
> >>
> >> 2. Unclear semantics. Alex tried to document what the actual semantics
> >> of hinted pages are.
> > 
> > I'll reply to that now.
> > 
> >> Assume the following in the guest to a previously
> >> hinted page
> >>
> >> /* page was hinted and is reused now */
> >> if (page[x] != Y)
> >> 	page[x] == Y;
> >> /* migration ends, we now run on the destination */
> >> BUG_ON(page[x] != Y);
> >> /* BUG, because the content chan
> > 
> > The assumption hinting makes is that data in page is writtent to before it's used.
> > 
> > 
> >> A guest can observe that. And that could be a random driver that just
> >> allocated a page.
> >>
> >> (I *assume* in Linux we might catch that using kasan, but I am not 100%
> >> sure, also, the actual semantics to document are unclear - e.g., for
> >> other guests)
> > 
> > I think it's basically simple: hinting means it's ok to
> > fill page with trash unless it has been modified since the command
> > ID supplied.
> 
> Yeah, I quite dislike the semantics, especially, as they are different
> to well-know semantics as e.g., represent in MADV_FREE. Getting changed
> content when reading is really weird. But it seemed to be easier to
> implement (low hanging fruit) and nobody complained back then. Well, now
> we are stuck with it.
> 
> [..]

The difference with MADV_FREE is
- asynchronous (using cmd id to synchronize)
- zero not guaranteed

right?

> > 
> >> There are other concerns I had regarding the iothread (e.g., while
> >> reporting is active, virtio_ballloon_get_free_page_hints() is
> >> essentially a busy loop, in contrast to documented -
> >> continue_to_get_hints will always be true).
> > 
> > So that would be a performance issue you are suggesting, right?
> 
> I misread the code, so that comment does no longer apply (see other
> message).
> 
> > 
> >>> 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.
> >>
> >> Depends on the actual users. If we're dropping a feature that nobody is
> >> actively using, I don't think we have to show anything.
> > 
> > 
> > I don't know how to find out. So far it doesn't look like we found
> > any common data corruptions that would indicate no one can use it safely.
> > Races around reset aren't all that uncommon but I don't think that
> > qualifies as a deal breaker.
> 
> As I said, there are no libvirt bindings, so at least anything using
> libvirt does not use it. I'd be curious about actual users.
> 
> > 
> > I find the idea of asynchronously sending hints to host without
> > waiting for them to be processed intriguing. Not something
> > I'd work on implementing if we had reporting originally,
> > but since it's there I'm not sure we should just discard it
> > at this point.
> > 
> >> This feature obviously saw no proper review.
> > 
> > I did my best but obviously missed some things.
> 
> Yeah, definitely not your fault. People cannot expect maintainers to
> review everything in detail.
> 
> -- 
> Thanks,
> 
> David / dhildenb