* Re: [PATCH] virtio: Don't access device data after unregistration.
From: Michael S. Tsirkin @ 2012-09-03 20:18 UTC (permalink / raw)
To: Sjur Brændeland; +Cc: linux-kernel, Guzman Lugo, Fernadndo, virtualization
In-Reply-To: <CAJK669aRsbOOmjmT+K8OHYvenOhkYWuSOx6nBM7hxC_C4mmw-g@mail.gmail.com>
On Mon, Sep 03, 2012 at 04:50:10PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> >> Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set.
> >
> > What's the root cause of the panic?
>
> I believe the cause of the panic is calling
> ida_simple_remove(&virtio_index_ida, dev->index);
> when the dev structure is "poisoned" after kfree.
> It might be the "BUG_ON((int)id < 0)" that bites...
>
> >> Use device_del() and put_device() instead of
> >> device_unregister(), and access device data before
> >> calling put_device().
>
> > Why does this help? Does device_unregister free the
> > device so dev->index access crashes?
>
> Yes, if device ref-count is one when calling unregister
> the device is freed.
Interesting. Where exactly? Note that:
struct rproc_vdev {
struct list_head node;
struct rproc *rproc;
struct virtio_device vdev;
struct rproc_vring vring[RVDEV_NUM_VRINGS];
unsigned long dfeatures;
unsigned long gfeatures;
};
kfree(&proc_vdev->vdev) is unlikely to be the right thing to do.
> > If yes virtio_pci_remove will crash too
> > as it accesses the device after the
> > call to unregister_virtio_device so the
> > fix won't be effective.
>
> I discovered this using the remoteproc framework.
> It might be that device is unregistered with ref-count greater
> than one normally, in that case this bug will not show up.
>
> Regards,
> Sjur
It might be remoteproc has an unrelated bug?
--
MST
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-03 20:27 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah
In-Reply-To: <CAJK669bkHHMAPYMEr22-6uOwfgJv1uKTbzNBQVVCq=OooExC0g@mail.gmail.com>
On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> > How does access to descriptors work in this setup?
>
> When the ring is setup by remoteproc the descriptors are
> also allocated using dma_alloc_coherent().
>
> >> -static void free_buf(struct port_buffer *buf)
> >> +/* Allcoate data buffer from DMA memory if requested */
> >
> > typo
>
> Thanks.
>
> >> +static inline void *
> >> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
> >> + gfp_t flag)
> >> {
> >> - kfree(buf->buf);
> >> +#ifdef CONFIG_HAS_DMA
> >> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> >> + struct device *dev = &vdev->dev;
> >> + /*
> >> + * Allocate DMA memory from ancestors. Finding the ancestor
> >> + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
> >> + * implemented.
> >> + */
> >> + dev = dev->parent ? dev->parent : dev;
> >> + dev = dev->parent ? dev->parent : dev;
> >> + return dma_alloc_coherent(dev, size, dma_handle, flag);
> >> + }
> >> +#endif
> >
> > Are these ifdefs really needed? If DMA_MEM is set,
> > can't we use dma_alloc_coherent
> > unconditionally?
>
> If an architecture do not support DMA you will get
> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Regards,
> Sjur
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
If yes you should just fail load.
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:
#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
Now use if instead of ifdef.
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Nicholas A. Bellinger @ 2012-09-04 2:21 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, kvm, linux-scsi, mst, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <1346154857-12487-6-git-send-email-pbonzini@redhat.com>
On Tue, 2012-08-28 at 13:54 +0200, Paolo Bonzini wrote:
> This patch adds queue steering to virtio-scsi. When a target is sent
> multiple requests, we always drive them to the same queue so that FIFO
> processing order is kept. However, if a target was idle, we can choose
> a queue arbitrarily. In this case the queue is chosen according to the
> current VCPU, so the driver expects the number of request queues to be
> equal to the number of VCPUs. This makes it easy and fast to select
> the 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).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
Hey Paolo & Co,
I've not had a chance to try this with tcm_vhost just yet, but noticed
one thing wrt to assumptions about virtio_scsi_target_state->reqs access
below..
> drivers/scsi/virtio_scsi.c | 162 +++++++++++++++++++++++++++++++++++---------
> 1 files changed, 130 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 6414ea0..0c4b096 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -26,6 +26,7 @@
>
> #define VIRTIO_SCSI_MEMPOOL_SZ 64
> #define VIRTIO_SCSI_EVENT_LEN 8
> +#define VIRTIO_SCSI_VQ_BASE 2
>
> /* Command queue element */
> struct virtio_scsi_cmd {
> @@ -59,9 +60,13 @@ struct virtio_scsi_vq {
>
> /* Per-target queue state */
> struct virtio_scsi_target_state {
> - /* Protects sg. Lock hierarchy is tgt_lock -> vq_lock. */
> + /* Protects sg, req_vq. Lock hierarchy is tgt_lock -> vq_lock. */
> spinlock_t tgt_lock;
>
> + struct virtio_scsi_vq *req_vq;
> +
> + atomic_t reqs;
> +
> /* For sglist construction when adding commands to the virtqueue. */
> struct scatterlist sg[];
> };
> @@ -70,14 +75,15 @@ struct virtio_scsi_target_state {
> struct virtio_scsi {
> struct virtio_device *vdev;
>
> - struct virtio_scsi_vq ctrl_vq;
> - struct virtio_scsi_vq event_vq;
> - struct virtio_scsi_vq req_vq;
> -
> /* Get some buffers ready for event vq */
> struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
>
> + u32 num_queues;
> struct virtio_scsi_target_state **tgt;
> +
> + struct virtio_scsi_vq ctrl_vq;
> + struct virtio_scsi_vq event_vq;
> + struct virtio_scsi_vq req_vqs[];
> };
>
> static struct kmem_cache *virtscsi_cmd_cache;
> @@ -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..?
> dev_dbg(&sc->device->sdev_gendev,
> "cmd %p response %u status %#02x sense_len %u\n",
> @@ -185,11 +194,13 @@ static void virtscsi_req_done(struct virtqueue *vq)
> {
> struct Scsi_Host *sh = virtio_scsi_host(vq->vdev);
> struct virtio_scsi *vscsi = shost_priv(sh);
> + int index = virtqueue_get_queue_index(vq) - VIRTIO_SCSI_VQ_BASE;
> + struct virtio_scsi_vq *req_vq = &vscsi->req_vqs[index];
> unsigned long flags;
>
> - spin_lock_irqsave(&vscsi->req_vq.vq_lock, flags);
> + spin_lock_irqsave(&req_vq->vq_lock, flags);
> virtscsi_vq_done(vscsi, vq, virtscsi_complete_cmd);
> - spin_unlock_irqrestore(&vscsi->req_vq.vq_lock, flags);
> + spin_unlock_irqrestore(&req_vq->vq_lock, flags);
> };
>
> static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
> @@ -429,10 +440,10 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
> return ret;
> }
>
> -static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
> +static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
> + struct virtio_scsi_target_state *tgt,
> + struct scsi_cmnd *sc)
> {
> - struct virtio_scsi *vscsi = shost_priv(sh);
> - struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
> struct virtio_scsi_cmd *cmd;
> int ret;
>
> @@ -466,7 +477,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
> BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
> memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>
> - if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
> + if (virtscsi_kick_cmd(tgt, tgt->req_vq, cmd,
> sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
> GFP_ATOMIC) >= 0)
> ret = 0;
> @@ -475,6 +486,38 @@ out:
> return ret;
> }
>
> +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);
> +}
> +
...
> +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.
> + */
> + spin_lock_irqsave(&tgt->tgt_lock, flags);
> + 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);
> +}
> +
The extra memory barriers to get this right for the current approach are
just going to slow things down even more for virtio-scsi-mq..
After hearing Jen's blk-mq talk last week in San Diego + having a look
at the new code in linux-block.git/new-queue, the approach of using a
per-cpu lock-less-list hw -> sw queue that uses IPI + numa_node hints
to make smart decisions for the completion path is making alot of sense.
Jen's approach is what we will ultimately need to re-architect in SCSI
core if we're ever going to move beyond the issues of legacy host_lock,
so I'm wondering if maybe this is the direction that virtio-scsi-mq
needs to go in as well..?
--nab
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Rusty Russell @ 2012-09-04 6:07 UTC (permalink / raw)
To: Michael S. Tsirkin, Sjur Brændeland
Cc: Amit Shah, Linus Walleij, linux-kernel, virtualization
In-Reply-To: <20120903202737.GD6181@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif
>
> Now use if instead of ifdef.
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Rusty Russell @ 2012-09-04 6:23 UTC (permalink / raw)
To: Kent Overstreet, mst; +Cc: linux-kernel, virtualization
In-Reply-To: <20120903044133.GA9980@moria.home.lan>
Kent Overstreet <koverstreet@google.com> writes:
> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> instead.
This is a slight mis-understanding. It's supposed to be selected by
the particular driver, probably virtio_pci in your case.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 6:46 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, kvm, linux-scsi, mst, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <1346725294.4162.79.camel@haakon2.linux-iscsi.org>
Il 04/09/2012 04:21, Nicholas A. Bellinger ha scritto:
>> @@ -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.
(Besides, on x86 smp_mb__after_atomic_dec is a nop).
>> +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.
>> + */
>> + spin_lock_irqsave(&tgt->tgt_lock, flags);
>> + 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);
>> +}
>> +
>
> The extra memory barriers to get this right for the current approach are
> just going to slow things down even more for virtio-scsi-mq..
virtio-scsi multiqueue has a performance benefit up to 20% (for a single
LUN) or 40% (on overall bandwidth across multiple LUNs). I doubt that a
single memory barrier can have that much impact. :)
The way to go to improve performance even more is to add new virtio APIs
for finer control of the usage of the ring. These should let us avoid
copying the sg list and almost get rid of the tgt_lock; even though the
locking is quite efficient in virtio-scsi (see how tgt_lock and vq_lock
are "pipelined" so as to overlap the preparation of two requests), it
should give a nice improvement and especially avoid a kmalloc with small
requests. I may have some time for it next month.
> Jen's approach is what we will ultimately need to re-architect in SCSI
> core if we're ever going to move beyond the issues of legacy host_lock,
> so I'm wondering if maybe this is the direction that virtio-scsi-mq
> needs to go in as well..?
We can see after the block layer multiqueue work goes in... I also need
to look more closely at Jens's changes.
Have you measured the host_lock to be a bottleneck in high-iops
benchmarks, even for a modern driver that does not hold it in
queuecommand? (Certainly it will become more important as the
virtio-scsi queuecommand becomes thinner and thinner). If so, we can
start looking at limiting host_lock usage in the fast path.
BTW, supporting this in tcm-vhost should be quite trivial, as all the
request queues are the same and all serialization is done in the
virtio-scsi driver.
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 8:46 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <5045A3B4.2030101@redhat.com>
On Tue, Sep 04, 2012 at 08:46:12AM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 04:21, Nicholas A. Bellinger ha scritto:
> >> @@ -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).
> >> +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".
> >> + 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.
> >> + 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);
> >> +}
> >> +
> >> +
.....
> >> +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
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 10:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <20120904084628.GA8437@redhat.com>
Il 04/09/2012 10:46, Michael S. Tsirkin ha scritto:
>>>> +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.
Sure, the comment explains that we use an atomic because _elsewhere_ the
tgt_lock is not held while modifying reqs.
> Please delete, and replace with the text from commit log
> that explains the heuristic used to select req_vq.
Ok.
> Also please add a comment near 'reqs' definition.
> Something like "number of outstanding requests - used to detect idle
> target".
Ok.
>
>>>> + 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.
If you have two incoming requests at the same time, req_vq is also
modified when the target is not idle; that's the point of the lock.
Suppose tgt->reqs = 0 initially, and you have two processors/queues.
Initially tgt->req_vq is queue #1. If you have this:
queuecommand on CPU #0 queuecommand #2 on CPU #1
--------------------------------------------------------------
atomic_inc_return(...) == 1
atomic_inc_return(...) == 2
virtscsi_queuecommand to queue #1
tgt->req_vq = queue #0
virtscsi_queuecommand to queue #0
then two requests are issued to different queues without a quiescent
point in the middle.
>>>> + 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);
>>>> +}
>>>> +
>>>> +
>
> .....
>
>>>> +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.
Well, I could also let tgt->reqs go negative, but it would be a bit untidy.
Another alternative is to access the target's target_busy field with
ACCESS_ONCE, and drop reqs altogether. Too tricky to do this kind of
micro-optimization so early, though.
>> 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.
Not measurable in my experiments.
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 11:09 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <5045D6FF.5020801@redhat.com>
On Tue, Sep 04, 2012 at 12:25:03PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 10:46, Michael S. Tsirkin ha scritto:
> >>>> +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.
>
> Sure, the comment explains that we use an atomic because _elsewhere_ the
> tgt_lock is not held while modifying reqs.
>
> > Please delete, and replace with the text from commit log
> > that explains the heuristic used to select req_vq.
>
> Ok.
>
> > Also please add a comment near 'reqs' definition.
> > Something like "number of outstanding requests - used to detect idle
> > target".
>
> Ok.
>
> >
> >>>> + 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.
>
> If you have two incoming requests at the same time, req_vq is also
> modified when the target is not idle; that's the point of the lock.
>
> Suppose tgt->reqs = 0 initially, and you have two processors/queues.
> Initially tgt->req_vq is queue #1. If you have this:
>
> queuecommand on CPU #0 queuecommand #2 on CPU #1
> --------------------------------------------------------------
> atomic_inc_return(...) == 1
> atomic_inc_return(...) == 2
> virtscsi_queuecommand to queue #1
> tgt->req_vq = queue #0
> virtscsi_queuecommand to queue #0
>
> then two requests are issued to different queues without a quiescent
> point in the middle.
What happens then? Does this break correctness?
> >>>> + 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);
> >>>> +}
> >>>> +
> >>>> +
> >
> > .....
> >
> >>>> +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.
>
> Well, I could also let tgt->reqs go negative, but it would be a bit untidy.
>
> Another alternative is to access the target's target_busy field with
> ACCESS_ONCE, and drop reqs altogether. Too tricky to do this kind of
> micro-optimization so early, though.
So keep it simple and just check a flag.
> >> 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.
>
> Not measurable in my experiments.
>
> Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 11:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <20120904110905.GA9119@redhat.com>
Il 04/09/2012 13:09, Michael S. Tsirkin ha scritto:
>> > queuecommand on CPU #0 queuecommand #2 on CPU #1
>> > --------------------------------------------------------------
>> > atomic_inc_return(...) == 1
>> > atomic_inc_return(...) == 2
>> > virtscsi_queuecommand to queue #1
>> > tgt->req_vq = queue #0
>> > virtscsi_queuecommand to queue #0
>> >
>> > then two requests are issued to different queues without a quiescent
>> > point in the middle.
> What happens then? Does this break correctness?
Yes, requests to the same target should be processed in FIFO order, or
you have things like a flush issued before the write it was supposed to
flush. This is why I can only change the queue when there is no request
pending.
Paolo
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland @ 2012-09-04 11:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah
In-Reply-To: <20120903202737.GD6181@redhat.com>
Hi Michael,
>> If an architecture do not support DMA you will get
>> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Yes, it even seems intentional.
> But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.
> Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.
> If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:
if (VIRTIO_CONSOLE_HAS_DMA &&
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...
This does not give any linker warnings when compiling for UML (no DMA).
Regards,
Sjur
^ permalink raw reply
* Re: [PATCH] virtio: Don't access device data after unregistration.
From: Sjur Brændeland @ 2012-09-04 12:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, Guzman Lugo, Fernadndo, virtualization
In-Reply-To: <20120903201847.GC6181@redhat.com>
Hi Michael,
>> >> Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set.
>> >
>> > What's the root cause of the panic?
>>
>> I believe the cause of the panic is calling
>> ida_simple_remove(&virtio_index_ida, dev->index);
>> when the dev structure is "poisoned" after kfree.
>> It might be the "BUG_ON((int)id < 0)" that bites...
>>
>> >> Use device_del() and put_device() instead of
>> >> device_unregister(), and access device data before
>> >> calling put_device().
>>
>> > Why does this help? Does device_unregister free the
>> > device so dev->index access crashes?
>>
>> Yes, if device ref-count is one when calling unregister
>> the device is freed.
>
> Interesting. Where exactly?...
I was wrong here, the reason is not related to ref-count being
above one. The reason this issue do not show up in virtio_pci
is that the release function is a dummy:
[snip]
static void virtio_pci_release_dev(struct device *_d)
{
/*
* No need for a release method as we allocate/free
* all devices together with the pci devices.
* Provide an empty one to avoid getting a warning from core.
*/
}
The device structure uses a kref for reference counting the device.
In virtio_pci() the release function virtio_pci_release_dev()
will be called when the device is unregistered, but because the
release function is dummy, data isn't freed or reset at this point.
So for virtio devices created from virtio_pci my patch is not
currently needed.
However, empty release functions are not the preferred way, e.g look at
https://lkml.org/lkml/2012/4/3/301
[Greg K.H:]
> > > > +static void hsi_port_release(struct device *dev __maybe_unused)
> > > > +{
> > > > +}
> > >
> > > As per the documentation in the kernel tree, I get to mock you
> > > mercilessly for doing something as foolish as this. You are not smarter
> > > than the kernel and don't think that you got rid of the kernel warning
> > > properly by doing this. Do you think that I wrote that code for no good
> > > reason? The kernel was being nice and telling you what you did wrong,
> > > don't try to fake it out, it's smarter than you are here.
But remoteproc frees the device memory in the release function
rproc_vdev_release() and needs this patch.
Regards,
Sjur
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 12:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <1346154857-12487-6-git-send-email-pbonzini@redhat.com>
On Tue, Aug 28, 2012 at 01:54:17PM +0200, Paolo Bonzini wrote:
> This patch adds queue steering to virtio-scsi. When a target is sent
> multiple requests, we always drive them to the same queue so that FIFO
> processing order is kept. However, if a target was idle, we can choose
> a queue arbitrarily. In this case the queue is chosen according to the
> current VCPU, so the driver expects the number of request queues to be
> equal to the number of VCPUs. This makes it easy and fast to select
> the 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).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
I guess an alternative is a per-target vq.
Is the reason you avoid this that you expect more targets
than cpus? If yes this is something you might want to
mention in the log.
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 13:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <5045E387.4030103@redhat.com>
On Tue, Sep 04, 2012 at 01:18:31PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 13:09, Michael S. Tsirkin ha scritto:
> >> > queuecommand on CPU #0 queuecommand #2 on CPU #1
> >> > --------------------------------------------------------------
> >> > atomic_inc_return(...) == 1
> >> > atomic_inc_return(...) == 2
> >> > virtscsi_queuecommand to queue #1
> >> > tgt->req_vq = queue #0
> >> > virtscsi_queuecommand to queue #0
> >> >
> >> > then two requests are issued to different queues without a quiescent
> >> > point in the middle.
> > What happens then? Does this break correctness?
>
> Yes, requests to the same target should be processed in FIFO order, or
> you have things like a flush issued before the write it was supposed to
> flush. This is why I can only change the queue when there is no request
> pending.
>
> Paolo
I see. I guess you can rewrite this as:
atomic_inc
if (atomic_read() == 1)
which is a bit cheaper, and make the fact
that you do not need increment and return to be atomic,
explicit.
Another simple idea: store last processor id in target,
if it is unchanged no need to play with req_vq
and take spinlock.
Also - some kind of comment explaining why a similar race can not happen
with this lock in place would be nice: I see why this specific race can
not trigger but since lock is dropped later before you submit command, I
have hard time convincing myself what exactly gurantees that vq is never
switched before or even while command is submitted.
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 13:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <20120904133543.GF9805@redhat.com>
Il 04/09/2012 15:35, Michael S. Tsirkin ha scritto:
> I see. I guess you can rewrite this as:
> atomic_inc
> if (atomic_read() == 1)
> which is a bit cheaper, and make the fact
> that you do not need increment and return to be atomic,
> explicit.
It seems more complicated to me for hardly any reason. (Besides, is it
cheaper? It has one less memory barrier on some architectures I frankly
do not care much about---not on x86---but it also has two memory
accesses instead of one on all architectures).
> Another simple idea: store last processor id in target,
> if it is unchanged no need to play with req_vq
> and take spinlock.
Not so sure, consider the previous example with last_processor_id equal
to 1.
queuecommand on CPU #0 queuecommand #2 on CPU #1
--------------------------------------------------------------
atomic_inc_return(...) == 1
atomic_inc_return(...) == 2
virtscsi_queuecommand to queue #1
last_processor_id == 0? no
spin_lock
tgt->req_vq = queue #0
spin_unlock
virtscsi_queuecommand to queue #0
This is not a network driver, there are still a lot of locks around.
This micro-optimization doesn't pay enough for the pain.
> Also - some kind of comment explaining why a similar race can not happen
> with this lock in place would be nice: I see why this specific race can
> not trigger but since lock is dropped later before you submit command, I
> have hard time convincing myself what exactly gurantees that vq is never
> switched before or even while command is submitted.
Because tgt->reqs will never become zero (which is a necessary condition
for tgt->req_vq to change), as long as one request is executing
virtscsi_queuecommand.
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 13:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <20120904124800.GE9805@redhat.com>
Il 04/09/2012 14:48, Michael S. Tsirkin ha scritto:
>> > This patch adds queue steering to virtio-scsi. When a target is sent
>> > multiple requests, we always drive them to the same queue so that FIFO
>> > processing order is kept. However, if a target was idle, we can choose
>> > a queue arbitrarily. In this case the queue is chosen according to the
>> > current VCPU, so the driver expects the number of request queues to be
>> > equal to the number of VCPUs. This makes it easy and fast to select
>> > the 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).
>> >
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> I guess an alternative is a per-target vq.
> Is the reason you avoid this that you expect more targets
> than cpus? If yes this is something you might want to
> mention in the log.
One reason is that, even though in practice I expect roughly the same
number of targets and VCPUs, hotplug means the number of targets is
difficult to predict and is usually fixed to 256.
The other reason is that per-target vq didn't give any performance
advantage. The bonus comes from cache locality and less process
migrations, more than from the independent virtqueues.
Paolo
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-04 13:50 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah
In-Reply-To: <CAJK669ZvnnG6fGOog-Z47B7xqyn9hcx5dAD7S--+2PSgE+hrLA@mail.gmail.com>
On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> >> If an architecture do not support DMA you will get
> >> a link error: "unknown symbol" for dma_alloc_coherent.
> >
> > Yes, it even seems intentional.
> > But I have a question: can the device work without DMA?
>
> The main dependency is actually on the dma-allocation.
> In my case I do dma_declare_coherent_memory() with
> the memory area shared with the remote device as argument.
> Subsequent calls to dma_alloc_coherent() will then allocate
> from this memory area.
>
> > Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
>
> Yes.dma_alloc_coherent to work.
>
> > If yes you should just fail load.
>
> Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
> VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
>
> > Also let's add a wrapper at top of file so ifdefs
> > do not litter the code like this. For example:
> >
> > #ifdef CONFIG_HAS_DMA
> > #define VIRTIO_CONSOLE_HAS_DMA (1)
> > #else
> > #define VIRTIO_CONSOLE_HAS_DMA (0)
> > #endif
>
> OK, good point. Perhaps we could even exploit gcc's
> ability to remove dead code and do something like this:
>
> if (VIRTIO_CONSOLE_HAS_DMA &&
> virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> ...
>
> This does not give any linker warnings when compiling for UML (no DMA).
>
> Regards,
> Sjur
Exactly. Though if we just fail load it will be much less code.
Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio: Don't access device data after unregistration.
From: Michael S. Tsirkin @ 2012-09-04 14:13 UTC (permalink / raw)
To: Sjur Brændeland; +Cc: linux-kernel, Guzman Lugo, Fernadndo, virtualization
In-Reply-To: <CAJK669ZVHOZVEw+1UeLU5KyxZexGfWC0VPnrK5XZd9x935PznQ@mail.gmail.com>
On Tue, Sep 04, 2012 at 02:12:33PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> >> >> Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set.
> >> >
> >> > What's the root cause of the panic?
> >>
> >> I believe the cause of the panic is calling
> >> ida_simple_remove(&virtio_index_ida, dev->index);
> >> when the dev structure is "poisoned" after kfree.
> >> It might be the "BUG_ON((int)id < 0)" that bites...
> >>
> >> >> Use device_del() and put_device() instead of
> >> >> device_unregister(), and access device data before
> >> >> calling put_device().
> >>
> >> > Why does this help? Does device_unregister free the
> >> > device so dev->index access crashes?
> >>
> >> Yes, if device ref-count is one when calling unregister
> >> the device is freed.
> >
> > Interesting. Where exactly?...
>
> I was wrong here, the reason is not related to ref-count being
> above one. The reason this issue do not show up in virtio_pci
> is that the release function is a dummy:
>
> [snip]
> static void virtio_pci_release_dev(struct device *_d)
> {
> /*
> * No need for a release method as we allocate/free
> * all devices together with the pci devices.
> * Provide an empty one to avoid getting a warning from core.
> */
> }
>
> The device structure uses a kref for reference counting the device.
> In virtio_pci() the release function virtio_pci_release_dev()
> will be called when the device is unregistered, but because the
> release function is dummy, data isn't freed or reset at this point.
> So for virtio devices created from virtio_pci my patch is not
> currently needed.
>
> However, empty release functions are not the preferred way, e.g look at
> https://lkml.org/lkml/2012/4/3/301
>
> [Greg K.H:]
> > > > > +static void hsi_port_release(struct device *dev __maybe_unused)
> > > > > +{
> > > > > +}
> > > >
> > > > As per the documentation in the kernel tree, I get to mock you
> > > > mercilessly for doing something as foolish as this. You are not smarter
> > > > than the kernel and don't think that you got rid of the kernel warning
> > > > properly by doing this. Do you think that I wrote that code for no good
> > > > reason? The kernel was being nice and telling you what you did wrong,
> > > > don't try to fake it out, it's smarter than you are here.
>
> But remoteproc frees the device memory in the release function
> rproc_vdev_release() and needs this patch.
>
> Regards,
> Sjur
Okay, so let's add a comment in virtio in unregister
function. Also slightly preferable to just use device_unregister
IMHO. Something like the below?
/*
device_unregister drops reference to device so put_device could
invoke release callback. In case that callback will free the device,
make sure we don't access device after this call.
*/
int index = dev->index;
device_unregister(&dev->dev);
ida_simple_remove(&virtio_index_ida, index);
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 14:19 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <50460615.3000006@redhat.com>
On Tue, Sep 04, 2012 at 03:45:57PM +0200, Paolo Bonzini wrote:
> > Also - some kind of comment explaining why a similar race can not happen
> > with this lock in place would be nice: I see why this specific race can
> > not trigger but since lock is dropped later before you submit command, I
> > have hard time convincing myself what exactly gurantees that vq is never
> > switched before or even while command is submitted.
>
> Because tgt->reqs will never become zero (which is a necessary condition
> for tgt->req_vq to change), as long as one request is executing
> virtscsi_queuecommand.
>
> Paolo
Yes but this logic would apparently imply the lock is not necessary, and
it actually is. I am not saying anything is wrong just that it
looks scary.
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 14:21 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <504606F6.4080603@redhat.com>
On Tue, Sep 04, 2012 at 03:49:42PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 14:48, Michael S. Tsirkin ha scritto:
> >> > This patch adds queue steering to virtio-scsi. When a target is sent
> >> > multiple requests, we always drive them to the same queue so that FIFO
> >> > processing order is kept. However, if a target was idle, we can choose
> >> > a queue arbitrarily. In this case the queue is chosen according to the
> >> > current VCPU, so the driver expects the number of request queues to be
> >> > equal to the number of VCPUs. This makes it easy and fast to select
> >> > the 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).
> >> >
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > I guess an alternative is a per-target vq.
> > Is the reason you avoid this that you expect more targets
> > than cpus? If yes this is something you might want to
> > mention in the log.
>
> One reason is that, even though in practice I expect roughly the same
> number of targets and VCPUs, hotplug means the number of targets is
> difficult to predict and is usually fixed to 256.
>
> The other reason is that per-target vq didn't give any performance
> advantage. The bonus comes from cache locality and less process
> migrations, more than from the independent virtqueues.
>
> Paolo
Okay, and why is per-target worse for cache locality?
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 14:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-scsi, kvm, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <20120904141924.GK9805@redhat.com>
Il 04/09/2012 16:19, Michael S. Tsirkin ha scritto:
> > > Also - some kind of comment explaining why a similar race can not happen
> > > with this lock in place would be nice: I see why this specific race can
> > > not trigger but since lock is dropped later before you submit command, I
> > > have hard time convincing myself what exactly gurantees that vq is never
> > > switched before or even while command is submitted.
> >
> > Because tgt->reqs will never become zero (which is a necessary condition
> > for tgt->req_vq to change), as long as one request is executing
> > virtscsi_queuecommand.
>
> Yes but this logic would apparently imply the lock is not necessary, and
> it actually is. I am not saying anything is wrong just that it
> looks scary.
Ok, I get the misunderstanding. For the logic to hold, you need a
serialization point after which tgt->req_vq is not changed. The lock
provides one such serialization point: after you unlock tgt->tgt_lock,
nothing else will change tgt->req_vq until your request completes.
Without the lock, there could always be a thread that is in the "then"
branch but has been scheduled out, and when rescheduled it will change
tgt->req_vq.
Perhaps the confusion comes from the atomic_inc_return, and that was
what my "why is this atomic" wanted to clear. **tgt->reqs is only
atomic to avoid taking a spinlock in the ISR**. If you read the code
with the lock, but with tgt->reqs as a regular non-atomic int, it should
be much easier to reason on the code. I can split the patch if needed.
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 14:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <20120904142154.GL9805@redhat.com>
Il 04/09/2012 16:21, Michael S. Tsirkin ha scritto:
> > One reason is that, even though in practice I expect roughly the same
> > number of targets and VCPUs, hotplug means the number of targets is
> > difficult to predict and is usually fixed to 256.
> >
> > The other reason is that per-target vq didn't give any performance
> > advantage. The bonus comes from cache locality and less process
> > migrations, more than from the independent virtqueues.
>
> Okay, and why is per-target worse for cache locality?
Because per-target doesn't have IRQ affinity for a particular CPU.
Assuming that the thread that is sending requests to the device is
I/O-bound, it is likely to be sleeping at the time the ISR is executed,
and thus executing the ISR on the same processor that sent the requests
is cheap.
But if you have many such I/O-bound processes, the kernel will execute
the ISR on a random processor, rather than the one that is sending
requests to the device.
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 14:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <5046108B.8030609@redhat.com>
On Tue, Sep 04, 2012 at 04:30:35PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 16:21, Michael S. Tsirkin ha scritto:
> > > One reason is that, even though in practice I expect roughly the same
> > > number of targets and VCPUs, hotplug means the number of targets is
> > > difficult to predict and is usually fixed to 256.
> > >
> > > The other reason is that per-target vq didn't give any performance
> > > advantage. The bonus comes from cache locality and less process
> > > migrations, more than from the independent virtqueues.
> >
> > Okay, and why is per-target worse for cache locality?
>
> Because per-target doesn't have IRQ affinity for a particular CPU.
>
> Assuming that the thread that is sending requests to the device is
> I/O-bound, it is likely to be sleeping at the time the ISR is executed,
> and thus executing the ISR on the same processor that sent the requests
> is cheap.
>
> But if you have many such I/O-bound processes, the kernel will execute
> the ISR on a random processor, rather than the one that is sending
> requests to the device.
>
> Paolo
I see, another case where our irq balancing makes bad decisions.
You could do it differently - pin irq to the cpu of the last task that
executed, tweak irq affinity when that changes.
Still if you want to support 256 targets vector per target
is not going to work.
Would be nice to add this motivation to commit log I think.
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 14:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <1346154857-12487-6-git-send-email-pbonzini@redhat.com>
On Tue, Aug 28, 2012 at 01:54:17PM +0200, Paolo Bonzini wrote:
> @@ -575,15 +630,19 @@ static struct scsi_host_template virtscsi_host_template = {
> &__val, sizeof(__val)); \
> })
>
> +
Pls don't add empty lines.
> static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
> - struct virtqueue *vq)
> + struct virtqueue *vq, bool affinity)
> {
> spin_lock_init(&virtscsi_vq->vq_lock);
> virtscsi_vq->vq = vq;
> + if (affinity)
> + virtqueue_set_affinity(vq, virtqueue_get_queue_index(vq) -
> + VIRTIO_SCSI_VQ_BASE);
> }
>
This means in practice if you have less virtqueues than CPUs,
things are not going to work well, will they?
Any idea what to do?
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 14:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <20120904144754.GO9805@redhat.com>
Il 04/09/2012 16:47, Michael S. Tsirkin ha scritto:
>> > static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
>> > - struct virtqueue *vq)
>> > + struct virtqueue *vq, bool affinity)
>> > {
>> > spin_lock_init(&virtscsi_vq->vq_lock);
>> > virtscsi_vq->vq = vq;
>> > + if (affinity)
>> > + virtqueue_set_affinity(vq, virtqueue_get_queue_index(vq) -
>> > + VIRTIO_SCSI_VQ_BASE);
>> > }
>> >
> This means in practice if you have less virtqueues than CPUs,
> things are not going to work well, will they?
Not particularly. It could be better or worse than single queue
depending on the workload.
> Any idea what to do?
Two possibilities:
1) Add a stride argument to virtqueue_set_affinity, and make it equal to
the number of queues.
2) Make multiqueue the default in QEMU, and make the default number of
queues equal to the number of VCPUs.
I was going for (2).
Paolo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox