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