[RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Collin Walling]

DIAGNOSE 0318 is invoked only once during IPL. As such, the
diag318 info will only change once initially and during resets.
Let's only sync the register to convey the info to KVM if and
only if the diag318 info has changed. Only set the dirty bit
flag for diag318 whenever it must be updated.

The migration handler will invoke the set_diag318 helper on
post_load to ensure the info is set on the destination machine.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 target/s390x/kvm/kvm.c |  5 -----
 target/s390x/machine.c | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 6acf14d5ec..6a141399f7 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
     }
 
-    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
-        cs->kvm_run->s.regs.diag318 = env->diag318_info;
-        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
-    }
-
     /* Finally the prefix */
     if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
         cs->kvm_run->s.regs.prefix = env->psa;
diff --git a/target/s390x/machine.c b/target/s390x/machine.c
index 37a076858c..a5d113ce3a 100644
--- a/target/s390x/machine.c
+++ b/target/s390x/machine.c
@@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
     }
 };
 
+static int diag318_post_load(void *opaque, int version_id)
+{
+    S390CPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPUS390XState *env = &S390_CPU(cs)->env;
+
+    if (kvm_enabled()) {
+        kvm_s390_set_diag318(cs, env->diag318_info);
+    }
+
+    return 0;
+}
+
 static bool diag318_needed(void *opaque)
 {
     return s390_has_feat(S390_FEAT_DIAG_318);
@@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
     .name = "cpu/diag318",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = diag318_post_load,
     .needed = diag318_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(env.diag318_info, S390CPU),
-- 
2.31.1

Re: [RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Collin Walling]

On 11/22/21 17:33, Collin Walling wrote:
> DIAGNOSE 0318 is invoked only once during IPL. As such, the
> diag318 info will only change once initially and during resets.
> Let's only sync the register to convey the info to KVM if and
> only if the diag318 info has changed. Only set the dirty bit
> flag for diag318 whenever it must be updated.
> 
> The migration handler will invoke the set_diag318 helper on
> post_load to ensure the info is set on the destination machine.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

This is a long-overdue response to this thread:
https://www.spinics.net/lists/kvm/msg258071.html

> ---
>  target/s390x/kvm/kvm.c |  5 -----
>  target/s390x/machine.c | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 6acf14d5ec..6a141399f7 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>      }
>  
> -    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
> -        cs->kvm_run->s.regs.diag318 = env->diag318_info;
> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> -    }
> -
>      /* Finally the prefix */
>      if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>          cs->kvm_run->s.regs.prefix = env->psa;
> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
> index 37a076858c..a5d113ce3a 100644
> --- a/target/s390x/machine.c
> +++ b/target/s390x/machine.c
> @@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
>      }
>  };
>  
> +static int diag318_post_load(void *opaque, int version_id)
> +{
> +    S390CPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    CPUS390XState *env = &S390_CPU(cs)->env;
> +
> +    if (kvm_enabled()) {
> +        kvm_s390_set_diag318(cs, env->diag318_info);
> +    }
> +
> +    return 0;
> +}
> +
>  static bool diag318_needed(void *opaque)
>  {
>      return s390_has_feat(S390_FEAT_DIAG_318);
> @@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
>      .name = "cpu/diag318",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = diag318_post_load,
>      .needed = diag318_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64(env.diag318_info, S390CPU),
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Christian Borntraeger]

Am 22.11.21 um 23:33 schrieb Collin Walling:
> DIAGNOSE 0318 is invoked only once during IPL. As such, the
> diag318 info will only change once initially and during resets.
> Let's only sync the register to convey the info to KVM if and
> only if the diag318 info has changed. Only set the dirty bit
> flag for diag318 whenever it must be updated.

