Skip to contentSkip to conversation

virtio_scsi: remove smp_read_barrier_depends

PATCH In review Ming Lei 2 participants 3 messages May 7, 2014

Subsystems (2)
lore
Share & tools
lore

Ming Lei

author
#0 lore raw

PATCH 2/2 virtio_scsi: remove smp_read_barrier_depends

The barrier 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 <[email protected]>
---
 drivers/scsi/virtio_scsi.c |   52 +++-----------------------------------------
 1 file changed, 3 insertions(+), 49 deletions(-)
drivers/scsi/virtio_scsi.c +3 -49
index aead20f..0c77ed6 100644
@@ -73,19 +73,9 @@ 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:
- *
- * - 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.
+ * tgt_lock is held to serialize reading and writing req_vq, and reading
+ * req_vq should have been done concurrently, but need a bit complicated
+ * trick.
  */
 struct virtio_scsi_target_state {
 	/* This spinlock never held at the same time as vq_lock. */
@@ -238,38 +228,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,10 +518,6 @@ 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 = tgt->req_vq;
 	else {
Signature · 1 line
1.7.9.5
reviewer
#1 lore raw

Il 07/05/2014 16:13, Ming Lei ha scritto:

Quoted 25 lines

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 <[email protected]>
---
 drivers/scsi/virtio_scsi.c |   52 +++-----------------------------------------
 1 file changed, 3 insertions(+), 49 deletions(-)
drivers/scsi/virtio_scsi.c +0 -3
index aead20f..0c77ed6 100644
@@ -73,19 +73,9 @@ 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:
- *

drivers/scsi/virtio_scsi.c struct virtio_scsi_vq { · line 88

Quoted 10 lines
drivers/scsi/virtio_scsi.c +0 -10
- * - 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.

You are removing a bit too much; decrements of reqs are still done outside the tgt_lock, and you need to explain why.

drivers/scsi/virtio_scsi.c struct virtio_scsi_vq { · line 78

Quoted 3 lines
drivers/scsi/virtio_scsi.c +3 -0
+ * tgt_lock is held to serialize reading and writing req_vq, and reading
+ * req_vq should have been done concurrently, but need a bit complicated
+ * trick.

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.

You can also merge the two patches together.

Paolo

Quoted 54 lines
drivers/scsi/virtio_scsi.c +0 -36
  */
 struct virtio_scsi_target_state {
 	/* This spinlock never held at the same time as vq_lock. */
@@ -238,38 +228,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,10 +518,6 @@ 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 = tgt->req_vq;
 	else {
author
#2 lore raw
Quoted 6 lines · Paolo Bonzini

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.

You can also merge the two patches together.

OK, please ignore these two patches, and I have sent out the merged one.

Thanks,

Signature · 1 line
Ming Lei