Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support

On Tue, Sep 04, 2012 at 08:46:12AM +0200, Paolo Bonzini wrote:

Il 04/09/2012 04:21, Nicholas A. Bellinger ha scritto:

quoted

quoted

@@ -112,6 +118,9 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
 	struct virtio_scsi_cmd *cmd = buf;
 	struct scsi_cmnd *sc = cmd->sc;
 	struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
+	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+
+	atomic_dec(&tgt->reqs);
 

As tgt->tgt_lock is taken in virtscsi_queuecommand_multi() before the
atomic_inc_return(tgt->reqs) check, it seems like using atomic_dec() w/o
smp_mb__after_atomic_dec or tgt_lock access here is not using atomic.h
accessors properly, no..?

No, only a single "thing" is being accessed, and there is no need to
order the decrement with respect to preceding or subsequent accesses to
other locations.

In other words, tgt->reqs is already synchronized with itself, and that
is enough.

I think your logic is correct and barrier is not needed,
but this needs better documentation.

(Besides, on x86 smp_mb__after_atomic_dec is a nop).

quoted

quoted

+static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
+				       struct scsi_cmnd *sc)
+{
+	struct virtio_scsi *vscsi = shost_priv(sh);
+	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+	unsigned long flags;
+	u32 queue_num;
+
+	/* Using an atomic_t for tgt->reqs lets the virtqueue handler
+	 * decrement it without taking the spinlock.
+	 */

Above comment is not really helpful - reader can be safely assumed to
know what atomic_t is.

Please delete, and replace with the text from commit log
that explains the heuristic used to select req_vq.

Also please add a comment near 'reqs' definition.
Something like "number of outstanding requests - used to detect idle
target".

quoted

quoted

+	spin_lock_irqsave(&tgt->tgt_lock, flags);

Looks like this lock can be removed - req_vq is only
modified when target is idle and only used when it is
not idle.

quoted

quoted

+	if (atomic_inc_return(&tgt->reqs) == 1) {
+		queue_num = smp_processor_id();
+		while (unlikely(queue_num >= vscsi->num_queues))
+			queue_num -= vscsi->num_queues;
+		tgt->req_vq = &vscsi->req_vqs[queue_num];
+	}
+	spin_unlock_irqrestore(&tgt->tgt_lock, flags);
+	return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
+

.....

quoted

quoted

+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+                                       struct scsi_cmnd *sc)
+{
+       struct virtio_scsi *vscsi = shost_priv(sh);
+       struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+
+       atomic_inc(&tgt->reqs);
+       return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+

Here, reqs is unused - why bother incrementing it?
A branch on completion would be cheaper IMHO.

virtio-scsi multiqueue has a performance benefit up to 20%

To be fair, you could be running in single queue mode.
In that case extra atomics and indirection that this code
brings will just add overhead without benefits.
I don't know how significant would that be.

-- 
MST