Re: [PATCH] virtio_blk: allow 0 as num_request_queues
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2021-10-24 15:49:10
Also in:
linux-block, lkml
On Sun, Oct 24, 2021 at 06:29:59PM +0300, Max Gurtovoy wrote:
On 10/24/2021 6:11 PM, Michael S. Tsirkin wrote:quoted
On Sun, Oct 24, 2021 at 05:19:33PM +0300, Max Gurtovoy wrote:quoted
On 10/24/2021 4:54 PM, Michael S. Tsirkin wrote:quoted
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).
That doesn't avoid code duplication - the limit of nr_cpu_ids is applied twice.
quoted
quoted
quoted
MODULE_PARM_DESC(num_request_queues, "Limit the number of request queues to use for blk device. " "0 for no limit. "
_______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization