On 2016年03月08日 23:27, Paolo Bonzini wrote: > Unfortunately that patch added a bad memory barrier: 1) it lacks a > comment; 2) it lacks obvious pairing; 3) it is an smp_mb() after a read, > so it's not even obvious that this memory barrier has to do with the > immediately preceding read of kvm->tlbs_dirty. It also is not > documented in Documentation/virtual/kvm/mmu.txt (Guangrong documented > there most of his other work, back in 2013, but not this one :)). > > The cmpxchg is ordered anyway against the read, because 1) x86 has > implicit ordering between earlier loads and later stores; 2) even > store-load barriers are unnecessary for accesses to the same variable > (in this case kvm->tlbs_dirty). > > So offhand, I cannot say what it orders. There are two possibilities: > > 1) it orders the read of tlbs_dirty with the read of mode. In this > case, a smp_rmb() would have been enough, and it's not clear where is > the matching smp_wmb(). > > 2) it orders the read of tlbs_dirty with the KVM_REQ_TLB_FLUSH request. > In this case a smp_load_acquire would be better. > > 3) it does the same as kvm_mmu_commit_zap_page's smp_mb() but for other > callers of kvm_flush_remote_tlbs(). In this case, we know what's the > matching memory barrier (walk_shadow_page_lockless_*). > > 4) it is completely unnecessary. > > My guess is either (2) or (3), but I am not sure. We know that > anticipating kvm->tlbs_dirty should be safe; worst case, it causes the > cmpxchg to fail and an extra TLB flush on the next call to the MMU > notifier. But I'm not sure of what happens if the processor moves the > read later. I found the smp_mb() in the kvm_mmu_commit_zap_page() was removed by commit 5befdc38 and the commit was reverted by commit a086f6a1e. The remove/revert reason is whether kvm_flush_remote_tlbs() is under mmu_lock or not. The mode and request aren't always under mmu_lock and so I think the smp_mb() should not be related with mode and request when introduced. > >> > The smp_mb() in the kvm_mmu_commit_zap_page() was introduced by commit >> > c142786c6 which was merged later than commit a4ee1ca4. It pairs with >> > smp_mb() in the walk_shadow_page_lockless_begin/end() to keep order >> > between modifications of the page tables and reading mode. > Yes; it also pairs with the smp_mb__after_srcu_unlock() in vcpu_enter_guest. > >> > The smp_mb() in the kvm_make_all_cpus_request() was introduced by commit >> > 6b7e2d09. It keeps order between setting request bit and reading mode. > Yes. > >>> >> So you can: >>> >> >>> >> 1) add a comment to kvm_flush_remote_tlbs like: >>> >> >>> >> /* >>> >> * 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. >>> >> * - powerpc: smp_mb in kvmppc_prepare_to_enter. >>> >> */ >>> >> >>> >> 2) add a comment to vcpu_enter_guest and kvmppc_prepare_to_enter, saying >>> >> that the memory barrier also orders the write to mode from any reads >>> >> to the page tables done while the VCPU is running. In other words, on >>> >> entry a single memory barrier achieves two purposes (write ->mode before >>> >> reading requests, write ->mode before reading page tables). >> > >> > These sounds good. >> > >>> >> The same should be true in kvm_flush_remote_tlbs(). So you may investigate >>> >> removing the barrier from kvm_flush_remote_tlbs, because >>> >> kvm_make_all_cpus_request already includes a memory barrier. Like >>> >> Thomas suggested, leave a comment in kvm_flush_remote_tlbs(), >>> >> saying which memory barrier you are relying on and for what. >> > >> > If we remove the smp_mb() in the kvm_flush_remote_tlbs(), we need to >> > leave comments both in the kvm_flush_remote_tlbs() and >> > kvm_mmu_commit_zap_page(), right? > Yes. In fact, instead of removing it, I would change it to > > smp_mb__before_atomic(); > > with a comment that points to the addition of the barrier in commit > a4ee1ca4. Unless Guangrong can enlighten us. :) > 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. 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); 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. * - powerpc: smp_mb in kvmppc_prepare_to_enter. */ smp_mb__before_atomic(); >>> >> >>> >> And finally, the memory barrier in kvm_make_all_cpus_request can become >>> >> smp_mb__after_atomic, which is free on x86. >> > >> > I found you have done this in your tree :) >> > >> > commit 5b06b60b72b73cbe3805d2a64d223e0264bd0479 >> > Author: Paolo Bonzini <pbonzini@xxxxxxxxxx> >> > Date: Wed Feb 24 12:44:17 2016 +0100 >> > >> > KVM: mark memory barrier with smp_mb__after_atomic >> > >> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > Yeah, but I reverted it because it makes sense to do it together with > your patch. > > Paolo -- Best regards Tianyu Lan -- 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
