On 06/25/2018 11:02 PM, Paolo Bonzini wrote:
> On 26/06/2018 01:23, Junaid Shahid wrote:
>>>
>>> 2) /*
>>> * Read sp->unsync before the spinlock. sp->unsync is written in
>>> * kvm_mmu->page_fault with mmu_lock held; this barrier pairs with
>>> * spin_lock's acquire barrier, which guarantees that the spinlock
>>> * is written before sp->unsync.
>>> */
>>> smp_rmb();
>>>
>>> Feel free to make it less verbose. :)
>>>
>> Hmm. I am not really convinced of the necessity of an smp_rmb()
>> between the sp->unsync test and the spin_lock (I assume that is where
>> you mean to place the smp_rmb, correct?), though a READ_ONCE of
>> sp->unsync may be warranted. To illustrate, consider that even if the
>> read of sp->unsync were to happen after the spin_lock, would it
>> really be any different than what used to happen before this change?
>
> I don't know, the loop in spin_lock probably saves it, but it's
> nevertheless the tricky control-dependency case. Since it's basically
> zero cost, I would feel safer adding a smp_rmb (or smp_load_acquire).
>
As for the control dependency, Documentation/memory-barriers.txt says that load-store control dependencies are already ordered even without a barrier e.g.
q = READ_ONCE(a);
if (q) {
WRITE_ONCE(b, 1);
}
which roughly maps in this case to:
q = READ_ONCE(sp->unsync) || READ_ONCE(sp->unsync_children);
if (q) {
spin_lock();
...
}
Since spin_lock() involves a store via the atomic test-and-set, so I would assume that it falls under the load-store control dependency case.
I do agree that there is basically no run-time cost to add a barrier here, but my concern is more about added confusion for someone modifying this code later. Nevertheless, if you think that we need to add it to be extra safe, then I guess I can do the loads via smp_load_acquire().
Thanks,
Junaid
