| Message ID | 20180717224737.14019.3324.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 17, 2018 at 04:47:37PM -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. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > balloon.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/balloon.c b/balloon.c > index 6bf0a9681377..2a6a7e1a22a0 100644 > --- a/balloon.c > +++ b/balloon.c > @@ -37,16 +37,17 @@ > static QEMUBalloonEvent *balloon_event_fn; > static QEMUBalloonStatus *balloon_stat_fn; > static void *balloon_opaque; > -static bool balloon_inhibited; > +static int balloon_inhibited; > > bool qemu_balloon_is_inhibited(void) > { > - return balloon_inhibited; > + return balloon_inhibited > 0; > } > > void qemu_balloon_inhibit(bool state) > { > - balloon_inhibited = state; > + balloon_inhibited += (state ? 1 : -1); > + assert(balloon_inhibited >= 0); Better do it atomically? > } > > static bool have_balloon(Error **errp) > > Regards,
On Wed, 18 Jul 2018 14:40:15 +0800 Peter Xu <peterx@redhat.com> wrote: > On Tue, Jul 17, 2018 at 04:47:37PM -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. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > balloon.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/balloon.c b/balloon.c > > index 6bf0a9681377..2a6a7e1a22a0 100644 > > --- a/balloon.c > > +++ b/balloon.c > > @@ -37,16 +37,17 @@ > > static QEMUBalloonEvent *balloon_event_fn; > > static QEMUBalloonStatus *balloon_stat_fn; > > static void *balloon_opaque; > > -static bool balloon_inhibited; > > +static int balloon_inhibited; > > > > bool qemu_balloon_is_inhibited(void) > > { > > - return balloon_inhibited; > > + return balloon_inhibited > 0; > > } > > > > void qemu_balloon_inhibit(bool state) > > { > > - balloon_inhibited = state; > > + balloon_inhibited += (state ? 1 : -1); > > + assert(balloon_inhibited >= 0); > > Better do it atomically? I'd assumed we're protected by the BQL anywhere this is called. Is that not the case? Generally when I try to add any sort of locking to QEMU it's shot down because the code paths are already serialized. Thanks, Alex
On Wed, Jul 18, 2018 at 10:37:36AM -0600, Alex Williamson wrote: > On Wed, 18 Jul 2018 14:40:15 +0800 > Peter Xu <peterx@redhat.com> wrote: > > > On Tue, Jul 17, 2018 at 04:47:37PM -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. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > balloon.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/balloon.c b/balloon.c > > > index 6bf0a9681377..2a6a7e1a22a0 100644 > > > --- a/balloon.c > > > +++ b/balloon.c > > > @@ -37,16 +37,17 @@ > > > static QEMUBalloonEvent *balloon_event_fn; > > > static QEMUBalloonStatus *balloon_stat_fn; > > > static void *balloon_opaque; > > > -static bool balloon_inhibited; > > > +static int balloon_inhibited; > > > > > > bool qemu_balloon_is_inhibited(void) > > > { > > > - return balloon_inhibited; > > > + return balloon_inhibited > 0; > > > } > > > > > > void qemu_balloon_inhibit(bool state) > > > { > > > - balloon_inhibited = state; > > > + balloon_inhibited += (state ? 1 : -1); > > > + assert(balloon_inhibited >= 0); > > > > Better do it atomically? > > I'd assumed we're protected by the BQL anywhere this is called. Is > that not the case? Generally when I try to add any sort of locking to > QEMU it's shot down because the code paths are already serialized. Ah I see. :-) But I guess current code might call these without BQL. For example, qemu_balloon_inhibit() has: postcopy_ram_listen_thread qemu_loadvm_state_main loadvm_process_command loadvm_postcopy_handle_listen postcopy_ram_enable_notify qemu_balloon_inhibit While the whole stack is inside a standalone thread to load QEMU for postcopy incoming migration rather than the main thread. Thanks,
diff --git a/balloon.c b/balloon.c index 6bf0a9681377..2a6a7e1a22a0 100644 --- a/balloon.c +++ b/balloon.c @@ -37,16 +37,17 @@ static QEMUBalloonEvent *balloon_event_fn; static QEMUBalloonStatus *balloon_stat_fn; static void *balloon_opaque; -static bool balloon_inhibited; +static int balloon_inhibited; bool qemu_balloon_is_inhibited(void) { - return balloon_inhibited; + return balloon_inhibited > 0; } void qemu_balloon_inhibit(bool state) { - balloon_inhibited = state; + balloon_inhibited += (state ? 1 : -1); + assert(balloon_inhibited >= 0); } static bool have_balloon(Error **errp)
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. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- balloon.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)