[PATCH 0/2] virtio_scsi: Set can_queue based on size of virtqueue.

[PATCH 0/2] virtio_scsi: Set can_queue based on size of virtqueue.

[Richard W.M. Jones]

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

[PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Richard W.M. Jones]

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@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

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Paolo Bonzini]

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@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@redhat.com>

Paolo

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

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@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

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Richard W.M. Jones]

On Fri, Aug 11, 2017 at 12:21:16AM +0300, Michael S. Tsirkin wrote:
> 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?

This patch is needed because the following (2/2) patch will trigger
that BUG_ON if I set virtqueue_size=64 or any smaller value.

The precise backtrace is below.

Rich.

[    4.029510] ------------[ cut here ]------------
[    4.030127] kernel BUG at drivers/virtio/virtio_ring.c:299!
[    4.030834] invalid opcode: 0000 [#1] SMP
[    4.031340] Modules linked in: libcrc32c crc8 crc7 crc4 crc_itu_t virtio_pci virtio_mmio virtio_input virtio_balloon virtio_scsi nd_pmem nd_btt virtio_net virtio_crypto crypto_engine virtio_console virtio_rng virtio_blk virtio_ring virtio nfit crc32_generic crct10dif_pclmul crc32c_intel crc32_pclmul
[    4.034606] CPU: 0 PID: 1 Comm: init Not tainted 4.13.0-rc4+ #100
[    4.035354] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
[    4.036770] task: ffff9a859e243300 task.stack: ffffa731c00cc000
[    4.037506] RIP: 0010:virtqueue_add_sgs+0x23d/0x460 [virtio_ring]
[    4.038250] RSP: 0000:ffffa731c00cf6e0 EFLAGS: 00010097
[    4.038898] RAX: 0000000000000000 RBX: 0000000000000011 RCX: ffffdd0680646c40
[    4.039762] RDX: 0000000000000000 RSI: ffffa731c00cf7b8 RDI: ffff9a85945c4d68
[    4.040634] RBP: ffffa731c00cf748 R08: ffff9a85945c4a78 R09: 0000000001080020
[    4.041508] R10: ffffa731c00cf788 R11: ffff9a859b3d3120 R12: ffffa731c00cf7d0
[    4.042382] R13: ffffa731c00cf7d0 R14: 0000000000000001 R15: ffff9a859b3f8200
[    4.043248] FS:  0000000000000000(0000) GS:ffff9a859e600000(0000) knlGS:0000000000000000
[    4.044232] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    4.044942] CR2: 00007fcff02e931c CR3: 000000001d23b000 CR4: 00000000003406f0
[    4.045815] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    4.046684] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    4.047559] Call Trace:
[    4.047876]  virtscsi_add_cmd+0x1c9/0x280 [virtio_scsi]
[    4.048528]  virtscsi_kick_cmd+0x38/0x90 [virtio_scsi]
[    4.049161]  virtscsi_queuecommand+0x104/0x280 [virtio_scsi]
[    4.049875]  virtscsi_queuecommand_single+0x38/0x40 [virtio_scsi]
[    4.050628]  scsi_dispatch_cmd+0xf9/0x390
[    4.051128]  scsi_queue_rq+0x5e5/0x6f0
[    4.051602]  blk_mq_dispatch_rq_list+0x1ff/0x3c0
[    4.052175]  blk_mq_sched_dispatch_requests+0x199/0x1f0
[    4.052824]  __blk_mq_run_hw_queue+0x11d/0x1b0
[    4.053382]  __blk_mq_delay_run_hw_queue+0x8d/0xa0
[    4.053972]  blk_mq_run_hw_queue+0x14/0x20
[    4.054485]  blk_mq_sched_insert_requests+0x96/0x120
[    4.055095]  blk_mq_flush_plug_list+0x19d/0x410
[    4.055661]  blk_flush_plug_list+0xf9/0x2b0
[    4.056182]  blk_finish_plug+0x2c/0x40
[    4.056655]  __do_page_cache_readahead+0x32e/0x400
[    4.057261]  filemap_fault+0x2fb/0x890
[    4.057731]  ? filemap_fault+0x2fb/0x890
[    4.058220]  ? find_held_lock+0x3c/0xb0
[    4.058714]  ext4_filemap_fault+0x34/0x50
[    4.059212]  __do_fault+0x1e/0x110
[    4.059644]  __handle_mm_fault+0x6b2/0x1080
[    4.060167]  handle_mm_fault+0x178/0x350
[    4.060662]  __do_page_fault+0x26e/0x510
[    4.061152]  trace_do_page_fault+0x9d/0x290
[    4.061677]  do_async_page_fault+0x51/0xa0
[    4.062189]  async_page_fault+0x28/0x30
[    4.062667] RIP: 0033:0x7fcff030a24f
[    4.063113] RSP: 002b:00007ffefc2ad078 EFLAGS: 00010206
[    4.063760] RAX: 00007fcff02e931c RBX: 00007fcff050f660 RCX: 00007fcff02e935c
[    4.064648] RDX: 0000000000000664 RSI: 0000000000000000 RDI: 00007fcff02e931c
[    4.065519] RBP: 00007ffefc2ad320 R08: 00007fcff02e931c R09: 0000000000027000
[    4.066392] R10: 00007fcff02e9980 R11: 0000000000000206 R12: 00007ffefc2ad0b0
[    4.067263] R13: 00007ffefc2ad408 R14: 0000000000000002 R15: 00000000000087f0
[    4.068135] Code: af 01 c7 45 c8 01 00 00 00 45 31 ed e9 9b fe ff ff 31 db 48 83 7d d0 00 0f 85 3f fe ff ff 0f 0b 48 8b 7d b8 e8 e5 fd 22 de eb 8b <0f> 0b 0f 0b 44 89 6d a8 45 89 f5 48 8b 45 a0 44 8d 70 01 48 83 
[    4.070506] RIP: virtqueue_add_sgs+0x23d/0x460 [virtio_ring] RSP: ffffa731c00cf6e0
[    4.071434] ---[ end trace 02532659840e2a64 ]---


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Thu, Aug 10, 2017 at 10:30:38PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 11, 2017 at 12:21:16AM +0300, Michael S. Tsirkin wrote:
> > 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?
> 
> This patch is needed because the following (2/2) patch will trigger
> that BUG_ON if I set virtqueue_size=64 or any smaller value.
> 
> The precise backtrace is below.
> 
> Rich.
> 
> [    4.029510] ------------[ cut here ]------------
> [    4.030127] kernel BUG at drivers/virtio/virtio_ring.c:299!
> [    4.030834] invalid opcode: 0000 [#1] SMP
> [    4.031340] Modules linked in: libcrc32c crc8 crc7 crc4 crc_itu_t virtio_pci virtio_mmio virtio_input virtio_balloon virtio_scsi nd_pmem nd_btt virtio_net virtio_crypto crypto_engine virtio_console virtio_rng virtio_blk virtio_ring virtio nfit crc32_generic crct10dif_pclmul crc32c_intel crc32_pclmul
> [    4.034606] CPU: 0 PID: 1 Comm: init Not tainted 4.13.0-rc4+ #100
> [    4.035354] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> [    4.036770] task: ffff9a859e243300 task.stack: ffffa731c00cc000
> [    4.037506] RIP: 0010:virtqueue_add_sgs+0x23d/0x460 [virtio_ring]
> [    4.038250] RSP: 0000:ffffa731c00cf6e0 EFLAGS: 00010097
> [    4.038898] RAX: 0000000000000000 RBX: 0000000000000011 RCX: ffffdd0680646c40
> [    4.039762] RDX: 0000000000000000 RSI: ffffa731c00cf7b8 RDI: ffff9a85945c4d68
> [    4.040634] RBP: ffffa731c00cf748 R08: ffff9a85945c4a78 R09: 0000000001080020
> [    4.041508] R10: ffffa731c00cf788 R11: ffff9a859b3d3120 R12: ffffa731c00cf7d0
> [    4.042382] R13: ffffa731c00cf7d0 R14: 0000000000000001 R15: ffff9a859b3f8200
> [    4.043248] FS:  0000000000000000(0000) GS:ffff9a859e600000(0000) knlGS:0000000000000000
> [    4.044232] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    4.044942] CR2: 00007fcff02e931c CR3: 000000001d23b000 CR4: 00000000003406f0
> [    4.045815] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    4.046684] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [    4.047559] Call Trace:
> [    4.047876]  virtscsi_add_cmd+0x1c9/0x280 [virtio_scsi]
> [    4.048528]  virtscsi_kick_cmd+0x38/0x90 [virtio_scsi]
> [    4.049161]  virtscsi_queuecommand+0x104/0x280 [virtio_scsi]
> [    4.049875]  virtscsi_queuecommand_single+0x38/0x40 [virtio_scsi]
> [    4.050628]  scsi_dispatch_cmd+0xf9/0x390
> [    4.051128]  scsi_queue_rq+0x5e5/0x6f0
> [    4.051602]  blk_mq_dispatch_rq_list+0x1ff/0x3c0
> [    4.052175]  blk_mq_sched_dispatch_requests+0x199/0x1f0
> [    4.052824]  __blk_mq_run_hw_queue+0x11d/0x1b0
> [    4.053382]  __blk_mq_delay_run_hw_queue+0x8d/0xa0
> [    4.053972]  blk_mq_run_hw_queue+0x14/0x20
> [    4.054485]  blk_mq_sched_insert_requests+0x96/0x120
> [    4.055095]  blk_mq_flush_plug_list+0x19d/0x410
> [    4.055661]  blk_flush_plug_list+0xf9/0x2b0
> [    4.056182]  blk_finish_plug+0x2c/0x40
> [    4.056655]  __do_page_cache_readahead+0x32e/0x400
> [    4.057261]  filemap_fault+0x2fb/0x890
> [    4.057731]  ? filemap_fault+0x2fb/0x890
> [    4.058220]  ? find_held_lock+0x3c/0xb0
> [    4.058714]  ext4_filemap_fault+0x34/0x50
> [    4.059212]  __do_fault+0x1e/0x110
> [    4.059644]  __handle_mm_fault+0x6b2/0x1080
> [    4.060167]  handle_mm_fault+0x178/0x350
> [    4.060662]  __do_page_fault+0x26e/0x510
> [    4.061152]  trace_do_page_fault+0x9d/0x290
> [    4.061677]  do_async_page_fault+0x51/0xa0
> [    4.062189]  async_page_fault+0x28/0x30
> [    4.062667] RIP: 0033:0x7fcff030a24f
> [    4.063113] RSP: 002b:00007ffefc2ad078 EFLAGS: 00010206
> [    4.063760] RAX: 00007fcff02e931c RBX: 00007fcff050f660 RCX: 00007fcff02e935c
> [    4.064648] RDX: 0000000000000664 RSI: 0000000000000000 RDI: 00007fcff02e931c
> [    4.065519] RBP: 00007ffefc2ad320 R08: 00007fcff02e931c R09: 0000000000027000
> [    4.066392] R10: 00007fcff02e9980 R11: 0000000000000206 R12: 00007ffefc2ad0b0
> [    4.067263] R13: 00007ffefc2ad408 R14: 0000000000000002 R15: 00000000000087f0
> [    4.068135] Code: af 01 c7 45 c8 01 00 00 00 45 31 ed e9 9b fe ff ff 31 db 48 83 7d d0 00 0f 85 3f fe ff ff 0f 0b 48 8b 7d b8 e8 e5 fd 22 de eb 8b <0f> 0b 0f 0b 44 89 6d a8 45 89 f5 48 8b 45 a0 44 8d 70 01 48 83 
> [    4.070506] RIP: virtqueue_add_sgs+0x23d/0x460 [virtio_ring] RSP: ffffa731c00cf6e0
> [    4.071434] ---[ end trace 02532659840e2a64 ]---

