Richard W.M. Jones
2017-Aug-10 16:40 UTC
[PATCH 0/2] virtio_scsi: Set can_queue based on size of virtqueue.
Earlier discussion: https://lkml.org/lkml/2017/8/4/601 "Increased memory usage with scsi-mq" Downstream bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1478201
Richard W.M. Jones
2017-Aug-10 16:40 UTC
[PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
If using indirect descriptors, you can make the total_sg as large as you want. If not, BUG is too serious because the function later returns -ENOSPC. Thanks Paolo Bonzini, Christoph Hellwig. Signed-off-by: Richard W.M. Jones <rjones at redhat.com> --- drivers/virtio/virtio_ring.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 5e1b548828e6..27cbc1eab868 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq, } #endif - BUG_ON(total_sg > vq->vring.num); BUG_ON(total_sg == 0); head = vq->free_head; @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq, * buffers, then go indirect. FIXME: tune this threshold */ if (vq->indirect && total_sg > 1 && vq->vq.num_free) desc = alloc_indirect(_vq, total_sg, gfp); - else + else { desc = NULL; + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect); + } if (desc) { /* Use a single buffer which doesn't continue */ -- 2.13.1
Richard W.M. Jones
2017-Aug-10 16:40 UTC
[PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.
Since switching to blk-mq as the default in commit 5c279bd9e406 ("scsi: default to scsi-mq"), virtio-scsi LUNs consume about 10x as much kernel memory. qemu currently allocates a fixed 128 entry virtqueue. can_queue currently is set to 1024. But with indirect descriptors, each command in the queue takes 1 virtqueue entry, so the number of commands which can be queued is equal to the length of the virtqueue. Note I intend to send a patch to qemu to allow the virtqueue size to be configured from the qemu command line. Thanks Paolo Bonzini, Christoph Hellwig. Signed-off-by: Richard W.M. Jones <rjones at redhat.com> --- drivers/scsi/virtio_scsi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 9be211d68b15..d6b4ff634c0d 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev) if (err) goto virtscsi_init_failed; + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq); + cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1; shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue); shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0xFFFF; -- 2.13.1
Paolo Bonzini
2017-Aug-10 16:46 UTC
[PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.
On 10/08/2017 18:40, Richard W.M. Jones wrote:> Since switching to blk-mq as the default in commit 5c279bd9e406 > ("scsi: default to scsi-mq"), virtio-scsi LUNs consume about 10x as > much kernel memory. > > qemu currently allocates a fixed 128 entry virtqueue. can_queue > currently is set to 1024. But with indirect descriptors, each command > in the queue takes 1 virtqueue entry, so the number of commands which > can be queued is equal to the length of the virtqueue. > > Note I intend to send a patch to qemu to allow the virtqueue size to > be configured from the qemu command line.You can clear .can_queue from the templates. Otherwise looks good. Paolo> Thanks Paolo Bonzini, Christoph Hellwig. > > Signed-off-by: Richard W.M. Jones <rjones at redhat.com> > --- > drivers/scsi/virtio_scsi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 9be211d68b15..d6b4ff634c0d 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev) > if (err) > goto virtscsi_init_failed; > > + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq); > + > cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1; > shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue); > shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0xFFFF; >
Paolo Bonzini
2017-Aug-10 16:47 UTC
[PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
On 10/08/2017 18:40, Richard W.M. Jones wrote:> If using indirect descriptors, you can make the total_sg as large as > you want. If not, BUG is too serious because the function later > returns -ENOSPC. > > Thanks Paolo Bonzini, Christoph Hellwig. > > Signed-off-by: Richard W.M. Jones <rjones at redhat.com> > --- > drivers/virtio/virtio_ring.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 5e1b548828e6..27cbc1eab868 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq, > } > #endif > > - BUG_ON(total_sg > vq->vring.num); > BUG_ON(total_sg == 0); > > head = vq->free_head; > @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq, > * buffers, then go indirect. FIXME: tune this threshold */ > if (vq->indirect && total_sg > 1 && vq->vq.num_free) > desc = alloc_indirect(_vq, total_sg, gfp); > - else > + else { > desc = NULL; > + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);So we get here only if vq->vq.num_free is zero. In that case, vq->indirect makes no difference for the appropriateness of the warning (that is, it should never be issued for indirect descriptors).> + } > > if (desc) { > /* Use a single buffer which doesn't continue */ >Reviewed-by: Paolo Bonzini <pbonzini at redhat.com> Paolo
Michael S. Tsirkin
2017-Aug-10 21:21 UTC
[PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
On Thu, Aug 10, 2017 at 05:40:34PM +0100, Richard W.M. Jones wrote:> If using indirect descriptors, you can make the total_sg as large as > you want.That would be a spec violation though, even if it happens to work on current QEMU. The spec says: A driver MUST NOT create a descriptor chain longer than the Queue Size of the device. What prompted this patch? Do we ever encounter this situation?> If not, BUG is too serious because the function later > returns -ENOSPC. > > Thanks Paolo Bonzini, Christoph Hellwig. > > Signed-off-by: Richard W.M. Jones <rjones at redhat.com> > --- > drivers/virtio/virtio_ring.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 5e1b548828e6..27cbc1eab868 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq, > } > #endif > > - BUG_ON(total_sg > vq->vring.num); > BUG_ON(total_sg == 0); > > head = vq->free_head; > @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq, > * buffers, then go indirect. FIXME: tune this threshold */ > if (vq->indirect && total_sg > 1 && vq->vq.num_free) > desc = alloc_indirect(_vq, total_sg, gfp); > - else > + else { > desc = NULL; > + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect); > + } > > if (desc) { > /* Use a single buffer which doesn't continue */ > -- > 2.13.1
Apparently Analagous Threads
- [PATCH 0/2] virtio_scsi: Set can_queue based on size of virtqueue.
- [PATCH v2 0/2] virtio_scsi: Set can_queue based on size of virtqueue.
- [PATCH v2 0/2] virtio_scsi: Set can_queue based on size of virtqueue.
- [PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.
- [PATCH V6 0/5] virtio-scsi multiqueue