virtio: use QUEUE_FLAG_CLUSTER in virtio_blk

RFC PATCH In review Rusty Russell 3 participants 7 messages Nov 17, 2008 – Dec 3, 2008 2 messages not in the archive

Subsystems (3)

Block layer virtio core virtio block and SCSI drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rusty Russell

author

2008-11-17 06:52 UTC #2 lore raw

Quoted 8 lines · On Friday 14 November 2008 20:00:23 Jens Axboe wrote:

Queue clustering is on by default though when you allocate your queue, so I'm surprised you see a difference by doing:
(quoted diff) +2 -0
+	/* Gather adjacent buffers to minimize sg length. */
+	queue_flag_set(QUEUE_FLAG_CLUSTER, vblk->disk->queue);
did test_bit(QUEUE_FLAG_CLUSTER, &vblk->disk->queue->queue_flags) really return 0 before?

Apparently not. My results must be noise :(

However, it made me look harder at the behaviour of the driver. I have two other enhancement patches while I'll send now, but I still only see sgs of 116 elements.

Thanks for the clue donation,
Rusty.

Rusty Russell

author

2008-11-17 06:54 UTC #3 lore raw

PATCH 1/2 virtio: block: set max_segment_size and max_sectors to infinite.

Setting max_segment_size allows more than 64k per sg element, unless the host specified a limit. Setting max_sectors indicates that our max_hw_segments is the only limit.

Signed-off-by Rusty Russell <[email protected]>

drivers/block/virtio_blk.c +5 -0

diff -r 446171198334 drivers/block/virtio_blk.c
@@ -277,6 +277,9 @@
 	}
 	set_capacity(vblk->disk, cap);
 
+	/* No real sector limit. */
+	blk_queue_max_sectors(vblk->disk->queue, -1U);
+
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SIZE_MAX,

@@ -284,6 +287,8 @@
 				&v);
 	if (!err)
 		blk_queue_max_segment_size(vblk->disk->queue, v);
+	else
+		blk_queue_max_segment_size(vblk->disk->queue, -1UL);
 
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
 				offsetof(struct virtio_blk_config, seg_max),

Rusty Russell

author

2008-11-17 07:04 UTC #4 lore raw

PATCH 2/2 virtio: block: dynamic maximum segments

Enhance the driver to handle whatever maximum segment number the host tells us to handle. Do to this, we need to allocate the scatterlist dynamically.

We set max_phys_segments and max_hw_segments to the same value (1 if the host doesn't tell us, since that's safest and all known hosts do tell us).

Note that kmalloc'ing the structure for large sg_elems might be
problematic: the fix for this is sg_table, but that requires more work.

Signed-off-by Rusty Russell <[email protected]>

drivers/block/virtio_blk.c +23 ~~-13~~

diff -r 177219c4da07 drivers/block/virtio_blk.c
@@ -6,7 +6,6 @@
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
 
-#define VIRTIO_MAX_SG	(3+MAX_PHYS_SEGMENTS)
 #define PART_BITS 4
 
 static int major, index;

@@ -26,8 +25,11 @@
 
 	mempool_t *pool;
 
+	/* What host tells us, plus 2 for header & tailer. */
+	unsigned int sg_elems;
+
 	/* Scatterlist: can be too big for stack. */
-	struct scatterlist sg[VIRTIO_MAX_SG];
+	struct scatterlist sg[/*sg_elems*/];
 };
 
 struct virtblk_req

@@ -97,8 +99,6 @@
 	if (blk_barrier_rq(vbr->req))
 		vbr->out_hdr.type |= VIRTIO_BLK_T_BARRIER;
 
-	/* This init could be done at vblk creation time */
-	sg_init_table(vblk->sg, VIRTIO_MAX_SG);
 	sg_set_buf(&vblk->sg[0], &vbr->out_hdr, sizeof(vbr->out_hdr));
 	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);
 	sg_set_buf(&vblk->sg[num+1], &vbr->status, sizeof(vbr->status));