Then we probably should fail probe if vq size is too small.

> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Richard W.M. Jones]

On Fri, Aug 11, 2017 at 12:31:44AM +0300, Michael S. Tsirkin wrote:
> Then we probably should fail probe if vq size is too small.

What does this mean?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Thu, Aug 10, 2017 at 10:35:11PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 11, 2017 at 12:31:44AM +0300, Michael S. Tsirkin wrote:
> > Then we probably should fail probe if vq size is too small.
> 
> What does this mean?
> 
> Rich.

We must prevent driver from submitting s/g lists > vq size to device.


Either tell linux to avoid s/g lists that are too long, or
simply fail request if this happens, or refuse to attach driver to device.

Later option would look something like this within probe:

        for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
		if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
			goto err;


I don't know what's MAX_SG_USED_BY_LINUX though.

> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Richard W.M. Jones]

On Fri, Aug 11, 2017 at 12:41:47AM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 10, 2017 at 10:35:11PM +0100, Richard W.M. Jones wrote:
> > On Fri, Aug 11, 2017 at 12:31:44AM +0300, Michael S. Tsirkin wrote:
> > > Then we probably should fail probe if vq size is too small.
> > 
> > What does this mean?
> > 
> > Rich.
> 
> We must prevent driver from submitting s/g lists > vq size to device.

