| Message ID | 20200506094948.76388-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-mem: Paravirtualized memory hot(un)plug | expand |
* David Hildenbrand (david@redhat.com) wrote: > We want to replace qemu_balloon_inhibit() by something more generic. > Especially, we want to make sure that technologies that really rely on > RAM block discards to work reliably to run mutual exclusive with > technologies that break it. > > E.g., vfio will usually pin all guest memory, turning the virtio-balloon > basically useless and make the VM consume more memory than reported via > the balloon. While the balloon is special already (=> no guarantees, same > behavior possible afer reboots and with huge pages), this will be > different, especially, with virtio-mem. > > Let's implement a way such that we can make both types of technology run > mutually exclusive. We'll convert existing balloon inhibitors in successive > patches and add some new ones. Add the check to > qemu_balloon_is_inhibited() for now. We might want to make > virtio-balloon an acutal inhibitor in the future - however, that > requires more thought to not break existing setups. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > balloon.c | 3 ++- > exec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ > include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/balloon.c b/balloon.c > index f104b42961..c49f57c27b 100644 > --- a/balloon.c > +++ b/balloon.c > @@ -40,7 +40,8 @@ static int balloon_inhibit_count; > > bool qemu_balloon_is_inhibited(void) > { > - return atomic_read(&balloon_inhibit_count) > 0; > + return atomic_read(&balloon_inhibit_count) > 0 || > + ram_block_discard_is_broken(); > } > > void qemu_balloon_inhibit(bool state) > diff --git a/exec.c b/exec.c > index 2874bb5088..52a6e40e99 100644 > --- a/exec.c > +++ b/exec.c > @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) > } > } > > +static int ram_block_discard_broken; This could do with a comment; if I'm reading this right then +ve means broken -ve means required > +int ram_block_discard_set_broken(bool state) > +{ > + int old; > + > + if (!state) { > + atomic_dec(&ram_block_discard_broken); > + return 0; > + } > + > + do { > + old = atomic_read(&ram_block_discard_broken); > + if (old < 0) { /* Currently required */ > + return -EBUSY; > + } > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old + 1) != old); > + return 0; > +} > + > +int ram_block_discard_set_required(bool state) > +{ > + int old; > + > + if (!state) { > + atomic_inc(&ram_block_discard_broken); > + return 0; > + } > + > + do { > + old = atomic_read(&ram_block_discard_broken); > + if (old > 0) { /* Currently broken */ > + return -EBUSY; > + } > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old - 1) != old); > + return 0; > +} > + > +bool ram_block_discard_is_broken(void) > +{ > + return atomic_read(&ram_block_discard_broken) > 0; > +} > + > +bool ram_block_discard_is_required(void) > +{ > + return atomic_read(&ram_block_discard_broken) < 0; > +} > + > #endif > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e000bd2f97..9bb5ced38d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -2463,6 +2463,47 @@ static inline MemOp devend_memop(enum device_endian end) > } > #endif > > +/* > + * Inhibit technologies that rely on discarding of parts of RAM blocks to work > + * reliably, e.g., to manage the actual amount of memory consumed by the VM > + * (then, the memory provided by RAM blocks might be bigger than the desired > + * memory consumption). This *must* be set if: 'technologies that rely on discarding of parts of RAM blocks to work reliably' is pretty long; I'm not sure of a better way of saying it though. Other than the comments; Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > + * - Discarding parts of a RAM blocks does not result in the change being > + * reflected in the VM and the pages getting freed. > + * - All memory in RAM blocks is pinned or duplicated, invaldiating any previous > + * discards blindly. > + * - Discarding parts of a RAM blocks will result in integrity issues (e.g., > + * encrypted VMs). > + * Technologies that only temporarily pin the current working set of a > + * driver are fine, because we don't expect such pages to be discarded > + * (esp. based on guest action like balloon inflation). > + * > + * This is *not* to be used to protect from concurrent discards (esp., > + * postcopy). > + * > + * Returns 0 if successful. Returns -EBUSY if a technology that relies on > + * discards to work reliably is active. > + */ > +int ram_block_discard_set_broken(bool state); > + > +/* > + * Inhibit technologies that will break discarding of pages in RAM blocks. > + * > + * Returns 0 if successful. Returns -EBUSY if discards are already set to > + * broken. > + */ > +int ram_block_discard_set_required(bool state); > + > +/* > + * Test if discarding of memory in ram blocks is broken. > + */ > +bool ram_block_discard_is_broken(void); > + > +/* > + * Test if discarding of memory in ram blocks is required to work reliably. > + */ > +bool ram_block_discard_is_required(void); > + > #endif > > #endif > -- > 2.25.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 15.05.20 11:54, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> We want to replace qemu_balloon_inhibit() by something more generic. >> Especially, we want to make sure that technologies that really rely on >> RAM block discards to work reliably to run mutual exclusive with >> technologies that break it. >> >> E.g., vfio will usually pin all guest memory, turning the virtio-balloon >> basically useless and make the VM consume more memory than reported via >> the balloon. While the balloon is special already (=> no guarantees, same >> behavior possible afer reboots and with huge pages), this will be >> different, especially, with virtio-mem. >> >> Let's implement a way such that we can make both types of technology run >> mutually exclusive. We'll convert existing balloon inhibitors in successive >> patches and add some new ones. Add the check to >> qemu_balloon_is_inhibited() for now. We might want to make >> virtio-balloon an acutal inhibitor in the future - however, that >> requires more thought to not break existing setups. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> balloon.c | 3 ++- >> exec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ >> include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 91 insertions(+), 1 deletion(-) >> >> diff --git a/balloon.c b/balloon.c >> index f104b42961..c49f57c27b 100644 >> --- a/balloon.c >> +++ b/balloon.c >> @@ -40,7 +40,8 @@ static int balloon_inhibit_count; >> >> bool qemu_balloon_is_inhibited(void) >> { >> - return atomic_read(&balloon_inhibit_count) > 0; >> + return atomic_read(&balloon_inhibit_count) > 0 || >> + ram_block_discard_is_broken(); >> } >> >> void qemu_balloon_inhibit(bool state) >> diff --git a/exec.c b/exec.c >> index 2874bb5088..52a6e40e99 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) >> } >> } >> >> +static int ram_block_discard_broken; > > This could do with a comment; if I'm reading this right then > +ve means broken > -ve means required > I'll add to ram_block_discard_broken: "If positive, discarding RAM is broken. If negative, discarding of RAM is required to work correctly." [...] >> >> +/* >> + * Inhibit technologies that rely on discarding of parts of RAM blocks to work >> + * reliably, e.g., to manage the actual amount of memory consumed by the VM >> + * (then, the memory provided by RAM blocks might be bigger than the desired >> + * memory consumption). This *must* be set if: > > 'technologies that rely on discarding of parts of RAM blocks to work > reliably' is pretty long; I'm not sure of a better way of saying it > though. Maybe simply "Inhibit technologies that rely on discarding of pages in RAM blocks to work"? ?
On 06.05.20 11:49, David Hildenbrand wrote: > We want to replace qemu_balloon_inhibit() by something more generic. > Especially, we want to make sure that technologies that really rely on > RAM block discards to work reliably to run mutual exclusive with > technologies that break it. > > E.g., vfio will usually pin all guest memory, turning the virtio-balloon > basically useless and make the VM consume more memory than reported via > the balloon. While the balloon is special already (=> no guarantees, same > behavior possible afer reboots and with huge pages), this will be > different, especially, with virtio-mem. > > Let's implement a way such that we can make both types of technology run > mutually exclusive. We'll convert existing balloon inhibitors in successive > patches and add some new ones. Add the check to > qemu_balloon_is_inhibited() for now. We might want to make > virtio-balloon an acutal inhibitor in the future - however, that > requires more thought to not break existing setups. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > balloon.c | 3 ++- > exec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ > include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/balloon.c b/balloon.c > index f104b42961..c49f57c27b 100644 > --- a/balloon.c > +++ b/balloon.c > @@ -40,7 +40,8 @@ static int balloon_inhibit_count; > > bool qemu_balloon_is_inhibited(void) > { > - return atomic_read(&balloon_inhibit_count) > 0; > + return atomic_read(&balloon_inhibit_count) > 0 || > + ram_block_discard_is_broken(); > } > > void qemu_balloon_inhibit(bool state) > diff --git a/exec.c b/exec.c > index 2874bb5088..52a6e40e99 100644 > --- a/exec.c > +++ b/exec.c > @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) > } > } > > +static int ram_block_discard_broken; > + > +int ram_block_discard_set_broken(bool state) > +{ > + int old; > + > + if (!state) { > + atomic_dec(&ram_block_discard_broken); > + return 0; > + } > + > + do { > + old = atomic_read(&ram_block_discard_broken); > + if (old < 0) { > + return -EBUSY; > + } > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old + 1) != old); > + return 0; > +} > + > +int ram_block_discard_set_required(bool state) > +{ > + int old; > + > + if (!state) { > + atomic_inc(&ram_block_discard_broken); > + return 0; > + } > + > + do { > + old = atomic_read(&ram_block_discard_broken); > + if (old > 0) { > + return -EBUSY; > + } > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old - 1) != old); > + return 0; > +} > + > +bool ram_block_discard_is_broken(void) > +{ > + return atomic_read(&ram_block_discard_broken) > 0; > +} > + > +bool ram_block_discard_is_required(void) > +{ > + return atomic_read(&ram_block_discard_broken) < 0; > +} > + > #endif > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e000bd2f97..9bb5ced38d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -2463,6 +2463,47 @@ static inline MemOp devend_memop(enum device_endian end) > } > #endif > > +/* > + * Inhibit technologies that rely on discarding of parts of RAM blocks to work > + * reliably, e.g., to manage the actual amount of memory consumed by the VM > + * (then, the memory provided by RAM blocks might be bigger than the desired > + * memory consumption). This *must* be set if: > + * - Discarding parts of a RAM blocks does not result in the change being > + * reflected in the VM and the pages getting freed. > + * - All memory in RAM blocks is pinned or duplicated, invaldiating any previous > + * discards blindly. > + * - Discarding parts of a RAM blocks will result in integrity issues (e.g., > + * encrypted VMs). > + * Technologies that only temporarily pin the current working set of a > + * driver are fine, because we don't expect such pages to be discarded > + * (esp. based on guest action like balloon inflation). > + * > + * This is *not* to be used to protect from concurrent discards (esp., > + * postcopy). > + * > + * Returns 0 if successful. Returns -EBUSY if a technology that relies on > + * discards to work reliably is active. > + */ > +int ram_block_discard_set_broken(bool state); > + > +/* > + * Inhibit technologies that will break discarding of pages in RAM blocks. > + * > + * Returns 0 if successful. Returns -EBUSY if discards are already set to > + * broken. > + */ > +int ram_block_discard_set_required(bool state); > + > +/* > + * Test if discarding of memory in ram blocks is broken. > + */ > +bool ram_block_discard_is_broken(void); > + > +/* > + * Test if discarding of memory in ram blocks is required to work reliably. > + */ > +bool ram_block_discard_is_required(void); > + > #endif > > #endif > I'm wondering if I'll just call these functions ram_block_discard_disable() and ram_block_discard_require()
* David Hildenbrand (david@redhat.com) wrote: > On 06.05.20 11:49, David Hildenbrand wrote: > > We want to replace qemu_balloon_inhibit() by something more generic. > > Especially, we want to make sure that technologies that really rely on > > RAM block discards to work reliably to run mutual exclusive with > > technologies that break it. > > > > E.g., vfio will usually pin all guest memory, turning the virtio-balloon > > basically useless and make the VM consume more memory than reported via > > the balloon. While the balloon is special already (=> no guarantees, same > > behavior possible afer reboots and with huge pages), this will be > > different, especially, with virtio-mem. > > > > Let's implement a way such that we can make both types of technology run > > mutually exclusive. We'll convert existing balloon inhibitors in successive > > patches and add some new ones. Add the check to > > qemu_balloon_is_inhibited() for now. We might want to make > > virtio-balloon an acutal inhibitor in the future - however, that > > requires more thought to not break existing setups. > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Richard Henderson <rth@twiddle.net> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: David Hildenbrand <david@redhat.com> > > --- > > balloon.c | 3 ++- > > exec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ > > include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 91 insertions(+), 1 deletion(-) > > > > diff --git a/balloon.c b/balloon.c > > index f104b42961..c49f57c27b 100644 > > --- a/balloon.c > > +++ b/balloon.c > > @@ -40,7 +40,8 @@ static int balloon_inhibit_count; > > > > bool qemu_balloon_is_inhibited(void) > > { > > - return atomic_read(&balloon_inhibit_count) > 0; > > + return atomic_read(&balloon_inhibit_count) > 0 || > > + ram_block_discard_is_broken(); > > } > > > > void qemu_balloon_inhibit(bool state) > > diff --git a/exec.c b/exec.c > > index 2874bb5088..52a6e40e99 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) > > } > > } > > > > +static int ram_block_discard_broken; > > + > > +int ram_block_discard_set_broken(bool state) > > +{ > > + int old; > > + > > + if (!state) { > > + atomic_dec(&ram_block_discard_broken); > > + return 0; > > + } > > + > > + do { > > + old = atomic_read(&ram_block_discard_broken); > > + if (old < 0) { > > + return -EBUSY; > > + } > > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old + 1) != old); > > + return 0; > > +} > > + > > +int ram_block_discard_set_required(bool state) > > +{ > > + int old; > > + > > + if (!state) { > > + atomic_inc(&ram_block_discard_broken); > > + return 0; > > + } > > + > > + do { > > + old = atomic_read(&ram_block_discard_broken); > > + if (old > 0) { > > + return -EBUSY; > > + } > > + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old - 1) != old); > > + return 0; > > +} > > + > > +bool ram_block_discard_is_broken(void) > > +{ > > + return atomic_read(&ram_block_discard_broken) > 0; > > +} > > + > > +bool ram_block_discard_is_required(void) > > +{ > > + return atomic_read(&ram_block_discard_broken) < 0; > > +} > > + > > #endif > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index e000bd2f97..9bb5ced38d 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -2463,6 +2463,47 @@ static inline MemOp devend_memop(enum device_endian end) > > } > > #endif > > > > +/* > > + * Inhibit technologies that rely on discarding of parts of RAM blocks to work > > + * reliably, e.g., to manage the actual amount of memory consumed by the VM > > + * (then, the memory provided by RAM blocks might be bigger than the desired > > + * memory consumption). This *must* be set if: > > + * - Discarding parts of a RAM blocks does not result in the change being > > + * reflected in the VM and the pages getting freed. > > + * - All memory in RAM blocks is pinned or duplicated, invaldiating any previous > > + * discards blindly. > > + * - Discarding parts of a RAM blocks will result in integrity issues (e.g., > > + * encrypted VMs). > > + * Technologies that only temporarily pin the current working set of a > > + * driver are fine, because we don't expect such pages to be discarded > > + * (esp. based on guest action like balloon inflation). > > + * > > + * This is *not* to be used to protect from concurrent discards (esp., > > + * postcopy). > > + * > > + * Returns 0 if successful. Returns -EBUSY if a technology that relies on > > + * discards to work reliably is active. > > + */ > > +int ram_block_discard_set_broken(bool state); > > + > > +/* > > + * Inhibit technologies that will break discarding of pages in RAM blocks. > > + * > > + * Returns 0 if successful. Returns -EBUSY if discards are already set to > > + * broken. > > + */ > > +int ram_block_discard_set_required(bool state); > > + > > +/* > > + * Test if discarding of memory in ram blocks is broken. > > + */ > > +bool ram_block_discard_is_broken(void); > > + > > +/* > > + * Test if discarding of memory in ram blocks is required to work reliably. > > + */ > > +bool ram_block_discard_is_required(void); > > + > > #endif > > > > #endif > > > > I'm wondering if I'll just call these functions > > ram_block_discard_disable() > > and > > ram_block_discard_require() Yeh I prefer that. Dave > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/balloon.c b/balloon.c index f104b42961..c49f57c27b 100644 --- a/balloon.c +++ b/balloon.c @@ -40,7 +40,8 @@ static int balloon_inhibit_count; bool qemu_balloon_is_inhibited(void) { - return atomic_read(&balloon_inhibit_count) > 0; + return atomic_read(&balloon_inhibit_count) > 0 || + ram_block_discard_is_broken(); } void qemu_balloon_inhibit(bool state) diff --git a/exec.c b/exec.c index 2874bb5088..52a6e40e99 100644 --- a/exec.c +++ b/exec.c @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) } } +static int ram_block_discard_broken; + +int ram_block_discard_set_broken(bool state) +{ + int old; + + if (!state) { + atomic_dec(&ram_block_discard_broken); + return 0; + } + + do { + old = atomic_read(&ram_block_discard_broken); + if (old < 0) { + return -EBUSY; + } + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old + 1) != old); + return 0; +} + +int ram_block_discard_set_required(bool state) +{ + int old; + + if (!state) { + atomic_inc(&ram_block_discard_broken); + return 0; + } + + do { + old = atomic_read(&ram_block_discard_broken); + if (old > 0) { + return -EBUSY; + } + } while (atomic_cmpxchg(&ram_block_discard_broken, old, old - 1) != old); + return 0; +} + +bool ram_block_discard_is_broken(void) +{ + return atomic_read(&ram_block_discard_broken) > 0; +} + +bool ram_block_discard_is_required(void) +{ + return atomic_read(&ram_block_discard_broken) < 0; +} + #endif diff --git a/include/exec/memory.h b/include/exec/memory.h index e000bd2f97..9bb5ced38d 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2463,6 +2463,47 @@ static inline MemOp devend_memop(enum device_endian end) } #endif +/* + * Inhibit technologies that rely on discarding of parts of RAM blocks to work + * reliably, e.g., to manage the actual amount of memory consumed by the VM + * (then, the memory provided by RAM blocks might be bigger than the desired + * memory consumption). This *must* be set if: + * - Discarding parts of a RAM blocks does not result in the change being + * reflected in the VM and the pages getting freed. + * - All memory in RAM blocks is pinned or duplicated, invaldiating any previous + * discards blindly. + * - Discarding parts of a RAM blocks will result in integrity issues (e.g., + * encrypted VMs). + * Technologies that only temporarily pin the current working set of a + * driver are fine, because we don't expect such pages to be discarded + * (esp. based on guest action like balloon inflation). + * + * This is *not* to be used to protect from concurrent discards (esp., + * postcopy). + * + * Returns 0 if successful. Returns -EBUSY if a technology that relies on + * discards to work reliably is active. + */ +int ram_block_discard_set_broken(bool state); + +/* + * Inhibit technologies that will break discarding of pages in RAM blocks. + * + * Returns 0 if successful. Returns -EBUSY if discards are already set to + * broken. + */ +int ram_block_discard_set_required(bool state); + +/* + * Test if discarding of memory in ram blocks is broken. + */ +bool ram_block_discard_is_broken(void); + +/* + * Test if discarding of memory in ram blocks is required to work reliably. + */ +bool ram_block_discard_is_required(void); + #endif #endif
We want to replace qemu_balloon_inhibit() by something more generic. Especially, we want to make sure that technologies that really rely on RAM block discards to work reliably to run mutual exclusive with technologies that break it. E.g., vfio will usually pin all guest memory, turning the virtio-balloon basically useless and make the VM consume more memory than reported via the balloon. While the balloon is special already (=> no guarantees, same behavior possible afer reboots and with huge pages), this will be different, especially, with virtio-mem. Let's implement a way such that we can make both types of technology run mutually exclusive. We'll convert existing balloon inhibitors in successive patches and add some new ones. Add the check to qemu_balloon_is_inhibited() for now. We might want to make virtio-balloon an acutal inhibitor in the future - however, that requires more thought to not break existing setups. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- balloon.c | 3 ++- exec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-)