Il 08/05/2014 14:55, Ming Lei ha scritto:
On Thu, May 8, 2014 at 8:17 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
Il 08/05/2014 12:44, Ming Lei ha scritto:
On Wed, 07 May 2014 18:43:45 +0200
Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
Per-CPU spinlocks have bad scalability problems, especially if you're
overcommitting. Writing req_vq is not at all rare.
OK, thought about it further, and I believe seqcount may
be a match for the case, could you take a look at below patch?
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 13dd500..1adbad7 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;
@@ -521,19 +520,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;
+ }
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);
I find this harder to think about than the double-check with a
spin_lock_irqsave in the middle,
Sorry, could you explain it a bit? With seqcount, spin_lock
isn't needed, which should have been used for serialize
read and write.
Yes, the spin lock is not needed but you are still potentially spinning
on the read side.
and the read side is not lock free anymore.
It is still lock free, because reader won't block reader, and
both read_seqcount_begin and read_seqcount_retry only
checks if there is writer in progress or being completed,
and the two helpers are very cheap.
Lock-free has a precise meaning, which is that the system will progress
regardless of scheduling. In this case, readers won't progress while
the writer is preempted between write_seqcount_begin and write_seqcount_end.
My cmpxchg example had a lock-free read-side and a blocking write-side,
while your code is the opposite, the write-side is lock-free and the
read-side is blocking.
I'm not sure which is better. You can try both and the current code,
and show that some benchmarks improve. Otherwise, it's better to leave
the current code. Simple code is better that complex code that was
never benchmarked (which is why in the end I and Wanlong Gao settled for
the simple spinlock).
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