Is this really necessary? In my logs the sync only happens for larger
changes (like reset) operations and then yes we log again.
But I think it is ok to see such a log entry in these rare events.
> 
> The migration handler will invoke the set_diag318 helper on
> post_load to ensure the info is set on the destination machine.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>   target/s390x/kvm/kvm.c |  5 -----
>   target/s390x/machine.c | 14 ++++++++++++++
>   2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 6acf14d5ec..6a141399f7 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>       }
>   
> -    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
> -        cs->kvm_run->s.regs.diag318 = env->diag318_info;
> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> -    }
> -
>       /* Finally the prefix */
>       if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>           cs->kvm_run->s.regs.prefix = env->psa;
> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
> index 37a076858c..a5d113ce3a 100644
> --- a/target/s390x/machine.c
> +++ b/target/s390x/machine.c
> @@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
>       }
>   };
>   
> +static int diag318_post_load(void *opaque, int version_id)
> +{
> +    S390CPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    CPUS390XState *env = &S390_CPU(cs)->env;
> +
> +    if (kvm_enabled()) {
> +        kvm_s390_set_diag318(cs, env->diag318_info);
> +    }
> +
> +    return 0;
> +}
> +
>   static bool diag318_needed(void *opaque)
>   {
>       return s390_has_feat(S390_FEAT_DIAG_318);
> @@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
>       .name = "cpu/diag318",
>       .version_id = 1,
>       .minimum_version_id = 1,
> +    .post_load = diag318_post_load,
>       .needed = diag318_needed,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT64(env.diag318_info, S390CPU),
> 

Re: [RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Collin Walling]

On 11/23/21 01:14, Christian Borntraeger wrote:
> 
> Am 22.11.21 um 23:33 schrieb Collin Walling:
>> DIAGNOSE 0318 is invoked only once during IPL. As such, the
>> diag318 info will only change once initially and during resets.
>> Let's only sync the register to convey the info to KVM if and
>> only if the diag318 info has changed. Only set the dirty bit
>> flag for diag318 whenever it must be updated.
> 
> Is this really necessary? In my logs the sync only happens for larger
> changes (like reset) operations and then yes we log again.
> But I think it is ok to see such a log entry in these rare events.

Albeit a micro-optimization, I don't see why the diag318 dirtied bit
must to be set during /every/ sync. It makes more sense to set the flag
after the register was actually modified.

>>
>> The migration handler will invoke the set_diag318 helper on
>> post_load to ensure the info is set on the destination machine.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   target/s390x/kvm/kvm.c |  5 -----
>>   target/s390x/machine.c | 14 ++++++++++++++
>>   2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 6acf14d5ec..6a141399f7 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>>       }
>>   -    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> -        cs->kvm_run->s.regs.diag318 = env->diag318_info;
>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> -    }
>> -
>>       /* Finally the prefix */
>>       if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>>           cs->kvm_run->s.regs.prefix = env->psa;
>> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
>> index 37a076858c..a5d113ce3a 100644
>> --- a/target/s390x/machine.c
>> +++ b/target/s390x/machine.c
>> @@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
>>       }
>>   };
>>   +static int diag318_post_load(void *opaque, int version_id)
>> +{
>> +    S390CPU *cpu = opaque;
>> +    CPUState *cs = CPU(cpu);
>> +    CPUS390XState *env = &S390_CPU(cs)->env;
>> +
>> +    if (kvm_enabled()) {
>> +        kvm_s390_set_diag318(cs, env->diag318_info);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static bool diag318_needed(void *opaque)
>>   {
>>       return s390_has_feat(S390_FEAT_DIAG_318);
>> @@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
>>       .name = "cpu/diag318",
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>> +    .post_load = diag318_post_load,
>>       .needed = diag318_needed,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINT64(env.diag318_info, S390CPU),
>>
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Christian Borntraeger]

Am 02.12.21 um 21:54 schrieb Collin Walling:
> On 11/23/21 01:14, Christian Borntraeger wrote:
>>
>> Am 22.11.21 um 23:33 schrieb Collin Walling:
>>> DIAGNOSE 0318 is invoked only once during IPL. As such, the
>>> diag318 info will only change once initially and during resets.
>>> Let's only sync the register to convey the info to KVM if and
>>> only if the diag318 info has changed. Only set the dirty bit
>>> flag for diag318 whenever it must be updated.
>>
>> Is this really necessary? In my logs the sync only happens for larger
>> changes (like reset) operations and then yes we log again.
>> But I think it is ok to see such a log entry in these rare events.
> 
> Albeit a micro-optimization, I don't see why the diag318 dirtied bit
> must to be set during /every/ sync. It makes more sense to set the flag
> after the register was actually modified.

My point is that it is NOT set during /every/ sync.
We only set it when level != KVM_PUT_RUNTIME_STATE (see kvm_arch_put_registers)
And this basically never happens during normal runtime, only for major events
like reset.

> 
>>>
>>> The migration handler will invoke the set_diag318 helper on
>>> post_load to ensure the info is set on the destination machine.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>    target/s390x/kvm/kvm.c |  5 -----
>>>    target/s390x/machine.c | 14 ++++++++++++++
>>>    2 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>> index 6acf14d5ec..6a141399f7 100644
>>> --- a/target/s390x/kvm/kvm.c
>>> +++ b/target/s390x/kvm/kvm.c
>>> @@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>>            cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>>>        }
>>>    -    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>>> -        cs->kvm_run->s.regs.diag318 = env->diag318_info;
>>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>>> -    }
>>> -
>>>        /* Finally the prefix */
>>>        if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>>>            cs->kvm_run->s.regs.prefix = env->psa;
>>> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
>>> index 37a076858c..a5d113ce3a 100644
>>> --- a/target/s390x/machine.c
>>> +++ b/target/s390x/machine.c
>>> @@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
>>>        }
>>>    };
>>>    +static int diag318_post_load(void *opaque, int version_id)
>>> +{
>>> +    S390CPU *cpu = opaque;
>>> +    CPUState *cs = CPU(cpu);
>>> +    CPUS390XState *env = &S390_CPU(cs)->env;
>>> +
>>> +    if (kvm_enabled()) {
>>> +        kvm_s390_set_diag318(cs, env->diag318_info);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    static bool diag318_needed(void *opaque)
>>>    {
>>>        return s390_has_feat(S390_FEAT_DIAG_318);
>>> @@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
>>>        .name = "cpu/diag318",
>>>        .version_id = 1,
>>>        .minimum_version_id = 1,
>>> +    .post_load = diag318_post_load,
>>>        .needed = diag318_needed,
>>>        .fields = (VMStateField[]) {
>>>            VMSTATE_UINT64(env.diag318_info, S390CPU),
>>>
>>
> 
>