| Message ID | 20230509162740.58081-1-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] s390x/pv: Fix spurious warning with asynchronous teardown | expand |
On 09/05/2023 18.27, Claudio Imbrenda wrote: > When rebooting a small VM using asynchronous teardown, a spurious > warning is emitted when the KVM_PV_ASYNC_CLEANUP_PREPARE ioctl fails. Why does the _PREPARE fail in that case? Why 4GiB and not more or less? This sounds racy... what if you have a faster or slower machine? > Avoid using asynchronous teardown altogether when the VM is small > enough; the cutoff is set at 4GiB. This will avoid triggering the > warning and also avoid pointless overhead; normal teardown is fast > enough for small VMs. > > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > Fixes: c3a073c610 ("s390x/pv: Add support for asynchronous teardown for reboot") > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > hw/s390x/pv.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c > index 49ea38236c..17c5556319 100644 > --- a/hw/s390x/pv.c > +++ b/hw/s390x/pv.c > @@ -13,6 +13,7 @@ > > #include <linux/kvm.h> > > +#include "qemu/units.h" > #include "qapi/error.h" > #include "qemu/error-report.h" > #include "sysemu/kvm.h" > @@ -117,13 +118,16 @@ static void *s390_pv_do_unprot_async_fn(void *p) > > bool s390_pv_vm_try_disable_async(void) > { > + MachineState *machine = MACHINE(qdev_get_machine()); The calling function (s390_machine_unprotect()) already has a S390CcwMachineState as parameter ... so you could pass along that value to avoid the qdev_get_machine() here. > /* > * t is only needed to create the thread; once qemu_thread_create > * returns, it can safely be discarded. > */ > QemuThread t; > > - if (!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { > + /* Avoid the overhead of asynchronous teardown for small machines */ > + if ((machine->maxram_size < 4 * GiB) || > + !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { > return false; > } > if (s390_pv_cmd(KVM_PV_ASYNC_CLEANUP_PREPARE, NULL) != 0) { Thomas
On Wed, 10 May 2023 08:47:08 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 09/05/2023 18.27, Claudio Imbrenda wrote: > > When rebooting a small VM using asynchronous teardown, a spurious > > warning is emitted when the KVM_PV_ASYNC_CLEANUP_PREPARE ioctl fails. > > Why does the _PREPARE fail in that case? Why 4GiB and not more or less? This because of kernel commit 292a7d6fca33df70ca4b8e9b0d0e74adf87582dc, which fixes problems in case the VM is small (<2GiB) > sounds racy... what if you have a faster or slower machine? why racy? 2 or 4GiB is still very fast, and at some point you have to draw a line. I could make it 2GiB, which is the limit at which _PREPARE will fail, but since I'm touching this code, I would like to avoid unnecessary overhead, instead of "just fixing" I can put the limit to 2GiB if you think it's more clean > > > Avoid using asynchronous teardown altogether when the VM is small > > enough; the cutoff is set at 4GiB. This will avoid triggering the > > warning and also avoid pointless overhead; normal teardown is fast > > enough for small VMs. > > > > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > Fixes: c3a073c610 ("s390x/pv: Add support for asynchronous teardown for reboot") > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > hw/s390x/pv.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c > > index 49ea38236c..17c5556319 100644 > > --- a/hw/s390x/pv.c > > +++ b/hw/s390x/pv.c > > @@ -13,6 +13,7 @@ > > > > #include <linux/kvm.h> > > > > +#include "qemu/units.h" > > #include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "sysemu/kvm.h" > > @@ -117,13 +118,16 @@ static void *s390_pv_do_unprot_async_fn(void *p) > > > > bool s390_pv_vm_try_disable_async(void) > > { > > + MachineState *machine = MACHINE(qdev_get_machine()); > > The calling function (s390_machine_unprotect()) already has a > S390CcwMachineState as parameter ... so you could pass along that value to > avoid the qdev_get_machine() here. yes, I was thinking about that and decided against it to avoid changing interfaces; I'll fix it in the next version > > > /* > > * t is only needed to create the thread; once qemu_thread_create > > * returns, it can safely be discarded. > > */ > > QemuThread t; > > > > - if (!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { > > + /* Avoid the overhead of asynchronous teardown for small machines */ > > + if ((machine->maxram_size < 4 * GiB) || > > + !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { > > return false; > > } > > if (s390_pv_cmd(KVM_PV_ASYNC_CLEANUP_PREPARE, NULL) != 0) { > > Thomas >
On 10/05/2023 09.32, Claudio Imbrenda wrote: > On Wed, 10 May 2023 08:47:08 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 09/05/2023 18.27, Claudio Imbrenda wrote: >>> When rebooting a small VM using asynchronous teardown, a spurious >>> warning is emitted when the KVM_PV_ASYNC_CLEANUP_PREPARE ioctl fails. >> >> Why does the _PREPARE fail in that case? Why 4GiB and not more or less? This > > because of kernel commit 292a7d6fca33df70ca4b8e9b0d0e74adf87582dc, which > fixes problems in case the VM is small (<2GiB) > >> sounds racy... what if you have a faster or slower machine? > > why racy? > > 2 or 4GiB is still very fast, and at some point you have to draw a line. > I could make it 2GiB, which is the limit at which _PREPARE will fail, > but since I'm touching this code, I would like to avoid unnecessary > overhead, instead of "just fixing" > > I can put the limit to 2GiB if you think it's more clean Ok, so this is not due to some race (which I first suspected), but due to some change in recent kernels. Please put a link to the related kernel commit in the patch description, and yes, I'd prefer to use the same boundary as the kernel here (i.e. 2GiB instead of 4GiB), just that it is clear that we're not using an arbitrary magic value here. Thanks, Thomas
diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c index 49ea38236c..17c5556319 100644 --- a/hw/s390x/pv.c +++ b/hw/s390x/pv.c @@ -13,6 +13,7 @@ #include <linux/kvm.h> +#include "qemu/units.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "sysemu/kvm.h" @@ -117,13 +118,16 @@ static void *s390_pv_do_unprot_async_fn(void *p) bool s390_pv_vm_try_disable_async(void) { + MachineState *machine = MACHINE(qdev_get_machine()); /* * t is only needed to create the thread; once qemu_thread_create * returns, it can safely be discarded. */ QemuThread t; - if (!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { + /* Avoid the overhead of asynchronous teardown for small machines */ + if ((machine->maxram_size < 4 * GiB) || + !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) { return false; } if (s390_pv_cmd(KVM_PV_ASYNC_CLEANUP_PREPARE, NULL) != 0) {
When rebooting a small VM using asynchronous teardown, a spurious warning is emitted when the KVM_PV_ASYNC_CLEANUP_PREPARE ioctl fails. Avoid using asynchronous teardown altogether when the VM is small enough; the cutoff is set at 4GiB. This will avoid triggering the warning and also avoid pointless overhead; normal teardown is fast enough for small VMs. Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> Fixes: c3a073c610 ("s390x/pv: Add support for asynchronous teardown for reboot") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- hw/s390x/pv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)