| Message ID | 56E1875E.8060007@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/03/2016 15:40, Xiao Guangrong wrote: > long dirty_count = kvm->tlbs_dirty; > > + /* > + * read tlbs_dirty before doing tlb flush to make sure not tlb > request is > + * lost. > + */ > smp_mb(); > + > if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH)) > ++kvm->stat.remote_tlb_flush; > cmpxchg(&kvm->tlbs_dirty, dirty_count, 0); > > > Any comment? Compared to smp_load_acquire(), smp_mb() adds an ordering between stores and loads. Is the The load of kvm->tlbs_dirty should then be /* * Read tlbs_dirty before setting KVM_REQ_TLB_FLUSH in * kvm_make_all_cpus_request. This */ long dirty_count = smp_load_acquire(kvm->tlbs_dirty); Tianyu, I think Xiao provided the information that I was missing. Would you like to prepare the patch? Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2016 16:26, Paolo Bonzini wrote: > Compared to smp_load_acquire(), smp_mb() adds an ordering between stores > and loads. Here, the ordering is load-store, hence... > The load of kvm->tlbs_dirty should then be > > /* > * Read tlbs_dirty before setting KVM_REQ_TLB_FLUSH in > * kvm_make_all_cpus_request. This > */ > long dirty_count = smp_load_acquire(kvm->tlbs_dirty); > > Tianyu, I think Xiao provided the information that I was missing. Would > you like to prepare the patch? Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2016 11:31 PM, Paolo Bonzini wrote: > > > On 10/03/2016 16:26, Paolo Bonzini wrote: >> Compared to smp_load_acquire(), smp_mb() adds an ordering between stores >> and loads. > > Here, the ordering is load-store, hence... Yes, this is why i put smp_mb() in the code. :) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2016 16:45, Xiao Guangrong wrote: >> >>> Compared to smp_load_acquire(), smp_mb() adds an ordering between stores >>> and loads. >> >> Here, the ordering is load-store, hence... > > Yes, this is why i put smp_mb() in the code. :) Here is a table of barriers: '. after| | before '. | load | store __________'.|___________________|________________________ | | | smp_rmb | smp_load_acquire load | smp_load_acquire | smp_store_release XX | smp_mb | smp_mb ____________|___________________|________________________ | | | | smp_wmb store | smp_mb | smp_store_release | | smp_mb | | Your case is the one marked with XX, so a smp_load_acquire() is enough---and it's preferrable, because it's cheaper than smp_mb() and more self-documenting. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11/2016 12:04 AM, Paolo Bonzini wrote: > > > On 10/03/2016 16:45, Xiao Guangrong wrote: >>> >>>> Compared to smp_load_acquire(), smp_mb() adds an ordering between stores >>>> and loads. >>> >>> Here, the ordering is load-store, hence... >> >> Yes, this is why i put smp_mb() in the code. :) > > Here is a table of barriers: > > > '. after| | > before '. | load | store > __________'.|___________________|________________________ > | | > | smp_rmb | smp_load_acquire > load | smp_load_acquire | smp_store_release XX > | smp_mb | smp_mb > ____________|___________________|________________________ > | | > | | smp_wmb > store | smp_mb | smp_store_release > | | smp_mb > | | > > Your case is the one marked with XX, so a smp_load_acquire() is > enough---and it's preferrable, because it's cheaper than smp_mb() and > more self-documenting. Yes, you are right and thank you for pointing it out. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016?03?10? 23:26, Paolo Bonzini wrote: > > > On 10/03/2016 15:40, Xiao Guangrong wrote: >> long dirty_count = kvm->tlbs_dirty; >> >> + /* >> + * read tlbs_dirty before doing tlb flush to make sure not tlb >> request is >> + * lost. >> + */ >> smp_mb(); >> + >> if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH)) >> ++kvm->stat.remote_tlb_flush; >> cmpxchg(&kvm->tlbs_dirty, dirty_count, 0); >> >> >> Any comment? > > Compared to smp_load_acquire(), smp_mb() adds an ordering between stores > and loads. Is the > > The load of kvm->tlbs_dirty should then be > > /* > * Read tlbs_dirty before setting KVM_REQ_TLB_FLUSH in > * kvm_make_all_cpus_request. This > */ > long dirty_count = smp_load_acquire(kvm->tlbs_dirty); > > Tianyu, I think Xiao provided the information that I was missing. Would > you like to prepare the patch? Paolo: Sure. I will do that. Guangrong: Thanks a lot for your input.
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 91e939b..4cad57f 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -948,6 +948,12 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) return -EINVAL; if (FNAME(prefetch_invalid_gpte)(vcpu, sp, &sp->spt[i], gpte)) { + /* + * update spte before increasing tlbs_dirty to make sure no tlb + * flush in lost after spte is zapped, see the comments in + * kvm_flush_remote_tlbs(). + */ + smp_wmb(); vcpu->kvm->tlbs_dirty++; continue; } @@ -963,6 +969,8 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) if (gfn != sp->gfns[i]) { drop_spte(vcpu->kvm, &sp->spt[i]); + /* the same as above where we are doing prefetch_invalid_gpte(). */ + smp_wmb(); vcpu->kvm->tlbs_dirty++; continue; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 314c777..82c86ea 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -193,7 +193,12 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) { long dirty_count = kvm->tlbs_dirty; + /* + * read tlbs_dirty before doing tlb flush to make sure not tlb request is + * lost. + */ smp_mb(); + if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH)) ++kvm->stat.remote_tlb_flush; cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);