Re: virtio-scsi: two questions related with picking up queue

[Paolo Bonzini <pbonzini@redhat.com>]

Il 07/05/2014 03:07, Ming Lei ha scritto:
> Hi Paolo,
>
> On Tue, May 6, 2014 at 9:17 PM, Paolo Bonzini <pbonzini@redhat.com> 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