* [PATCH v1] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends()
@ 2014-05-08 5:24 Ming Lei
2014-05-08 7:05 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Ming Lei @ 2014-05-08 5:24 UTC (permalink / raw)
To: James E.J. Bottomley, Paolo Bonzini
Cc: linux-scsi, Wanlong Gao, Rusty Russell, Ming Lei
Access to tgt->req_vq is strictly serialized by spin_lock
of tgt->tgt_lock, so the ACCESS_ONCE() isn't necessary.
smp_read_barrier_depends() in virtscsi_req_done was introduced
to order reading req_vq and decreasing tgt->reqs, but it isn't
needed now because req_vq is read from
scsi->req_vqs[vq->index - VIRTIO_SCSI_VQ_BASE] instead of
tgt->req_vq, so remove the unnecessary barrier.
Also remove related comment about the barrier.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
v1:
- fix comment on decrements of reqs with writing of req_vq
as suggested by Paolo
drivers/scsi/virtio_scsi.c | 55 +++++---------------------------------------
1 file changed, 6 insertions(+), 49 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 16bfd50..bedea60 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -73,19 +73,12 @@ struct virtio_scsi_vq {
* queue, and also lets the driver optimize the IRQ affinity for the virtqueues
* (each virtqueue's affinity is set to the CPU that "owns" the queue).
*
- * An interesting effect of this policy is that only writes to req_vq need to
- * take the tgt_lock. Read can be done outside the lock because:
+ * tgt_lock is held to serialize reading and writing req_vq. Reading req_vq
+ * could be done locklessly, but we do not do it yet.
*
- * - writes of req_vq only occur when atomic_inc_return(&tgt->reqs) returns 1.
- * In that case, no other CPU is reading req_vq: even if they were in
- * virtscsi_queuecommand_multi, they would be spinning on tgt_lock.
- *
- * - reads of req_vq only occur when the target is not idle (reqs != 0).
- * A CPU that enters virtscsi_queuecommand_multi will not modify req_vq.
- *
- * Similarly, decrements of reqs are never concurrent with writes of req_vq.
- * Thus they can happen outside the tgt_lock, provided of course we make reqs
- * an atomic_t.
+ * Decrements of reqs are never concurrent with writes of req_vq because
+ * request is always completd after being queued. Thus they can happen
+ * outside the tgt_lock, provided of course we make reqs an atomic_t.
*/
struct virtio_scsi_target_state {
/* This spinlock never held at the same time as vq_lock. */
@@ -238,38 +231,6 @@ static void virtscsi_req_done(struct virtqueue *vq)
int index = vq->index - VIRTIO_SCSI_VQ_BASE;
struct virtio_scsi_vq *req_vq = &vscsi->req_vqs[index];
- /*
- * Read req_vq before decrementing the reqs field in
- * virtscsi_complete_cmd.
- *
- * With barriers:
- *
- * CPU #0 virtscsi_queuecommand_multi (CPU #1)
- * ------------------------------------------------------------
- * lock vq_lock
- * read req_vq
- * read reqs (reqs = 1)
- * write reqs (reqs = 0)
- * increment reqs (reqs = 1)
- * write req_vq
- *
- * Possible reordering without barriers:
- *
- * CPU #0 virtscsi_queuecommand_multi (CPU #1)
- * ------------------------------------------------------------
- * lock vq_lock
- * read reqs (reqs = 1)
- * write reqs (reqs = 0)
- * increment reqs (reqs = 1)
- * write req_vq
- * read (wrong) req_vq
- *
- * We do not need a full smp_rmb, because req_vq is required to get
- * to tgt->reqs: tgt is &vscsi->tgt[sc->device->id], where sc is stored
- * in the virtqueue as the user token.
- */
- smp_read_barrier_depends();
-
virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
};
@@ -560,12 +521,8 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
spin_lock_irqsave(&tgt->tgt_lock, flags);
- /*
- * The memory barrier after atomic_inc_return matches
- * the smp_read_barrier_depends() in virtscsi_req_done.
- */
if (atomic_inc_return(&tgt->reqs) > 1)
- vq = ACCESS_ONCE(tgt->req_vq);
+ vq = tgt->req_vq;
else {
queue_num = smp_processor_id();
while (unlikely(queue_num >= vscsi->num_queues))
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends()
2014-05-08 5:24 [PATCH v1] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends() Ming Lei
@ 2014-05-08 7:05 ` Paolo Bonzini
2014-05-08 7:25 ` Ming Lei
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2014-05-08 7:05 UTC (permalink / raw)
To: Ming Lei, James E.J. Bottomley; +Cc: linux-scsi, Wanlong Gao, Rusty Russell
Il 08/05/2014 07:24, Ming Lei ha scritto:
> Access to tgt->req_vq is strictly serialized by spin_lock
> of tgt->tgt_lock, so the ACCESS_ONCE() isn't necessary.
>
> smp_read_barrier_depends() in virtscsi_req_done was introduced
> to order reading req_vq and decreasing tgt->reqs, but it isn't
> needed now because req_vq is read from
> scsi->req_vqs[vq->index - VIRTIO_SCSI_VQ_BASE] instead of
> tgt->req_vq, so remove the unnecessary barrier.
>
> Also remove related comment about the barrier.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
> v1:
> - fix comment on decrements of reqs with writing of req_vq
> as suggested by Paolo
>
> drivers/scsi/virtio_scsi.c | 55 +++++---------------------------------------
> 1 file changed, 6 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 16bfd50..bedea60 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -73,19 +73,12 @@ struct virtio_scsi_vq {
> * queue, and also lets the driver optimize the IRQ affinity for the virtqueues
> * (each virtqueue's affinity is set to the CPU that "owns" the queue).
> *
> - * An interesting effect of this policy is that only writes to req_vq need to
> - * take the tgt_lock. Read can be done outside the lock because:
> + * tgt_lock is held to serialize reading and writing req_vq. Reading req_vq
> + * could be done locklessly, but we do not do it yet.
> *
> - * - writes of req_vq only occur when atomic_inc_return(&tgt->reqs) returns 1.
> - * In that case, no other CPU is reading req_vq: even if they were in
> - * virtscsi_queuecommand_multi, they would be spinning on tgt_lock.
> - *
> - * - reads of req_vq only occur when the target is not idle (reqs != 0).
> - * A CPU that enters virtscsi_queuecommand_multi will not modify req_vq.
> - *
> - * Similarly, decrements of reqs are never concurrent with writes of req_vq.
> - * Thus they can happen outside the tgt_lock, provided of course we make reqs
> - * an atomic_t.
> + * Decrements of reqs are never concurrent with writes of req_vq because
> + * request is always completd after being queued. Thus they can happen
> + * outside the tgt_lock, provided of course we make reqs an atomic_t.
Decrements of reqs are never concurrent with writes of req_vq: before
the decrement reqs will be != 0; after the decrement the virtqueue
completion routine will not use the req_vq so it can be changed by a new
request. Thus they can happen outside the tgt_lock, provided of course
we make reqs an atomic_t.
Otherwise looks good.
Paolo
> */
> struct virtio_scsi_target_state {
> /* This spinlock never held at the same time as vq_lock. */
> @@ -238,38 +231,6 @@ static void virtscsi_req_done(struct virtqueue *vq)
> int index = vq->index - VIRTIO_SCSI_VQ_BASE;
> struct virtio_scsi_vq *req_vq = &vscsi->req_vqs[index];
>
> - /*
> - * Read req_vq before decrementing the reqs field in
> - * virtscsi_complete_cmd.
> - *
> - * With barriers:
> - *
> - * CPU #0 virtscsi_queuecommand_multi (CPU #1)
> - * ------------------------------------------------------------
> - * lock vq_lock
> - * read req_vq
> - * read reqs (reqs = 1)
> - * write reqs (reqs = 0)
> - * increment reqs (reqs = 1)
> - * write req_vq
> - *
> - * Possible reordering without barriers:
> - *
> - * CPU #0 virtscsi_queuecommand_multi (CPU #1)
> - * ------------------------------------------------------------
> - * lock vq_lock
> - * read reqs (reqs = 1)
> - * write reqs (reqs = 0)
> - * increment reqs (reqs = 1)
> - * write req_vq
> - * read (wrong) req_vq
> - *
> - * We do not need a full smp_rmb, because req_vq is required to get
> - * to tgt->reqs: tgt is &vscsi->tgt[sc->device->id], where sc is stored
> - * in the virtqueue as the user token.
> - */
> - smp_read_barrier_depends();
> -
> virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
> };
>
> @@ -560,12 +521,8 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
>
> spin_lock_irqsave(&tgt->tgt_lock, flags);
>
> - /*
> - * The memory barrier after atomic_inc_return matches
> - * the smp_read_barrier_depends() in virtscsi_req_done.
> - */
> if (atomic_inc_return(&tgt->reqs) > 1)
> - vq = ACCESS_ONCE(tgt->req_vq);
> + vq = tgt->req_vq;
> else {
> queue_num = smp_processor_id();
> while (unlikely(queue_num >= vscsi->num_queues))
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends()
2014-05-08 7:05 ` Paolo Bonzini
@ 2014-05-08 7:25 ` Ming Lei
0 siblings, 0 replies; 3+ messages in thread
From: Ming Lei @ 2014-05-08 7:25 UTC (permalink / raw)
To: Paolo Bonzini
Cc: James E.J. Bottomley, Linux SCSI List, Wanlong Gao, Rusty Russell
On Thu, May 8, 2014 at 3:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> Decrements of reqs are never concurrent with writes of req_vq: before the
> decrement reqs will be != 0; after the decrement the virtqueue completion
> routine will not use the req_vq so it can be changed by a new request. Thus
> they can happen outside the tgt_lock, provided of course we make reqs an
> atomic_t.
OK, will take above comment in V2.
> Otherwise looks good.
Thanks for your review.
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-08 7:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-08 5:24 [PATCH v1] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends() Ming Lei
2014-05-08 7:05 ` Paolo Bonzini
2014-05-08 7:25 ` Ming Lei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.