@@ -130,7 +130,7 @@
 
 	while ((req = elv_next_request(q)) != NULL) {
 		vblk = req->rq_disk->private_data;
-		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
+		BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
 
 		/* If this request fails, stop queue and wait for something to
 		   finish to restart it. */

@@ -196,12 +196,22 @@
 	int err;
 	u64 cap;
 	u32 v;
-	u32 blk_size;
+	u32 blk_size, sg_elems;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;
 
-	vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
+	/* We need to know how many segments before we allocate. */
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
+				offsetof(struct virtio_blk_config, seg_max),
+				&sg_elems);
+	if (err)
+		sg_elems = 1;
+
+	/* We need an extra sg elements at head and tail. */
+	sg_elems += 2;
+	vdev->priv = vblk = kmalloc(sizeof(*vblk) +
+				    sizeof(vblk->sg[0]) * sg_elems, GFP_KERNEL);
 	if (!vblk) {
 		err = -ENOMEM;
 		goto out;

@@ -210,6 +220,8 @@
 	INIT_LIST_HEAD(&vblk->reqs);
 	spin_lock_init(&vblk->lock);
 	vblk->vdev = vdev;
+	vblk->sg_elems = sg_elems;
+	sg_init_table(vblk->sg, vblk->sg_elems);
 
 	/* We expect one virtqueue, for output. */
 	vblk->vq = vdev->config->find_vq(vdev, 0, blk_done);

@@ -277,6 +289,10 @@
 	}
 	set_capacity(vblk->disk, cap);
 
+	/* We can handle whatever the host told us to handle. */
+	blk_queue_max_phys_segments(vblk->disk->queue, vblk->sg_elems-2);
+	blk_queue_max_hw_segments(vblk->disk->queue, vblk->sg_elems-2);
+
 	/* No real sector limit. */
 	blk_queue_max_sectors(vblk->disk->queue, -1U);

@@ -289,12 +305,6 @@
 		blk_queue_max_segment_size(vblk->disk->queue, v);
 	else
 		blk_queue_max_segment_size(vblk->disk->queue, -1UL);
-
-	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
-				offsetof(struct virtio_blk_config, seg_max),
-				&v);
-	if (!err)
-		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_SIZE,

Chris Wright

2008-11-26 17:42 UTC #5 lore raw

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

In reply to Rusty Russell

drivers/block/virtio_blk.c line 282

Quoted 3 lines · * Rusty Russell ([email protected]) wrote:

drivers/block/virtio_blk.c +3 -0

+	/* No real sector limit. */
+	blk_queue_max_sectors(vblk->disk->queue, -1U);
+

Is that actually legitimate? I think it'd still work out, but seems odd, e.g. all the spots that do:

	q->max_hw_sectors << 9

will just toss the upper bits...

thanks,
-chris

Rusty Russell

author

2008-11-27 13:00 UTC #6 lore raw

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

Quoted 11 lines · On Thursday 27 November 2008 04:12:25 Chris Wright wrote:

drivers/block/virtio_blk.c line 282
Quoted 3 lines · * Rusty Russell ([email protected]) wrote:
drivers/block/virtio_blk.c +3 -0
+	/* No real sector limit. */
+	blk_queue_max_sectors(vblk->disk->queue, -1U);
+
Is that actually legitimate? I think it'd still work out, but seems odd, e.g. all the spots that do:
	q->max_hw_sectors << 9
will just toss the upper bits...

I think this just indicates that *my driver* doesn't have an issue with large numbers of sectors. The block layer may well...

Rusty.

Quoted 3 lines

thanks,
-chris

Jens Axboe

2008-11-27 13:04 UTC #7 lore raw

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

Quoted 9 lines · Rusty Russell

Quoted 8 lines · On Thursday 27 November 2008 04:12:25 Chris Wright wrote:
drivers/block/virtio_blk.c line 282
Quoted 3 lines · * Rusty Russell ([email protected]) wrote:
drivers/block/virtio_blk.c +3 -0
+	/* No real sector limit. */
+	blk_queue_max_sectors(vblk->disk->queue, -1U);
+
Is that actually legitimate? I think it'd still work out, but seems odd, e.g. all the spots that do:

Quoted 6 lines

Quoted 3 lines
	q->max_hw_sectors << 9
will just toss the upper bits...
I think this just indicates that *my driver* doesn't have an issue with large numbers of sectors. The block layer may well...

We should probably just truncate insanely large values like -1 to something sane. For the above case we already do trunc it to 1MB, as it's the soft IO size. The segment size is already in bytes, so in fact everything already looks fine.

Signature · 1 line

Jens Axboe

Chris Wright

Acked-by on v1 1/2

2008-12-03 18:43 UTC #8 lore raw

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

In reply to Rusty Russell

Quoted 3 lines · * Rusty Russell ([email protected]) wrote:

Setting max_segment_size allows more than 64k per sg element, unless the host specified a limit. Setting max_sectors indicates that our max_hw_segments is the only limit.

We had been using a simple hardocded constant to increase max sectors to improve throughput. This (along with 2/2) are tested and showing nice numbers.

Acked-by Chris Wright <[email protected]>