linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio_blk: allow 0 as num_request_queues
@ 2021-10-24 13:54 Michael S. Tsirkin
  2021-10-24 14:19 ` Max Gurtovoy
  0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2021-10-24 13:54 UTC (permalink / raw)
  To: linux-kernel
  Cc: Max Gurtovoy, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block

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);
 MODULE_PARM_DESC(num_request_queues,
 		 "Limit the number of request queues to use for blk device. "
 		 "0 for no limit. "
-- 
MST


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 13:54 [PATCH] virtio_blk: allow 0 as num_request_queues Michael S. Tsirkin
@ 2021-10-24 14:19 ` Max Gurtovoy
  2021-10-24 15:11   ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Max Gurtovoy @ 2021-10-24 14:19 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Jason Wang, Paolo Bonzini, Stefan Hajnoczi, Jens Axboe,
	virtualization, linux-block


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.


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 14:19 ` Max Gurtovoy
@ 2021-10-24 15:11   ` Michael S. Tsirkin
  2021-10-24 15:29     ` Max Gurtovoy
  0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2021-10-24 15:11 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: linux-kernel, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block

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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 15:11   ` Michael S. Tsirkin
@ 2021-10-24 15:29     ` Max Gurtovoy
  2021-10-24 15:49       ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Max Gurtovoy @ 2021-10-24 15:29 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block


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. "

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 15:29     ` Max Gurtovoy
@ 2021-10-24 15:49       ` Michael S. Tsirkin
  2021-10-24 22:30         ` Max Gurtovoy
  0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2021-10-24 15:49 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: linux-kernel, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block

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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 15:49       ` Michael S. Tsirkin
@ 2021-10-24 22:30         ` Max Gurtovoy
  2021-10-25 21:09           ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Max Gurtovoy @ 2021-10-24 22:30 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block


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.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_blk: allow 0 as num_request_queues
  2021-10-24 22:30         ` Max Gurtovoy
@ 2021-10-25 21:09           ` Michael S. Tsirkin
  0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2021-10-25 21:09 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: linux-kernel, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
	Jens Axboe, virtualization, linux-block

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. "


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-25 21:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-24 13:54 [PATCH] virtio_blk: allow 0 as num_request_queues Michael S. Tsirkin
2021-10-24 14:19 ` Max Gurtovoy
2021-10-24 15:11   ` Michael S. Tsirkin
2021-10-24 15:29     ` Max Gurtovoy
2021-10-24 15:49       ` Michael S. Tsirkin
2021-10-24 22:30         ` Max Gurtovoy
2021-10-25 21:09           ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).