* Re: [PATCH] virtio-scsi: replace target spinlock with seqcount
@ 2014-05-27 16:50 Venkatesh Srinivas
2014-05-27 16:57 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Venkatesh Srinivas @ 2014-05-27 16:50 UTC (permalink / raw)
To: linux-scsi; +Cc: tom.leiming, Paolo Bonzini
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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] virtio-scsi: replace target spinlock with seqcount
2014-05-27 16:50 [PATCH] virtio-scsi: replace target spinlock with seqcount Venkatesh Srinivas
@ 2014-05-27 16:57 ` Paolo Bonzini
2014-05-28 1:48 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2014-05-27 16:57 UTC (permalink / raw)
To: Venkatesh Srinivas, linux-scsi; +Cc: tom.leiming
Il 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
> 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.
The NULL case is true, indeed I was going to send a patch to initialize
tgt->req_vq but I have not tested it.
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index fc054935eb1f..f0b4cdbfceb0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] virtio-scsi: replace target spinlock with seqcount
2014-05-27 16:57 ` Paolo Bonzini
@ 2014-05-28 1:48 ` Ming Lei
2014-05-28 8:52 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2014-05-28 1:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Venkatesh Srinivas, Linux SCSI List
On Wed, May 28, 2014 at 12:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
>
>> 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()
Good catch, thanks Venkatesh
And it doesn't happen in my test, so looks it won't be easy to trigger, :-)
>> 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.
>>
>> 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.
>
>
> The NULL case is true, indeed I was going to send a patch to initialize
> tgt->req_vq but I have not tested it.
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index fc054935eb1f..f0b4cdbfceb0 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -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;
Paolo, do you need me to integrate this one into the patch? or
you will make it as one standalone?
>
>
> 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
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] virtio-scsi: replace target spinlock with seqcount
2014-05-28 1:48 ` Ming Lei
@ 2014-05-28 8:52 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-05-28 8:52 UTC (permalink / raw)
To: Ming Lei; +Cc: Venkatesh Srinivas, Linux SCSI List
Il 28/05/2014 03:48, Ming Lei ha scritto:
> On Wed, May 28, 2014 at 12:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 27/05/2014 18:50, Venkatesh Srinivas ha scritto:
>>
>>> 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()
>
> 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.
>>> 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.
>>
>>
>> The NULL case is true, indeed I was going to send a patch to initialize
>> tgt->req_vq but I have not tested it.
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index fc054935eb1f..f0b4cdbfceb0 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
>> @@ -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;
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] virtio-scsi: replace target spinlock with seqcount
@ 2014-05-23 13:28 Ming Lei
2014-05-23 13:59 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2014-05-23 13:28 UTC (permalink / raw)
To: James E.J. Bottomley, Paolo Bonzini
Cc: linux-scsi, Christoph Hellwig, Wanlong Gao, Rusty Russell,
Ming Lei
The spinlock of tgt_lock is only for serializing read and write
req_vq, one lockless seqcount is enough for the purpose.
On one 16core VM with vhost-scsi backend, the patch can improve
IOPS with 3% on random read test.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/scsi/virtio_scsi.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index d4727b3..9a81579 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -26,6 +26,7 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_device.h>
#include <scsi/scsi_cmnd.h>
+#include <linux/seqlock.h>
#define VIRTIO_SCSI_MEMPOOL_SZ 64
#define VIRTIO_SCSI_EVENT_LEN 8
@@ -73,18 +74,16 @@ 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).
*
- * 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.
+ * tgt_seq is held to serialize reading and writing req_vq.
*
* 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
+ * Thus they can happen outside the tgt_seq, 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. */
- spinlock_t tgt_lock;
+ seqcount_t tgt_seq;
/* Count of outstanding requests. */
atomic_t reqs;
@@ -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 {
+ /* 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;
}
@@ -610,7 +623,7 @@ static int virtscsi_target_alloc(struct scsi_target *starget)
if (!tgt)
return -ENOMEM;
- spin_lock_init(&tgt->tgt_lock);
+ seqcount_init(&tgt->tgt_seq);
atomic_set(&tgt->reqs, 0);
tgt->req_vq = NULL;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] virtio-scsi: replace target spinlock with seqcount
2014-05-23 13:28 Ming Lei
@ 2014-05-23 13:59 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-05-23 13:59 UTC (permalink / raw)
To: Ming Lei, James E.J. Bottomley
Cc: linux-scsi, Christoph Hellwig, Wanlong Gao, Rusty Russell
Il 23/05/2014 15:28, Ming Lei ha scritto:
> The spinlock of tgt_lock is only for serializing read and write
> req_vq, one lockless seqcount is enough for the purpose.
>
> On one 16core VM with vhost-scsi backend, the patch can improve
> IOPS with 3% on random read test.
Nice. :) The patch looks good, thanks for working this out!
James, I have a couple more patches planned for virtio-scsi, so I'll
send out the update next week.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-28 8:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-27 16:50 [PATCH] virtio-scsi: replace target spinlock with seqcount Venkatesh Srinivas
2014-05-27 16:57 ` Paolo Bonzini
2014-05-28 1:48 ` Ming Lei
2014-05-28 8:52 ` Paolo Bonzini
-- strict thread matches above, loose matches on Subject: below --
2014-05-23 13:28 Ming Lei
2014-05-23 13:59 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).