[Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Gavin Shan]

Hi Folks,

We're reviewing upstream commits and found that it seems that
++kvm->stat.generic.remote_tlb_flush has been missed from
kvm_flush_remote_tlbs(). If I'm correct, we still need to
increase the statistic in kvm_flush_remote_tlbs()?

History about the changes:

ce6a7007048b staging: r8188eu: remove {read,write}_macreg
     The changes were NOT there any more.
419025b3b419 Merge branch kvm-arm64/misc-5.15 into kvmarm-master/next
     The changes were still there
38f703663d4c KVM: arm64: Count VMID-wide TLB invalidations
     The changes were initially introduced by this commit,
     to increase 'kvm->stat.generic.remote_tlb_flush' in
     kvm_flush_remote_tlbs().

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Marc Zyngier]

On Fri, 17 Jun 2022 11:02:44 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Hi Folks,
> 
> We're reviewing upstream commits and found that it seems that
> ++kvm->stat.generic.remote_tlb_flush has been missed from
> kvm_flush_remote_tlbs(). If I'm correct, we still need to
> increase the statistic in kvm_flush_remote_tlbs()?
> 
> History about the changes:
> 
> ce6a7007048b staging: r8188eu: remove {read,write}_macreg
>     The changes were NOT there any more.
> 419025b3b419 Merge branch kvm-arm64/misc-5.15 into kvmarm-master/next
>     The changes were still there
> 38f703663d4c KVM: arm64: Count VMID-wide TLB invalidations
>     The changes were initially introduced by this commit,
>     to increase 'kvm->stat.generic.remote_tlb_flush' in
>     kvm_flush_remote_tlbs().

I'm not sure what you are asking. This change is definitely still
present in the upstream kernel, and I don't get your point with the
staging commit, which is totally unrelated.

$ git describe --contains ce6a7007048b --match=v\*
v5.15-rc1~154^2~11
$ git describe --contains 419025b3b419 --match=v\*
v5.15-rc1~65^2~4^2
$ git describe --contains 38f703663d4c --match=v\*
v5.15-rc1~65^2~4^2^2~13

As you can see, the commit fixing the statistics was merged after
staging one (it appears closer to -rc1, as there is 6 days between the
two merge commits from Linus).

Puzzled,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Andrew Jones]

On Fri, Jun 17, 2022 at 11:19:28AM +0100, Marc Zyngier wrote:
> On Fri, 17 Jun 2022 11:02:44 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
> > 
> > Hi Folks,
> > 
> > We're reviewing upstream commits and found that it seems that
> > ++kvm->stat.generic.remote_tlb_flush has been missed from
> > kvm_flush_remote_tlbs(). If I'm correct, we still need to
> > increase the statistic in kvm_flush_remote_tlbs()?
> > 
> > History about the changes:
> > 
> > ce6a7007048b staging: r8188eu: remove {read,write}_macreg
> >     The changes were NOT there any more.
> > 419025b3b419 Merge branch kvm-arm64/misc-5.15 into kvmarm-master/next
> >     The changes were still there
> > 38f703663d4c KVM: arm64: Count VMID-wide TLB invalidations
> >     The changes were initially introduced by this commit,
> >     to increase 'kvm->stat.generic.remote_tlb_flush' in
> >     kvm_flush_remote_tlbs().
> 
> I'm not sure what you are asking. This change is definitely still
> present in the upstream kernel, and I don't get your point with the
> staging commit, which is totally unrelated.
> 
> $ git describe --contains ce6a7007048b --match=v\*
> v5.15-rc1~154^2~11
> $ git describe --contains 419025b3b419 --match=v\*
> v5.15-rc1~65^2~4^2
> $ git describe --contains 38f703663d4c --match=v\*
> v5.15-rc1~65^2~4^2^2~13
> 
> As you can see, the commit fixing the statistics was merged after
> staging one (it appears closer to -rc1, as there is 6 days between the
> two merge commits from Linus).
>

Hi Marc,

I don't see the change for commit 38f703663d4c as of an upstream pull
right now

$ git show 47700948a4ab:arch/arm64/kvm/mmu.c | grep -A4 'void kvm_flush_remote_tlbs'
void kvm_flush_remote_tlbs(struct kvm *kvm)
{
	++kvm->stat.generic.remote_tlb_flush_requests;
	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
}

and I do see it got dropped with merge commit e99314a340d2.

$ git diff 419025b3b419 0d0a19395baa -- arch/arm64/kvm/mmu.c | grep -A5 'void kvm_flush_remote_tlbs'
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
+	++kvm->stat.generic.remote_tlb_flush_requests;
 	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
