Re: [Qemu-devel] [RFC 2/2] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Hildenbrand <david@redhat.com>]

On 05.03.19 17:03, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 03:15:38PM +0100, David Hildenbrand wrote:
>> On 05.03.19 06:11, David Gibson wrote:
>>> Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
>>> deflate", the balloon device issued an madvise() MADV_WILLNEED on
>>> pages removed from the balloon.  That would hint to the host kernel
>>> that the pages were likely to be needed by the guest in the near
>>> future.
>>>
>>> It's unclear if this is actually valuable or not, and so f6deb6d9
>>> removed this, essentially ignoring balloon deflate requests.  However,
>>> concerns have been raised that this might cause a performance
>>> regression by causing extra latency for the guest in certain
>>> configurations.
>>
>> I mean, it will mainly create page tables as far as I know. Any write to
>> a page will have an overhead either way (COW zero page). Reads *might*
>> be faster.
>>
>> As we are working on 4k granularity in the balloon (and doing
>> MADV_DONTNEED on 4k granularity!), there will most probably be page
>> tables already either way. A page table could only be zapped if all
>> pages of that page table are MADV_DONTNEED'ed (or I assume never were
>> touched), and I am not sure if "random MADV_DONTNEED'ing of 4k pages"
>> will actually get rid of page tables (my assumption would be: only if a
>> complete range is zapped at once). I haven't looked into the details,
>> though (plenty of other stuff to do).
>>
>> I am not sure if I share the concerns. Real-time workload should never
>> use the virtio-balloon in a way that anything like that would be possible.
>>
>>>
>>> So, until we can get actual benchmark data to see if that's the case,
>>> this restores (by default) the old behaviour, issuing a MADV_WILLNEED
>>> when a page is removed from the balloon.  A new property on the
>>> balloon device "hint-on-deflate" can be set to false to remove this
>>> behaviour for testing.
>>
>> This is certainly a good approach for you to finally be able to leave
>> the ugly land of virtio-balloon :)
>>
>> But at least to me, this looks completely useless. I'll be happy to be
>> proven wrong as always :)
> 
> Point is if we don't intend to extend balloon any further then let's
> not make random untested changes to its behaviour.

Agreed, but than I'd much rather want to avoid the original change
instead of adding a new property that most probably nobody will ever use.

-- 

Thanks,

David / dhildenb