Messages in this thread | | | Subject | Re: [PATCH] KVM: Remove redundant smp_mb() in the kvm_mmu_commit_zap_page() | From | Xiao Guangrong <> | Date | Fri, 11 Mar 2016 02:08:05 +0800 |
| |
On 03/11/2016 01:07 AM, Paolo Bonzini wrote: > On 09/03/2016 08:18, Lan Tianyu wrote: >> How about the following comments. >> >> Log for kvm_mmu_commit_zap_page() >> /* >> * We need to make sure everyone sees our modifications to >> * the page tables and see changes to vcpu->mode here. > > Please mention that this pairs with vcpu_enter_guest and > walk_shadow_page_lockless_begin/end. > >> The >> * barrier in the kvm_flush_remote_tlbs() helps us to achieve >> * these. Otherwise, wait for all vcpus to exit guest mode >> * and/or lockless shadow page table walks. >> */ >> kvm_flush_remote_tlbs(kvm); > > The rest of the comment is okay, but please replace "Otherwise" with "In > addition, we need to". > >> Log for kvm_flush_remote_tlbs() >> /* >> * We want to publish modifications to the page tables before >> * reading mode. Pairs with a memory barrier in arch-specific >> * code. >> * - x86: smp_mb__after_srcu_read_unlock in vcpu_enter_guest. > > ... and smp_mb in walk_shadow_page_lockless_begin/end. > >> * - powerpc: smp_mb in kvmppc_prepare_to_enter. >> */ >> smp_mb__before_atomic(); > > The comment looks good, but the smp_mb__before_atomic() is not needed. > As mentioned in the reply to Guangrong, only a smp_load_acquire is required. > > So the comment should say something like "There is already an smp_mb() > before kvm_make_all_cpus_request reads vcpu->mode. We reuse that > barrier here.". > > On top of this there is: > > - the change to paging_tmpl.h that Guangrong posted, adding smp_wmb() > before each increment of vcpu->kvm->tlbs_dirty
Yes, please make it as a separated patch.
> > - the change to smp_mb__after_atomic() in kvm_make_all_cpus_request > > - if you want :) you can also replace the store+mb in > walk_shadow_page_lockless_begin with smp_store_mb, and the mb+store in > walk_shadow_page_lockless_end with smp_store_release.
These changes are good to me.
TianYu, please CC me when you post the new version out.
| |