virtio_scsi: remove smp_read_barrier_depends
Subsystems (2)
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.
--- drivers/scsi/virtio_scsi.c | 52 +++----------------------------------------- 1 file changed, 3 insertions(+), 49 deletions(-)
drivers/scsi/virtio_scsi.c +3 -49
@@ -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
Paolo Bonzini
reviewerIl 07/05/2014 16:13, Ming Lei ha scritto:
Quoted 25 lines
order reading
req_vqand decreasingtgt->reqs, but it isn't needed now becausereq_vqis read from scsi->req_vqs[vq->index -VIRTIO_SCSI_VQ_BASE] instead oftgt->req_vq, so remove the unnecessary barrier.Also remove related comment about the barrier.
Cc Paolo Bonzini <[email protected]>Signed-off-by Ming Lei <[email protected]>--- drivers/scsi/virtio_scsi.c | 52 +++----------------------------------------- 1 file changed, 3 insertions(+), 49 deletions(-)drivers/scsi/virtio_scsi.c +0
-3@@ -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 {
Quoted 6 lines · Paolo Bonzini
tgt_lockis held to serialize reading and writingreq_vq. Readingreq_vqcould 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