[PATCH] loop: Improve the hw_queue_depth kernel module parameter implementation

[PATCH] loop: Improve the hw_queue_depth kernel module parameter implementation

[Bart Van Assche]

Make the following minor changes which were reported by colleagues
while reviewing this code:
- Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH
  definition since these are superfluous.
- Accept other number formats than decimal, e.g. hexadecimal.
- Do not set hw_queue_depth to an out-of-range value, even if that value
  won't be used.
- Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter
  description to prevent that the description gets out of sync.

This patch has been tested as follows:

 # modprobe -r loop
 # modprobe loop hw_queue_depth=-1
 modprobe: ERROR: could not insert 'loop': Invalid argument
 # modprobe loop hw_queue_depth=0
 modprobe: ERROR: could not insert 'loop': Invalid argument
 # modprobe loop hw_queue_depth=1; cat /sys/module/loop/parameters/hw_queue_depth
 1
 # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=0x10
 16
 # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=128
 128
 # modprobe -r loop; modprobe loop hw_queue_depth=129; cat /sys/module/loop/parameters/hw_queue_depth
 129
 # modprobe -r loop; modprobe loop hw_queue_depth=$((1<<32))
 modprobe: ERROR: could not insert 'loop': Numerical result out of range

See also commit ef44c50837ab ("loop: allow user to set the queue
depth").

Cc: Chaitanya Kulkarni <kch@nvidia.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/loop.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1518a6423279..5f04235e4ff7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -90,7 +90,7 @@ struct loop_cmd {
 };
 
 #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
-#define LOOP_DEFAULT_HW_Q_DEPTH (128)
+#define LOOP_DEFAULT_HW_Q_DEPTH 128
 
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
@@ -1792,9 +1792,15 @@ static int hw_queue_depth = LOOP_DEFAULT_HW_Q_DEPTH;
 
 static int loop_set_hw_queue_depth(const char *s, const struct kernel_param *p)
 {
-	int ret = kstrtoint(s, 10, &hw_queue_depth);
+	int qd, ret;
 
-	return (ret || (hw_queue_depth < 1)) ? -EINVAL : 0;
+	ret = kstrtoint(s, 0, &qd);
+	if (ret < 0)
+		return ret;
+	if (qd < 1)
+		return -EINVAL;
+	hw_queue_depth = qd;
+	return 0;
 }
 
 static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
@@ -1803,7 +1809,7 @@ static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
 };
 
 device_param_cb(hw_queue_depth, &loop_hw_qdepth_param_ops, &hw_queue_depth, 0444);
-MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128");
+MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: " __stringify(LOOP_DEFAULT_HW_Q_DEPTH));
 
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

Re: [PATCH] loop: Improve the hw_queue_depth kernel module parameter implementation

[Chaitanya Kulkarni]

On 1/30/23 13:13, Bart Van Assche wrote:
> Make the following minor changes which were reported by colleagues
> while reviewing this code:
> - Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH
>    definition since these are superfluous.
> - Accept other number formats than decimal, e.g. hexadecimal.
> - Do not set hw_queue_depth to an out-of-range value, even if that value
>    won't be used.
> - Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter
>    description to prevent that the description gets out of sync.
> 
> This patch has been tested as follows:
> 
>   # modprobe -r loop
>   # modprobe loop hw_queue_depth=-1
>   modprobe: ERROR: could not insert 'loop': Invalid argument
>   # modprobe loop hw_queue_depth=0
>   modprobe: ERROR: could not insert 'loop': Invalid argument
>   # modprobe loop hw_queue_depth=1; cat /sys/module/loop/parameters/hw_queue_depth
>   1
>   # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=0x10
>   16
>   # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=128
>   128
>   # modprobe -r loop; modprobe loop hw_queue_depth=129; cat /sys/module/loop/parameters/hw_queue_depth
>   129
>   # modprobe -r loop; modprobe loop hw_queue_depth=$((1<<32))
>   modprobe: ERROR: could not insert 'loop': Numerical result out of range
> 
> See also commit ef44c50837ab ("loop: allow user to set the queue
> depth").
> 
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] loop: Improve the hw_queue_depth kernel module parameter implementation

