| Message ID | 20190305051134.27930-3-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-balloon: Some further fixes | expand |
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 :) > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/virtio/virtio-balloon.c | 15 +++++++++++++++ > include/hw/virtio/virtio-balloon.h | 1 + > 2 files changed, 16 insertions(+) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index e5e82b556d..69968502d9 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -146,6 +146,20 @@ static void balloon_deflate_page(VirtIOBalloon *balloon, > balloon->pbp = NULL; > } > } > + > + if (balloon->hint_on_deflate) { > + void *host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1)); > + int ret; > + > + /* When a page is deflated, we hint the whole host page it > + * lives on, since we can't do anything smaller */ > + ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED); > + if (ret != 0) { > + warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s", > + strerror(errno)); > + /* Otherwise ignore, failing to page hint shouldn't be fatal */ > + } > + } > } > > static const char *balloon_stat_names[] = { > @@ -622,6 +636,7 @@ static const VMStateDescription vmstate_virtio_balloon = { > static Property virtio_balloon_properties[] = { > DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), > + DEFINE_PROP_BOOL("hint-on-deflate", VirtIOBalloon, hint_on_deflate, true), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > index 99dcd6d105..69732cedaa 100644 > --- a/include/hw/virtio/virtio-balloon.h > +++ b/include/hw/virtio/virtio-balloon.h > @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon { > int64_t stats_poll_interval; > uint32_t host_features; > PartiallyBalloonedPage *pbp; > + bool hint_on_deflate; > } VirtIOBalloon; > > #endif >
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. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/virtio/virtio-balloon.c | 15 +++++++++++++++ > > include/hw/virtio/virtio-balloon.h | 1 + > > 2 files changed, 16 insertions(+) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index e5e82b556d..69968502d9 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -146,6 +146,20 @@ static void balloon_deflate_page(VirtIOBalloon *balloon, > > balloon->pbp = NULL; > > } > > } > > + > > + if (balloon->hint_on_deflate) { > > + void *host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1)); > > + int ret; > > + > > + /* When a page is deflated, we hint the whole host page it > > + * lives on, since we can't do anything smaller */ > > + ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED); > > + if (ret != 0) { > > + warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s", > > + strerror(errno)); > > + /* Otherwise ignore, failing to page hint shouldn't be fatal */ > > + } > > + } > > } > > > > static const char *balloon_stat_names[] = { > > @@ -622,6 +636,7 @@ static const VMStateDescription vmstate_virtio_balloon = { > > static Property virtio_balloon_properties[] = { > > DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features, > > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), > > + DEFINE_PROP_BOOL("hint-on-deflate", VirtIOBalloon, hint_on_deflate, true), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > > index 99dcd6d105..69732cedaa 100644 > > --- a/include/hw/virtio/virtio-balloon.h > > +++ b/include/hw/virtio/virtio-balloon.h > > @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon { > > int64_t stats_poll_interval; > > uint32_t host_features; > > PartiallyBalloonedPage *pbp; > > + bool hint_on_deflate; > > } VirtIOBalloon; > > > > #endif > > > > > -- > > Thanks, > > David / dhildenb
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.
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. > -- > > Thanks, > > David / dhildenb
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 :) Frankly, I agree. But mst was nervous about us making that change to the balloon's previous behaviour.
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.
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index e5e82b556d..69968502d9 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -146,6 +146,20 @@ static void balloon_deflate_page(VirtIOBalloon *balloon, balloon->pbp = NULL; } } + + if (balloon->hint_on_deflate) { + void *host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1)); + int ret; + + /* When a page is deflated, we hint the whole host page it + * lives on, since we can't do anything smaller */ + ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED); + if (ret != 0) { + warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s", + strerror(errno)); + /* Otherwise ignore, failing to page hint shouldn't be fatal */ + } + } } static const char *balloon_stat_names[] = { @@ -622,6 +636,7 @@ static const VMStateDescription vmstate_virtio_balloon = { static Property virtio_balloon_properties[] = { DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features, VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), + DEFINE_PROP_BOOL("hint-on-deflate", VirtIOBalloon, hint_on_deflate, true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h index 99dcd6d105..69732cedaa 100644 --- a/include/hw/virtio/virtio-balloon.h +++ b/include/hw/virtio/virtio-balloon.h @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon { int64_t stats_poll_interval; uint32_t host_features; PartiallyBalloonedPage *pbp; + bool hint_on_deflate; } VirtIOBalloon; #endif
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. 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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/virtio/virtio-balloon.c | 15 +++++++++++++++ include/hw/virtio/virtio-balloon.h | 1 + 2 files changed, 16 insertions(+)