Il 07/05/2014 03:07, Ming Lei ha scritto:
Hi Paolo,
On Tue, May 6, 2014 at 9:17 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
Il 06/05/2014 11:26, Ming Lei ha scritto:
Hi Paolo and All,
One question is about ACCESS_ONCE() in virtscsi_pick_vq(),
looks it needn't since both reading and writing tgt->req_vq holds
tgt->tgt_lock.
You're right. It should be possible to avoid the lock in virtscsi_pick_vq
like this:
value = atomic_read(&tgt->reqs);
I am wondering if atomic_read() is OK because atomic_read()
should be treated as a simple C statement, and it may not reflect
the latest value of tgt->reqs.
It would be caught by cmpxchg below.
retry:
if (value != 0) {
old_value = atomic_cmpxchg(&tgt->regs, value, value + 1)
if (old_value != value) {
Maybe ' if (old_value != value && !old_value) ' is a bit better.
No, because if you have failed the cmpxchg you haven't incremented
tgt->reqs.
value = old_value;
goto retry;
}
vq = ACCESS_ONCE(tgt->req_vq);
} else {
spin_lock_irqsave(&tgt->tgt_lock, flags);
// tgt->reqs may not be 0 anymore, need to recheck
value = atomic_read(&tgt->reqs);
if (atomic_read(&tgt->reqs) != 0) {
spin_unlock_irqrestore(&tgt->tgt_lock, flags);
goto retry;
}
Same with above, if atomic_read() still returns zero even
after it is increased in read path from another CPU, then
an obsolete vq pointer might be seen in the read path.
If I understand you correctly, then the CPUs wouldn't be cache-coherent.
You would have bigger problems.
// tgt->reqs now will remain fixed to 0.
...
tgt->req_vq = vq = ...;
smp_wmb();
atomic_set(&tgt->reqs, 1);
spin_unlock_irqrestore(&tgt->tgt_lock, flags);
}
return vq;
and then you would need the ACCESS_ONCE but I'm not sure it's worthwhile.
Yes, I agree, :-)
Another one is about the comment in virtscsi_req_done(), which
said smp_read_barrier_depends() is needed for avoiding
out of order between reading req_vq and decreasing tgt->reqs.
But if I understand correctly, in virtscsi_req_done(), req_vq is
read from vscsi->req_vqs[vq->index - VIRTIO_SCSI_VQ_BASE],
instead of tgt->req_vq, and the former won't change wrt.
inc/dec tgt->reqs, so can the barrier be removed?
Right. The comment is obsolete and dates to before vq->index existed.
OK, I will cook a patch to remove the barrier and cleanup the comment.
Thanks. Please remove the ACCESS_ONCE too.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
