Michael S. Tsirkin
2021-Oct-24 15:49 UTC
[PATCH] virtio_blk: allow 0 as num_request_queues
On Sun, Oct 24, 2021 at 06:29:59PM +0300, Max Gurtovoy wrote:> > 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 at nvidia.com> > > > > Signed-off-by: Michael S. Tsirkin <mst at 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.> > > > > > MODULE_PARM_DESC(num_request_queues, > > > > "Limit the number of request queues to use for blk device. " > > > > "0 for no limit. "