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 - 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. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
