Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Message-ID: <a2060fc7-cc4d-c4d4-e7fe-e4a1e544104f@nvidia.com>
Date:   Sun, 24 Oct 2021 18:29:59 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] virtio_blk: allow 0 as num_request_queues
Content-Language: en-US
To:     "Michael S. Tsirkin" <mst@redhat.com>
CC:     <linux-kernel@vger.kernel.org>, Jason Wang <jasowang@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Jens Axboe <axboe@kernel.dk>,
        <virtualization@lists.linux-foundation.org>,
        <linux-block@vger.kernel.org>
References: <20211024135412.1516393-1-mst@redhat.com>
 <855eb993-074b-24b9-a184-d479bd0b342b@nvidia.com>
 <20211024105841-mutt-send-email-mst@kernel.org>
From:   Max Gurtovoy <mgurtovoy@nvidia.com>
In-Reply-To: <20211024105841-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2021 15:30:05.6490
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 404cf235-d027-43f1-13a2-08d9970327d0
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT064.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1201MB2558
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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