| Message ID | 20210622231952.5625-10-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loop: small clenaup | expand |
On 6/22/21 4:19 PM, Chaitanya Kulkarni wrote: > Instead of hardcoding queue depth allow user to set the hw queue depth > using module parameter. Set default value to 128 to retain the existing > behavior. > > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> > --- > drivers/block/loop.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 6fc3cfa87598..c0d54ffd6ef3 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1942,6 +1942,9 @@ module_param(max_loop, int, 0444); > MODULE_PARM_DESC(max_loop, "Maximum number of loop devices"); > module_param(max_part, int, 0444); > MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device"); > +static int hw_queue_depth = 128; > +module_param_named(hw_queue_depth, hw_queue_depth, int, 0444); > +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128"); > MODULE_LICENSE("GPL"); > MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR); > > @@ -2094,7 +2097,7 @@ static int loop_add(struct loop_device **l, int i) > err = -ENOMEM; > lo->tag_set.ops = &loop_mq_ops; > lo->tag_set.nr_hw_queues = 1; > - lo->tag_set.queue_depth = 128; > + lo->tag_set.queue_depth = hw_queue_depth; > lo->tag_set.numa_node = NUMA_NO_NODE; > lo->tag_set.cmd_size = sizeof(struct loop_cmd); > lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING; Is there any use case for which the performance improves by using another queue depth than the default? Thanks, Bart.
Bart, On 6/22/21 4:27 PM, Bart Van Assche wrote: > On 6/22/21 4:19 PM, Chaitanya Kulkarni wrote: >> Instead of hardcoding queue depth allow user to set the hw queue depth >> using module parameter. Set default value to 128 to retain the existing >> behavior. >> >> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> >> --- >> drivers/block/loop.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 6fc3cfa87598..c0d54ffd6ef3 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -1942,6 +1942,9 @@ module_param(max_loop, int, 0444); >> MODULE_PARM_DESC(max_loop, "Maximum number of loop devices"); >> module_param(max_part, int, 0444); >> MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device"); >> +static int hw_queue_depth = 128; >> +module_param_named(hw_queue_depth, hw_queue_depth, int, 0444); >> +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128"); >> MODULE_LICENSE("GPL"); >> MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR); >> >> @@ -2094,7 +2097,7 @@ static int loop_add(struct loop_device **l, int i) >> err = -ENOMEM; >> lo->tag_set.ops = &loop_mq_ops; >> lo->tag_set.nr_hw_queues = 1; >> - lo->tag_set.queue_depth = 128; >> + lo->tag_set.queue_depth = hw_queue_depth; >> lo->tag_set.numa_node = NUMA_NO_NODE; >> lo->tag_set.cmd_size = sizeof(struct loop_cmd); >> lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING; > Is there any use case for which the performance improves by using > another queue depth than the default? Unfortunately I don't have access to all the applications so I can come up with quantitative data, I can try synthetic applications such as fio. This patch is more on the side of allowing user to change the qd value so they can experiment, making loop qd flexible like other block drivers which loop lacks right now. > Thanks, > > Bart. > > >
On 6/23/21 9:36 PM, Chaitanya Kulkarni wrote: > On 6/22/21 4:27 PM, Bart Van Assche wrote: >> On 6/22/21 4:19 PM, Chaitanya Kulkarni wrote: >>> @@ -2094,7 +2097,7 @@ static int loop_add(struct loop_device **l, int i) >>> err = -ENOMEM; >>> lo->tag_set.ops = &loop_mq_ops; >>> lo->tag_set.nr_hw_queues = 1; >>> - lo->tag_set.queue_depth = 128; >>> + lo->tag_set.queue_depth = hw_queue_depth; >>> lo->tag_set.numa_node = NUMA_NO_NODE; >>> lo->tag_set.cmd_size = sizeof(struct loop_cmd); >>> lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING; >> Is there any use case for which the performance improves by using >> another queue depth than the default? > > Unfortunately I don't have access to all the applications so I can come > up with > quantitative data, I can try synthetic applications such as fio. > > This patch is more on the side of allowing user to change the qd value > so they can > experiment, making loop qd flexible like other block drivers which loop > lacks right now. Kernel module parameters are inflexible. If there is agreement that this parameter should become configurable it probably should be made configurable per loop device instead of introducing a single global parameter. Thanks, Bart.
>> Unfortunately I don't have access to all the applications so I can come >> up with >> quantitative data, I can try synthetic applications such as fio. >> >> This patch is more on the side of allowing user to change the qd value >> so they can >> experiment, making loop qd flexible like other block drivers which loop >> lacks right now. > Kernel module parameters are inflexible. If there is agreement that this > parameter should become configurable it probably should be made > configurable per loop device instead of introducing a single global > parameter. Agree, but it is still better than having a statically assigned value in the code. I'll drop this in the V2 version, meanwhile it will be great to have your review comments on the rest of the series.
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 6fc3cfa87598..c0d54ffd6ef3 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1942,6 +1942,9 @@ module_param(max_loop, int, 0444); MODULE_PARM_DESC(max_loop, "Maximum number of loop devices"); module_param(max_part, int, 0444); MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device"); +static int hw_queue_depth = 128; +module_param_named(hw_queue_depth, hw_queue_depth, int, 0444); +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128"); MODULE_LICENSE("GPL"); MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR); @@ -2094,7 +2097,7 @@ static int loop_add(struct loop_device **l, int i) err = -ENOMEM; lo->tag_set.ops = &loop_mq_ops; lo->tag_set.nr_hw_queues = 1; - lo->tag_set.queue_depth = 128; + lo->tag_set.queue_depth = hw_queue_depth; lo->tag_set.numa_node = NUMA_NO_NODE; lo->tag_set.cmd_size = sizeof(struct loop_cmd); lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
Instead of hardcoding queue depth allow user to set the hw queue depth using module parameter. Set default value to 128 to retain the existing behavior. Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> --- drivers/block/loop.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)