| Message ID | 20180807193125.30378-2-alex.williamson@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Balloon inhibit enhancements, vfio restriction | expand |
On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote: > A simple true/false internal state does not allow multiple users. Fix > this within the existing interface by converting to a counter, so long > as the counter is elevated, ballooning is inhibited. > > Reviewed-by: David Hildenbrand <david@redhat.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > balloon.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/balloon.c b/balloon.c > index 6bf0a9681377..931987983858 100644 > --- a/balloon.c > +++ b/balloon.c > @@ -26,6 +26,7 @@ > > #include "qemu/osdep.h" > #include "qemu-common.h" > +#include "qemu/atomic.h" > #include "exec/cpu-common.h" > #include "sysemu/kvm.h" > #include "sysemu/balloon.h" > @@ -37,16 +38,22 @@ > static QEMUBalloonEvent *balloon_event_fn; > static QEMUBalloonStatus *balloon_stat_fn; > static void *balloon_opaque; > -static bool balloon_inhibited; > +static int balloon_inhibit_count; > > bool qemu_balloon_is_inhibited(void) > { > - return balloon_inhibited; > + return atomic_read(&balloon_inhibit_count) > 0; > } > > void qemu_balloon_inhibit(bool state) > { > - balloon_inhibited = state; > + if (state) { > + atomic_inc(&balloon_inhibit_count); > + } else { > + atomic_dec(&balloon_inhibit_count); > + } > + > + assert(atomic_read(&balloon_inhibit_count) >= 0); > } > > static bool have_balloon(Error **errp) This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED. Is this necessarily a good idea? > -- > 2.18.0
On Tue, 7 Aug 2018 22:44:11 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote: > > A simple true/false internal state does not allow multiple users. Fix > > this within the existing interface by converting to a counter, so long > > as the counter is elevated, ballooning is inhibited. > > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > balloon.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/balloon.c b/balloon.c > > index 6bf0a9681377..931987983858 100644 > > --- a/balloon.c > > +++ b/balloon.c > > @@ -26,6 +26,7 @@ > > > > #include "qemu/osdep.h" > > #include "qemu-common.h" > > +#include "qemu/atomic.h" > > #include "exec/cpu-common.h" > > #include "sysemu/kvm.h" > > #include "sysemu/balloon.h" > > @@ -37,16 +38,22 @@ > > static QEMUBalloonEvent *balloon_event_fn; > > static QEMUBalloonStatus *balloon_stat_fn; > > static void *balloon_opaque; > > -static bool balloon_inhibited; > > +static int balloon_inhibit_count; > > > > bool qemu_balloon_is_inhibited(void) > > { > > - return balloon_inhibited; > > + return atomic_read(&balloon_inhibit_count) > 0; > > } > > > > void qemu_balloon_inhibit(bool state) > > { > > - balloon_inhibited = state; > > + if (state) { > > + atomic_inc(&balloon_inhibit_count); > > + } else { > > + atomic_dec(&balloon_inhibit_count); > > + } > > + > > + assert(atomic_read(&balloon_inhibit_count) >= 0); > > } > > > > static bool have_balloon(Error **errp) > > This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED. > Is this necessarily a good idea? This is existing balloon inhibitor behavior, but do you have some reason to suspect WILLNEED is necessary? It's my impression that WILLNEED is a purely optional prefetch directive that's entirely unnecessary if the page wasn't previously zapped with WONTNEED. If the page was zapped, it will fault in on demand, potentially with higher latency, but functionally correct. With vfio usage of the inhibitor, we expect pinning to fault in any previously ballooned pages, so calling WILLNEED on pages where the inhibit count is elevated due to an assigned device seems unnecessary. Thanks, Alex
On Tue, Aug 07, 2018 at 02:08:26PM -0600, Alex Williamson wrote: > On Tue, 7 Aug 2018 22:44:11 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote: > > > A simple true/false internal state does not allow multiple users. Fix > > > this within the existing interface by converting to a counter, so long > > > as the counter is elevated, ballooning is inhibited. > > > > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > balloon.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/balloon.c b/balloon.c > > > index 6bf0a9681377..931987983858 100644 > > > --- a/balloon.c > > > +++ b/balloon.c > > > @@ -26,6 +26,7 @@ > > > > > > #include "qemu/osdep.h" > > > #include "qemu-common.h" > > > +#include "qemu/atomic.h" > > > #include "exec/cpu-common.h" > > > #include "sysemu/kvm.h" > > > #include "sysemu/balloon.h" > > > @@ -37,16 +38,22 @@ > > > static QEMUBalloonEvent *balloon_event_fn; > > > static QEMUBalloonStatus *balloon_stat_fn; > > > static void *balloon_opaque; > > > -static bool balloon_inhibited; > > > +static int balloon_inhibit_count; > > > > > > bool qemu_balloon_is_inhibited(void) > > > { > > > - return balloon_inhibited; > > > + return atomic_read(&balloon_inhibit_count) > 0; > > > } > > > > > > void qemu_balloon_inhibit(bool state) > > > { > > > - balloon_inhibited = state; > > > + if (state) { > > > + atomic_inc(&balloon_inhibit_count); > > > + } else { > > > + atomic_dec(&balloon_inhibit_count); > > > + } > > > + > > > + assert(atomic_read(&balloon_inhibit_count) >= 0); > > > } > > > > > > static bool have_balloon(Error **errp) > > > > This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED. > > Is this necessarily a good idea? > > This is existing balloon inhibitor behavior, but do you have some > reason to suspect WILLNEED is necessary? It's my impression that > WILLNEED is a purely optional prefetch directive that's entirely > unnecessary if the page wasn't previously zapped with WONTNEED. If the > page was zapped, it will fault in on demand, potentially with higher > latency, but functionally correct. With vfio usage of the inhibitor, > we expect pinning to fault in any previously ballooned pages, so > calling WILLNEED on pages where the inhibit count is elevated due to an > assigned device seems unnecessary. Thanks, > > Alex So inhibit interface isn't great - it was designed for a single user: the post-copy. My point is generalizing it by reference counting does not seem to make for a sensible interface. I agree vfio itself pages in all guest memory but how will other users of this interface handle it? What would a sensible interface look like, and how it would account for post-copy requirements, I'm not yet sure. Ideas welcome.
diff --git a/balloon.c b/balloon.c index 6bf0a9681377..931987983858 100644 --- a/balloon.c +++ b/balloon.c @@ -26,6 +26,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/atomic.h" #include "exec/cpu-common.h" #include "sysemu/kvm.h" #include "sysemu/balloon.h" @@ -37,16 +38,22 @@ static QEMUBalloonEvent *balloon_event_fn; static QEMUBalloonStatus *balloon_stat_fn; static void *balloon_opaque; -static bool balloon_inhibited; +static int balloon_inhibit_count; bool qemu_balloon_is_inhibited(void) { - return balloon_inhibited; + return atomic_read(&balloon_inhibit_count) > 0; } void qemu_balloon_inhibit(bool state) { - balloon_inhibited = state; + if (state) { + atomic_inc(&balloon_inhibit_count); + } else { + atomic_dec(&balloon_inhibit_count); + } + + assert(atomic_read(&balloon_inhibit_count) >= 0); } static bool have_balloon(Error **errp)