[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v25 QEMU 3/3] virtio-balloon: Replace free page hinting references to 'report' with 'hint'
>> >> 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 consider free page hinting (in QEMU) to be in an unmaintainable state (and it looks like Alex and I are fixing a feature we don't actually intend to use / not aware of users). In contrast to that, the free page reporting functionality/implementation is a walk in the park. > > 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? Some things I still have in my mind 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. 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 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) As Alex mentioned, it is not even guaranteed in QEMU that we receive a zero page on the destination, it could also be something else (e.g., previously migrated values). 3. If I am not wrong, the iothread works in virtio_ballloon_get_free_page_hints() on the virtqueue only with holding the free_page_lock (no BQL). Assume we're migrating, the iothread is active, and the guest triggers a device reset. virtio_balloon_device_reset() will trigger a virtio_balloon_free_page_stop(s). That won't actually wait for the iothread to stop, it will only temporarily lock free_page_lock and update s->free_page_report_status. I think there can be a race between the device reset and the iothread. Once virtio_balloon_free_page_stop() returned, virtio_ballloon_get_free_page_hints() can still call - virtio_queue_set_notification(vq, 0); - virtio_queue_set_notification(vq, 1); - virtio_notify(vdev, vq); - virtqueue_pop() I doubt this is very nice. 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). > 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. This feature obviously saw no proper review. -- Thanks, David / dhildenb
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]