[Jens Axboe]

On Mon, 30 Jan 2023 13:13:47 -0800, Bart Van Assche wrote:
> Make the following minor changes which were reported by colleagues
> while reviewing this code:
> - Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH
>   definition since these are superfluous.
> - Accept other number formats than decimal, e.g. hexadecimal.
> - Do not set hw_queue_depth to an out-of-range value, even if that value
>   won't be used.
> - Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter
>   description to prevent that the description gets out of sync.
> 
> [...]

Applied, thanks!

[1/1] loop: Improve the hw_queue_depth kernel module parameter implementation
      commit: e152a05fa054170c05f1d5e04e93e2e75ea11405

Best regards,
-- 
Jens Axboe

Re: [PATCH] loop: Improve the hw_queue_depth kernel module parameter implementation

[Himanshu Madhani]

> On Jan 30, 2023, at 1:13 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> Make the following minor changes which were reported by colleagues
> while reviewing this code:
> - Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH
>  definition since these are superfluous.
> - Accept other number formats than decimal, e.g. hexadecimal.
> - Do not set hw_queue_depth to an out-of-range value, even if that value
>  won't be used.
> - Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter
>  description to prevent that the description gets out of sync.
> 
> This patch has been tested as follows:
> 
> # modprobe -r loop
> # modprobe loop hw_queue_depth=-1
> modprobe: ERROR: could not insert 'loop': Invalid argument
> # modprobe loop hw_queue_depth=0
> modprobe: ERROR: could not insert 'loop': Invalid argument
> # modprobe loop hw_queue_depth=1; cat /sys/module/loop/parameters/hw_queue_depth
> 1
> # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=0x10
> 16
> # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=128
> 128
> # modprobe -r loop; modprobe loop hw_queue_depth=129; cat /sys/module/loop/parameters/hw_queue_depth
> 129
> # modprobe -r loop; modprobe loop hw_queue_depth=$((1<<32))
> modprobe: ERROR: could not insert 'loop': Numerical result out of range
> 
> See also commit ef44c50837ab ("loop: allow user to set the queue
> depth").
> 
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/block/loop.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1518a6423279..5f04235e4ff7 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -90,7 +90,7 @@ struct loop_cmd {
> };
> 
> #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
> -#define LOOP_DEFAULT_HW_Q_DEPTH (128)
> +#define LOOP_DEFAULT_HW_Q_DEPTH 128
> 
> static DEFINE_IDR(loop_index_idr);
> static DEFINE_MUTEX(loop_ctl_mutex);
> @@ -1792,9 +1792,15 @@ static int hw_queue_depth = LOOP_DEFAULT_HW_Q_DEPTH;
> 
> static int loop_set_hw_queue_depth(const char *s, const struct kernel_param *p)
> {
> - int ret = kstrtoint(s, 10, &hw_queue_depth);
> + int qd, ret;
> 
> - return (ret || (hw_queue_depth < 1)) ? -EINVAL : 0;
> + ret = kstrtoint(s, 0, &qd);
> + if (ret < 0)
> + return ret;
> + if (qd < 1)
> + return -EINVAL;
> + hw_queue_depth = qd;
> + return 0;
> }
> 
> static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
> @@ -1803,7 +1809,7 @@ static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
> };
> 
> device_param_cb(hw_queue_depth, &loop_hw_qdepth_param_ops, &hw_queue_depth, 0444);
> -MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128");
> +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: " __stringify(LOOP_DEFAULT_HW_Q_DEPTH));
> 
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com <mailto:himanshu.madhani@oracle.com>>

-- 
Himanshu Madhani Oracle Linux Engineering