Fair point.  I would have thought (not knowing anything about how this
works in the kernel) that the driver should be able to split up
scatter-gather lists if they are larger than what the driver supports?

Anyway the commit message is derived from what Paolo told me on IRC,
so let's see what he says.

Rich.

> Either tell linux to avoid s/g lists that are too long, or
> simply fail request if this happens, or refuse to attach driver to device.
> 
> Later option would look something like this within probe:
> 
>         for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
> 		if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
> 			goto err;
> 
> 
> I don't know what's MAX_SG_USED_BY_LINUX though.
> 
> > -- 
> > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> > Read my programming and virtualization blog: http://rwmj.wordpress.com
> > virt-builder quickly builds VMs from scratch
> > http://libguestfs.org/virt-builder.1.html

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Paolo Bonzini]

On 10/08/2017 23:41, Michael S. Tsirkin wrote:
>>> Then we probably should fail probe if vq size is too small.
>> What does this mean?
> 
> We must prevent driver from submitting s/g lists > vq size to device.

What is the rationale for the limit?  It makes no sense if indirect
descriptors are available, especially because...

> Either tell linux to avoid s/g lists that are too long, or
> simply fail request if this happens, or refuse to attach driver to device.
> 
> Later option would look something like this within probe:
> 
>         for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
> 		if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
> 			goto err;
> 
> 
> I don't know what's MAX_SG_USED_BY_LINUX though.
> 

