[Qemu-devel] [RFC 0/2] virtio-balloon: Some further fixes

[Qemu-devel] [RFC 0/2] virtio-balloon: Some further fixes

[David Gibson]

Commits f6deb6d..ee1cd00 made some reworks to the balloon device to
fix behaviour on systems (both host and guest) which don't have 4kiB
pages.  However it introduced a couple of problems, which this series
addresses.

David Gibson (2):
  virtio-balloon: Fix possible guest memory corruption with inflates &
    deflates
  virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

 hw/virtio/virtio-balloon.c         | 63 +++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-balloon.h |  1 +
 2 files changed, 62 insertions(+), 2 deletions(-)

-- 
2.20.1

[Qemu-devel] [RFC 1/2] virtio-balloon: Fix possible guest memory corruption with inflates & deflates

[David Gibson]

This fixes a balloon bug with a nasty consequence - potentially
corrupting guest memory - but which is extremely unlikely to be
triggered in practice.

The balloon always works in 4kiB units, but the host could have a
larger page size on certain platforms.  Since ed48c59 "virtio-balloon:
Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this
by accumulating requests to balloon 4kiB subpages until they formed a
full host page.  Since f6deb6d "virtio-balloon: Remove unnecessary
MADV_WILLNEED on deflate" we essentially ignore deflate requests.

Suppose we have a host with 8kiB pages, and one host page has subpages
A & B.  If we get this sequence of events -
	inflate A
	deflate A
	inflate B
- the current logic will discard the whole host page.  That's
incorrect because the guest has deflated subpage A, and could have
written important data to it.

This patch fixes the problem by adjusting our state information about
partially ballooned host pages when deflate requests are received.

Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size"

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d3f2913a85..e5e82b556d 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
     }
 }
 
+static void balloon_deflate_page(VirtIOBalloon *balloon,
+                                 MemoryRegion *mr, hwaddr offset)
+{
+    void *addr = memory_region_get_ram_ptr(mr) + offset;
+    RAMBlock *rb;
+    size_t rb_page_size;
+    ram_addr_t ram_offset, host_page_base;
+
+    /* XXX is there a better way to get to the RAMBlock than via a
+     * host address? */
+    rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+    rb_page_size = qemu_ram_pagesize(rb);
+    host_page_base = ram_offset & ~(rb_page_size - 1);
+
+    if (balloon->pbp
+        && rb == balloon->pbp->rb
+        && host_page_base == balloon->pbp->base) {
+        int subpages = rb_page_size / BALLOON_PAGE_SIZE;
+
+        /*
+         * This means the guest has asked to discard some of the 4kiB
+         * subpages of a host page, but then changed its mind and
+         * asked to keep them after all.  It's exceedingly unlikely
+         * for a guest to do this in practice, but handle it anyway,
+         * since getting it wrong could mean discarding memory the
+         * guest is still using. */
+        bitmap_clear(balloon->pbp->bitmap,
+                     (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+                     subpages);
+
+        if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
+            free(balloon->pbp);
+            balloon->pbp = NULL;
+        }
+    }
+}
+
 static const char *balloon_stat_names[] = {
    [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
    [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
@@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
             trace_virtio_balloon_handle_output(memory_region_name(section.mr),
                                                pa);
-            if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
-                balloon_inflate_page(s, section.mr, section.offset_within_region);
+            if (!qemu_balloon_is_inhibited()) {
+                if (vq == s->ivq) {
+                    balloon_inflate_page(s, section.mr,
+                                         section.offset_within_region);
+                } else if (vq == s->dvq) {
+                    balloon_deflate_page(s, section.mr, section.offset_within_region);
+                } else {
+                    g_assert_not_reached();
+                }
             }
             memory_region_unref(section.mr);
         }
-- 
2.20.1

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

[David Gibson]

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(+)

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
-- 
2.20.1

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

[David Hildenbrand]

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
> 


-- 

Thanks,

David / dhildenb

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

[Michael S. Tsirkin]

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

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

[David Hildenbrand]

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

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

[Michael S. Tsirkin]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]