| Message ID | 20230130211347.832110-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loop: Improve the hw_queue_depth kernel module parameter implementation | expand |
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
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,
> 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>>
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);
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(-)