... both virtio-blk and virtio-scsi transmit their own value for the
maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk).

Paolo

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Fri, Aug 11, 2017 at 04:09:26PM +0200, Paolo Bonzini wrote:
> On 10/08/2017 23:41, Michael S. Tsirkin wrote:
> >>> Then we probably should fail probe if vq size is too small.
> >> What does this mean?
> > 
> > We must prevent driver from submitting s/g lists > vq size to device.
> 
> What is the rationale for the limit?

So the host knows what it needs to support.

>  It makes no sense if indirect
> descriptors are available, especially because...
> 
> > Either tell linux to avoid s/g lists that are too long, or
> > simply fail request if this happens, or refuse to attach driver to device.
> > 
> > Later option would look something like this within probe:
> > 
> >         for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
> > 		if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
> > 			goto err;
> > 
> > 
> > I don't know what's MAX_SG_USED_BY_LINUX though.
> > 
> 
> ... both virtio-blk and virtio-scsi transmit their own value for the
> maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk).
> 
> Paolo

No other device has it, and it seemed like a good idea to
limit it generally at the time.

we can fix the spec to relax the requirement for blk and scsi -
want to submit a proposal? Alternatively, add a generic field
for that.

For a quick fix, make sure vq size is >= max sg.

-- 
MST

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Paolo Bonzini]

On 11/08/2017 19:23, Michael S. Tsirkin wrote:
> On Fri, Aug 11, 2017 at 04:09:26PM +0200, Paolo Bonzini wrote:
>> On 10/08/2017 23:41, Michael S. Tsirkin wrote:
>>>>> Then we probably should fail probe if vq size is too small.
>>>> What does this mean?
>>>
>>> We must prevent driver from submitting s/g lists > vq size to device.
>>
>> What is the rationale for the limit?
> 
> So the host knows what it needs to support.
> 
>> both virtio-blk and virtio-scsi transmit their own value for the
>> maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk).
> 
> No other device has it, and it seemed like a good idea to
> limit it generally at the time.
> 
> we can fix the spec to relax the requirement for blk and scsi -
> want to submit a proposal? Alternatively, add a generic field
> for that.

Yes, I can submit a proposal.  blk and scsi are the ones that are most
likely to have very long sg lists.

When I was designing scsi I just copied that field from blk. :)

Paolo

[PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.

[Richard W.M. Jones]

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@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

Re: [PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.

[Paolo Bonzini]

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@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;
>