Re: [PATCH v2 1/1] virtio-blk: add num_io_queues module parameter

(off-list ancestor, not in this archive)

On Thu, Sep 09, 2021 at 07:45:42PM +0300, Max Gurtovoy wrote:
On 9/9/2021 7:31 PM, Michael S. Tsirkin wrote:
On Thu, Sep 09, 2021 at 06:51:56PM +0300, Max Gurtovoy wrote:
On 9/9/2021 6:40 PM, Michael S. Tsirkin wrote:
On Thu, Sep 09, 2021 at 06:37:37PM +0300, Max Gurtovoy wrote:
On 9/9/2021 4:42 PM, Michael S. Tsirkin wrote:
On Mon, Sep 06, 2021 at 02:59:40PM +0300, Max Gurtovoy wrote:
On 9/6/2021 2:20 PM, Michael S. Tsirkin wrote:
On Mon, Sep 06, 2021 at 01:31:32AM +0300, Max Gurtovoy wrote:
On 9/5/2021 7:02 PM, Michael S. Tsirkin wrote:
On Thu, Sep 02, 2021 at 02:45:52PM +0100, Stefan Hajnoczi wrote:
On Tue, Aug 31, 2021 at 04:50:35PM +0300, Max Gurtovoy wrote:
Sometimes a user would like to control the amount of IO queues to be
created for a block device. For example, for limiting the memory
footprint of virtio-blk devices.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---

changes from v1:
      - use param_set_uint_minmax (from Christoph)
      - added "Should > 0" to module description

Note: This commit apply on top of Jens's branch for-5.15/drivers
---
      drivers/block/virtio_blk.c | 20 +++++++++++++++++++-
      1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4b49df2dfd23..9332fc4e9b31 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -24,6 +24,22 @@
      /* The maximum number of sg elements that fit into a virtqueue */
      #define VIRTIO_BLK_MAX_SG_ELEMS 32768
+static int virtblk_queue_count_set(const char *val,
+		const struct kernel_param *kp)
+{
+	return param_set_uint_minmax(val, kp, 1, nr_cpu_ids);
+}
Hmm which tree is this for?
I've mentioned in the note that it apply on branch for-5.15/drivers. But now
you can apply it on linus/master as well.


+
+static const struct kernel_param_ops queue_count_ops = {
+	.set = virtblk_queue_count_set,
+	.get = param_get_uint,
+};
+
+static unsigned int num_io_queues;
+module_param_cb(num_io_queues, &queue_count_ops, &num_io_queues, 0644);
+MODULE_PARM_DESC(num_io_queues,
+		 "Number of IO virt queues to use for blk device. Should > 0");
better:

+MODULE_PARM_DESC(num_io_request_queues,
+                "Limit number of IO request virt queues to use for each device. 0 for now limit");
You proposed it and I replied on it bellow.
Can't say I understand 100% what you are saying. Want to send
a description that does make sense to you but
also reflects reality? 0 is the default so
"should > 0" besides being ungrammatical does not seem t"
reflect what it does ...
if you "modprobe virtio_blk" the previous behavior will happen.

You can't "modprobe virtio_blk num_io_request_queues=0" since the minimal
value is 1.

So your description is not reflecting the code.

We can do:

MODULE_PARM_DESC(num_io_request_queues, "Number of request virt queues to use for blk device. Minimum value is 1 queue");
What's the default value? We should document that.
default value for static global variables is 0.

MODULE_PARM_DESC(num_io_request_queues, "Number of request virt queues to
use for blk device. Minimum value is 1 queue. Default and Maximum value is
equal to the total number of CPUs");
So it says it's the # of cpus but yoiu inspect it with
sysfs and it's actually 0. Let's say that's confusing
at the least. why not just let users set it to 0
and document that?
Setting it by the user to 0 makes no sense.

We can say "if not set, the value equals to the total number of CPUs".
the value is 0. it seems to mean "no limit". the actual # of queues is
then te smaller between # of cpus and # of hardware queues.
I see no reason not to allow user to set that especially if
it was originally the value then user changed it
and is trying to change it back.

The actual value of the created queues can be seen in /sys/block/vda/mq/ as
done today.

+
      static int major;
      static DEFINE_IDA(vd_index_ida);
@@ -501,7 +517,9 @@ static int init_vq(struct virtio_blk *vblk)
      	if (err)
      		num_vqs = 1;
-	num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
+	num_vqs = min_t(unsigned int,
+			min_not_zero(num_io_queues, nr_cpu_ids),
+			num_vqs);
If you respin, please consider calling them request queues. That's the
terminology from the VIRTIO spec and it's nice to keep it consistent.
But the purpose of num_io_queues is clear, so:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
I did this:
+static unsigned int num_io_request_queues;
+module_param_cb(num_io_request_queues, &queue_count_ops, &num_io_request_queues, 0644);
+MODULE_PARM_DESC(num_io_request_queues,
+                "Limit number of IO request virt queues to use for each device. 0 for now limit");
The parameter is writable and can be changed and then new devices might be
probed with new value.

It can't be zero in the code. we can change param_set_uint_minmax args and
say that 0 says nr_cpus.

I'm ok with the renaming but I prefer to stick to the description we gave in
V3 of this patch (and maybe enable value of 0 as mentioned above).
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help