Re: [PATCH] virtio_blk: allow 0 as num_request_queues

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 25, 2021 at 01:30:19AM +0300, Max Gurtovoy wrote:
> 
> On 10/24/2021 6:49 PM, Michael S. Tsirkin wrote:
> > 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@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.
> 
> It's a small logic duplication and not code duplication.
> 
> The param_set_uint_minmax is a new API to make sure that the value is in the
> limit you set it, and it will only called if the user explicitly set the
> module parameter.
> 
> In your case, you allow setting 0 value in the comment for the module
> parameter. And this is the oneline change I suggested above.
> 
> The second check in the code is for the case that the user didn't set the
> module parameter explicitly and we need to make sure we don't set num_queues
> to 0 (the default value).
> 
> So I'm ok with these 2 checks.
> 
> Adding a sysfs entry might be nice as incremental patch.
> 
> Let me know if needed, I'll make sure it will be implemented.

No idea. Frankly I'm not sure I fully get the usecase for this feature
but we have an ack from people who know much more about storage. I don't
really want to have too much tricky code dealing with this cornercase
though, so I'd like this as simple as possible.

If you have a mind to implement the sysfs attribute, go ahead - if
someone acks I'll merge it no problem.


> > 
> > > > > >     MODULE_PARM_DESC(num_request_queues,
> > > > > >     		 "Limit the number of request queues to use for blk device. "
> > > > > >     		 "0 for no limit. "