virtio-scsi: replace target spinlock with seqcount
Subsystems (2)
Hi,
I think this patch has a small race involving just two commands:
1. The first command to a target is in virtscsi_pick_vq(), after atomic_inc_return(&tgt->tgt_lock)) but before write_seqcount_begin()
2. A second command to the same target enters virtscsi_pick_vq(). It will hit the same atomic inc, take the upper branch of the conditional, and read out a stale (or NULL) tgt->req_vq.
Specifically:
@@ -508,19 +507,33 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
unsigned long flags;
u32 queue_num;
- spin_lock_irqsave(&tgt->tgt_lock, flags);
+ local_irq_save(flags);
+ if (atomic_inc_return(&tgt->reqs) > 1) {
+ unsigned long seq;
+
+ do {
+ seq = read_seqcount_begin(&tgt->tgt_seq);
+ vq = tgt->req_vq;
+ } while (read_seqcount_retry(&tgt->tgt_seq, seq));
+ } else {
A second virtscsi_pick_vq() here will read a stale or NULL tgt->req_vq.
+ /* no writes can be concurrent because of atomic_t */
+ write_seqcount_begin(&tgt->tgt_seq);
+
+ /* keep previous req_vq if there is reader found */
+ if (unlikely(atomic_read(&tgt->reqs) > 1)) {
+ vq = tgt->req_vq;
+ goto unlock;
+ }
- if (atomic_inc_return(&tgt->reqs) > 1)
- vq = tgt->req_vq;
- else {
queue_num = smp_processor_id();
while (unlikely(queue_num >= vscsi->num_queues))
queue_num -= vscsi->num_queues;
-
tgt->req_vq = vq = &vscsi->req_vqs[queue_num];
+ unlock:
+ write_seqcount_end(&tgt->tgt_seq);
}
+ local_irq_restore(flags);
- spin_unlock_irqrestore(&tgt->tgt_lock, flags);
return vq;
Thanks,
-- vs;
Paolo Bonzini
reviewerIl 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
Quoted 28 lines
Hi,
I think this patch has a small race involving just two commands:
1. The first command to a target is in
virtscsi_pick_vq(), afteratomic_inc_return(&tgt->tgt_lock)) but beforewrite_seqcount_begin()
2. A second command to the same target entersvirtscsi_pick_vq(). It will hit the same atomic inc, take the upper branch of the conditional, and read out a stale (or NULL)tgt->req_vq.Specifically:
(quoted diff) +9
-1@@ -508,19 +507,33 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi, unsigned long flags; u32 queue_num; - spin_lock_irqsave(&tgt->tgt_lock, flags); + local_irq_save(flags); + if (atomic_inc_return(&tgt->reqs) > 1) { + unsigned long seq; + + do { + seq = read_seqcount_begin(&tgt->tgt_seq); + vq = tgt->req_vq; + } while (read_seqcount_retry(&tgt->tgt_seq, seq)); + } else {
Quoted 1 line
A second
virtscsi_pick_vq()here will read a stale or NULLtgt->req_vq.
The NULL case is true, indeed I was going to send a patch to initialize tgt->req_vq but I have not tested it.
drivers/scsi/virtio_scsi.c +4 -1
@@ -618,14 +631,17 @@ static int virtscsi_abort(struct scsi_cmnd *sc)static int virtscsi_target_alloc (struct scsi_target *starget) { +struct Scsi_Host *sh =dev_to_shost (starget->dev.parent); +struct virtio_scsi *vscsi =shost_priv (sh); +struct virtio_scsi_target_state *tgt =kmalloc (sizeof (*tgt),GFP_KERNEL );if (!tgt)return -ENOMEM ;seqcount_init (&tgt->tgt_seq);atomic_set (&tgt->reqs,0 ); - tgt->req_vq =NULL ; + tgt->req_vq = &vscsi->req_vqs[0 ]; starget->hostdata = tgt;return 0 ;
The case of a stale req_vq is okay and is the (IMO reasonable) price to pay for allowing more concurrency. If you have concurrent requests from multiple VCPUs, multiqueue is not great for your workload anyway, at least with the current steering algorithm.
Paolo
Quoted 8 lines · Paolo Bonzini
Il 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
Quoted 6 lines
Hi,
I think this patch has a small race involving just two commands:
1. The first command to a target is in
virtscsi_pick_vq(), afteratomic_inc_return(&tgt->tgt_lock)) but beforewrite_seqcount_begin()
Good catch, thanks Venkatesh
And it doesn't happen in my test, so looks it won't be easy to trigger, :-)
Quoted 3 lines
Quoted 3 lines
2. A second command to the same target enters
virtscsi_pick_vq(). It will hit the same atomic inc, take the upper branch of the conditional, and read out a stale (or NULL)tgt->req_vq.
Yes, that can't be avoided without spinlock and the 'stale' req_vq is still fine, and it isn't a big deal since the window is very very small.
Quoted 48 lines
Quoted 19 lines
Specifically:
(quoted diff) +9
-1@@ -508,19 +507,33 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi, unsigned long flags; u32 queue_num; - spin_lock_irqsave(&tgt->tgt_lock, flags); + local_irq_save(flags); + if (atomic_inc_return(&tgt->reqs) > 1) { + unsigned long seq; + + do { + seq = read_seqcount_begin(&tgt->tgt_seq); + vq = tgt->req_vq; + } while (read_seqcount_retry(&tgt->tgt_seq, seq)); + } else {Quoted 1 line
A second
virtscsi_pick_vq()here will read a stale or NULLtgt->req_vq.The NULL case is true, indeed I was going to send a patch to initialize
tgt->req_vqbut I have not tested it.drivers/scsi/virtio_scsi.c +4
-1@@ -618,14 +631,17 @@ static int virtscsi_abort(struct scsi_cmnd *sc)static int virtscsi_target_alloc (struct scsi_target *starget) { +struct Scsi_Host *sh =dev_to_shost (starget->dev.parent); +struct virtio_scsi *vscsi =shost_priv (sh); +struct virtio_scsi_target_state *tgt =kmalloc (sizeof (*tgt),GFP_KERNEL );if (!tgt)return -ENOMEM ;seqcount_init (&tgt->tgt_seq);atomic_set (&tgt->reqs,0 ); - tgt->req_vq =NULL ; + tgt->req_vq = &vscsi->req_vqs[0 ]; starget->hostdata = tgt;
Quoted 1 line
return 0;
Paolo, do you need me to integrate this one into the patch? or you will make it as one standalone?
Quoted 8 lines
The case of a stale
req_vqis okay and is the (IMO reasonable) price to pay for allowing more concurrency. If you have concurrent requests from multiple VCPUs, multiqueue is not great for your workload anyway, at least with the current steering algorithm.Paolo
Thanks,
Signature · 1 line
Ming Lei
Paolo Bonzini
reviewerIl 28/05/2014 03:48, Ming Lei ha scritto:
Quoted 5 lines · Paolo Bonzini
Quoted 4 lines
Il 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
Quoted 2 lines
Hi,
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
I think this patch has a small race involving just two commands:
1. The first command to a target is in
virtscsi_pick_vq(), afteratomic_inc_return(&tgt->tgt_lock)) but beforewrite_seqcount_begin()Good catch, thanks Venkatesh
And it doesn't happen in my test, so looks it won't be easy to trigger, :-)
No, it's basically impossible because the race window is very small and the first command (INQUIRY or REPORT LUNS) is likely to be synchronous anyway. But it's there anyway.
Quoted 46 lines
Quoted 46 lines
Quoted 18 lines
Specifically:
(quoted diff) +9
-1@@ -508,19 +507,33 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi, unsigned long flags; u32 queue_num; - spin_lock_irqsave(&tgt->tgt_lock, flags); + local_irq_save(flags); + if (atomic_inc_return(&tgt->reqs) > 1) { + unsigned long seq; + + do { + seq = read_seqcount_begin(&tgt->tgt_seq); + vq = tgt->req_vq; + } while (read_seqcount_retry(&tgt->tgt_seq, seq)); + } else {Quoted 1 line
A second
virtscsi_pick_vq()here will read a stale or NULLtgt->req_vq.The NULL case is true, indeed I was going to send a patch to initialize
tgt->req_vqbut I have not tested it.drivers/scsi/virtio_scsi.c +4
-1@@ -618,14 +631,17 @@ static int virtscsi_abort(struct scsi_cmnd *sc)static int virtscsi_target_alloc (struct scsi_target *starget) { +struct Scsi_Host *sh =dev_to_shost (starget->dev.parent); +struct virtio_scsi *vscsi =shost_priv (sh); +struct virtio_scsi_target_state *tgt =kmalloc (sizeof (*tgt),GFP_KERNEL );if (!tgt)return -ENOMEM ;seqcount_init (&tgt->tgt_seq);atomic_set (&tgt->reqs,0 ); - tgt->req_vq =NULL ; + tgt->req_vq = &vscsi->req_vqs[0 ];
Quoted 5 lines
Quoted 2 lines
drivers/scsi/virtio_scsi.c +0
-0starget->hostdata = tgt;return 0;Paolo, do you need me to integrate this one into the patch? or you will make it as one standalone?
I will integrate it myself after testing it.
Paolo