-	++kvm->stat.generic.remote_tlb_flush;
 }

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Paolo Bonzini]

On 6/17/22 12:33, Andrew Jones wrote:
> I don't see the change for commit 38f703663d4c as of an upstream pull
> right now
> 
> $ git show 47700948a4ab:arch/arm64/kvm/mmu.c | grep -A4 'void kvm_flush_remote_tlbs'
> void kvm_flush_remote_tlbs(struct kvm *kvm)
> {
> 	++kvm->stat.generic.remote_tlb_flush_requests;
> 	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
> }
> 
> and I do see it got dropped with merge commit e99314a340d2.
> 
> $ git diff 419025b3b419 0d0a19395baa -- arch/arm64/kvm/mmu.c | grep -A5 'void kvm_flush_remote_tlbs'
>   void kvm_flush_remote_tlbs(struct kvm *kvm)
>   {
> +	++kvm->stat.generic.remote_tlb_flush_requests;
>   	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
> -	++kvm->stat.generic.remote_tlb_flush;
>   }

Hi,

on ARM it makes little sense to split remote_tlb_flush_requests and 
remote_tlb_flush.  On x86 the latter means "a vmexit was forced in order 
to flush the TLB", and in fact this common code:

         if (!kvm_arch_flush_remote_tlb(kvm)
             || kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
                 ++kvm->stat.generic.remote_tlb_flush;

should probably be written

         if (!kvm_arch_flush_remote_tlb(kvm))
		return;

	if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
		++kvm->stat.generic.remote_tlb_flush;

Paolo

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?

[Marc Zyngier]

On Fri, 17 Jun 2022 11:33:16 +0100,
Andrew Jones <drjones@redhat.com> wrote:
> 
> On Fri, Jun 17, 2022 at 11:19:28AM +0100, Marc Zyngier wrote:
> > On Fri, 17 Jun 2022 11:02:44 +0100,
> > Gavin Shan <gshan@redhat.com> wrote:
> > > 
> > > Hi Folks,
> > > 
> > > We're reviewing upstream commits and found that it seems that
> > > ++kvm->stat.generic.remote_tlb_flush has been missed from
> > > kvm_flush_remote_tlbs(). If I'm correct, we still need to
> > > increase the statistic in kvm_flush_remote_tlbs()?
> > > 
> > > History about the changes:
> > > 
> > > ce6a7007048b staging: r8188eu: remove {read,write}_macreg
> > >     The changes were NOT there any more.
> > > 419025b3b419 Merge branch kvm-arm64/misc-5.15 into kvmarm-master/next
> > >     The changes were still there
> > > 38f703663d4c KVM: arm64: Count VMID-wide TLB invalidations
> > >     The changes were initially introduced by this commit,
> > >     to increase 'kvm->stat.generic.remote_tlb_flush' in
> > >     kvm_flush_remote_tlbs().
> > 
> > I'm not sure what you are asking. This change is definitely still
> > present in the upstream kernel, and I don't get your point with the
> > staging commit, which is totally unrelated.
> > 
> > $ git describe --contains ce6a7007048b --match=v\*
> > v5.15-rc1~154^2~11
> > $ git describe --contains 419025b3b419 --match=v\*
> > v5.15-rc1~65^2~4^2
> > $ git describe --contains 38f703663d4c --match=v\*
> > v5.15-rc1~65^2~4^2^2~13
> > 
> > As you can see, the commit fixing the statistics was merged after
> > staging one (it appears closer to -rc1, as there is 6 days between the
> > two merge commits from Linus).
> >
> 
> Hi Marc,
> 
> I don't see the change for commit 38f703663d4c as of an upstream pull
> right now
>
> $ git show 47700948a4ab:arch/arm64/kvm/mmu.c | grep -A4 'void kvm_flush_remote_tlbs'
> void kvm_flush_remote_tlbs(struct kvm *kvm)
> {
> 	++kvm->stat.generic.remote_tlb_flush_requests;
> 	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
> }
> 
> and I do see it got dropped with merge commit e99314a340d2.
> 
> $ git diff 419025b3b419 0d0a19395baa -- arch/arm64/kvm/mmu.c | grep -A5 'void kvm_flush_remote_tlbs'
>  void kvm_flush_remote_tlbs(struct kvm *kvm)
>  {
> +	++kvm->stat.generic.remote_tlb_flush_requests;
>  	kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu);
> -	++kvm->stat.generic.remote_tlb_flush;
>  }

That's because these two counters should have the exact same value at
all times. We don't use IPIs for TLB invalidation as the HW does the
broadcast for us, so there is no point in distinguishing requests from
actual flushes.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm