On 08/03/2016 09:36, Lan Tianyu wrote:
Summary about smp_mb()s we met in this thread. If misunderstood, please
correct me. Thanks.
The smp_mb() in the kvm_flush_remote_tlbs() was introduced by the commit
a4ee1ca4 and it seems to keep the order of reading and cmpxchg
Quote from Avi:
| I don't think we need to flush immediately; set a "tlb dirty" bit
| that is cleareded when we flush the tlb.
| can consult the bit and force a flush if set.
Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxxxxx>
Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
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.