[PATCH v1 1/1] s390x/pv: Fix spurious warning with asynchronous teardown

[PATCH v1 1/1] s390x/pv: Fix spurious warning with asynchronous teardown

[Claudio Imbrenda]

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

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) {
-- 
2.40.1

Re: [PATCH v1 1/1] s390x/pv: Fix spurious warning with asynchronous teardown

[Thomas Huth]

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

Re: [PATCH v1 1/1] s390x/pv: Fix spurious warning with asynchronous teardown

[Claudio Imbrenda]

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
> 

Re: [PATCH v1 1/1] s390x/pv: Fix spurious warning with asynchronous teardown

[Thomas Huth]

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