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

[David Gibson]

On Tue, Mar 05, 2019 at 12:02:30PM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 05:10:42PM +0100, David Hildenbrand wrote:
> > 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.
> 
> OK, I don't mind either way.

Ok, I'll take the property out.  Or at least, move the new property to
a private experimental patch to make it easier to try to benchmark.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature