Messages in this thread | | | Date | Sun, 24 Oct 2021 18:29:59 +0300 | Subject | Re: [PATCH] virtio_blk: allow 0 as num_request_queues | From | Max Gurtovoy <> |
| |
On 10/24/2021 6:11 PM, Michael S. Tsirkin wrote: > On Sun, Oct 24, 2021 at 05:19:33PM +0300, Max Gurtovoy wrote: >> On 10/24/2021 4:54 PM, Michael S. Tsirkin wrote: >>> The default value is 0 meaning "no limit". However if 0 >>> is specified on the command line it is instead silently >>> converted to 1. Further, the value is already validated >>> at point of use, there's no point in duplicating code >>> validating the value when it is set. >>> >>> Simplify the code while making the behaviour more consistent >>> by using plain module_param. >>> >>> Fixes: 1a662cf6cb9a ("virtio-blk: add num_request_queues module parameter") >>> Cc: Max Gurtovoy <mgurtovoy@nvidia.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> drivers/block/virtio_blk.c | 14 +------------- >>> 1 file changed, 1 insertion(+), 13 deletions(-) >>> >>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c >>> index 6318134aab76..c336d9bb9105 100644 >>> --- a/drivers/block/virtio_blk.c >>> +++ b/drivers/block/virtio_blk.c >>> @@ -30,20 +30,8 @@ >>> #define VIRTIO_BLK_INLINE_SG_CNT 2 >>> #endif >>> -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); >>> -} >>> - >>> -static const struct kernel_param_ops queue_count_ops = { >>> - .set = virtblk_queue_count_set, >>> - .get = param_get_uint, >>> -}; >>> - >>> static unsigned int num_request_queues; >>> -module_param_cb(num_request_queues, &queue_count_ops, &num_request_queues, >>> - 0644); >>> +module_param(num_request_queues, uint, 0644); >> Not the best solution. >> >> You can set the param to 1024 but in practice nr_cpu_ids can be 32 for >> example. > Well your patch does make it possible to know what nr_cpu_ids is. > But does it matter? The actual number of queues is still capped > by the num_queues of the device. And I'm concerned that some > userspace comes to depend on reading back nr_cpu_ids > from there, which will cement this as part of ABI instead of > being an implementation detail. > Exposing the actual number of queues in sysfs might make more sense ... > > Generally you suggested embedded as a use-case, and I don't really > see lots of embedded userspace poking at this parameter in sysfs. > > What I'd like to see, and attempted to achieve here: > - avoid code duplication > - consistency: some way to specify the parameter but still make it have the default value > > Better suggestions are welcome.
Just change return param_set_uint_minmax(val, kp, 1, nr_cpu_ids);
to
return param_set_uint_minmax(val, kp, *0*, nr_cpu_ids);
The real amount can be exposed by common sysfs.
We'll extend virtio_driver to have a new callback to return this value. If callback doesn't exist - return -1 (unknown value).
> >>> MODULE_PARM_DESC(num_request_queues, >>> "Limit the number of request queues to use for blk device. " >>> "0 for no limit. "
| |