[block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
diff mbox series

Message ID 20190611194959.GJ3341036@devbig004.ftw2.facebook.com
State New
Headers show
Series
  • [block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
Related show

Commit Message

Tejun Heo June 11, 2019, 7:49 p.m. UTC
(Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
symlink to the bfq.weight cgroup parameter")

Many userspace tools and services use the proportional-share policy of
the blkio/io cgroups controller. The CFQ I/O scheduler implemented
this policy for the legacy block layer. To modify the weight of a
group in case CFQ was in charge, the 'weight' parameter of the group
must be modified. On the other hand, the BFQ I/O scheduler implements
the same policy in blk-mq, but, with BFQ, the parameter to modify has
a different name: bfq.weight (forced choice until legacy block was
present, because two different policies cannot share a common
parameter in cgroups).

Due to CFQ legacy, most if not all userspace configurations still use
the parameter 'weight', and for the moment do not seem likely to be
changed. But, when CFQ went away with legacy block, such a parameter
ceased to exist.

19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup
parameter") tried to solve the problem by creating a symlink but there
doesn't seem to be a good reason for the added complexity.  Make bfq
simply create interface file io.weight instead of io.bfq.weight.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Paolo Valente <paolo.valente@linaro.org>
Cc: Angelo Ruocco <angeloruocco90@gmail.com>
---
 block/bfq-cgroup.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Tejun Heo June 11, 2019, 9:17 p.m. UTC | #1
On Tue, Jun 11, 2019 at 12:49:59PM -0700, Tejun Heo wrote:
> (Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
> symlink to the bfq.weight cgroup parameter")
> 
> Many userspace tools and services use the proportional-share policy of
> the blkio/io cgroups controller. The CFQ I/O scheduler implemented
> this policy for the legacy block layer. To modify the weight of a
> group in case CFQ was in charge, the 'weight' parameter of the group
> must be modified. On the other hand, the BFQ I/O scheduler implements
> the same policy in blk-mq, but, with BFQ, the parameter to modify has
> a different name: bfq.weight (forced choice until legacy block was
> present, because two different policies cannot share a common
> parameter in cgroups).

Sorry, please don't apply this patch.  The cgroup interface currently
implemented by bfq doesn't follow the io.weight interface spec.  It's
different from cfq interface and symlinking to or renaming it won't do
anybody any good.

For now, the only thing we can do looks like keeping it as-is.

Thanks.
Paolo Valente June 12, 2019, 7:32 a.m. UTC | #2
> Il giorno 11 giu 2019, alle ore 23:17, Tejun Heo <tj@kernel.org> ha scritto:
> 
> On Tue, Jun 11, 2019 at 12:49:59PM -0700, Tejun Heo wrote:
>> (Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
>> symlink to the bfq.weight cgroup parameter")
>> 
>> Many userspace tools and services use the proportional-share policy of
>> the blkio/io cgroups controller. The CFQ I/O scheduler implemented
>> this policy for the legacy block layer. To modify the weight of a
>> group in case CFQ was in charge, the 'weight' parameter of the group
>> must be modified. On the other hand, the BFQ I/O scheduler implements
>> the same policy in blk-mq, but, with BFQ, the parameter to modify has
>> a different name: bfq.weight (forced choice until legacy block was
>> present, because two different policies cannot share a common
>> parameter in cgroups).
> 
> Sorry, please don't apply this patch.  The cgroup interface currently
> implemented by bfq doesn't follow the io.weight interface spec.

Could you elaborate a little more on this?

bfq code for setting up and handling io.weight (more precisely
io.bfq.weight) is a copy and paste of cfq code.

Thanks,
Paolo

> It's
> different from cfq interface and symlinking to or renaming it won't do
> anybody any good.
> 
> For now, the only thing we can do looks like keeping it as-is.
> 
> Thanks.
> 
> --
> tejun
Tejun Heo June 12, 2019, 1:39 p.m. UTC | #3
On Wed, Jun 12, 2019 at 09:32:07AM +0200, Paolo Valente wrote:
> Could you elaborate a little more on this?

Doesn't seem like you did.

> bfq code for setting up and handling io.weight (more precisely
> io.bfq.weight) is a copy and paste of cfq code.

Please take a look at the documentations under
Documentation/cgroup-v1/blk-iocontroller.txt and
Documentation/admin-guide/cgroup-v2.rst.

Thanks.
Paolo Valente June 13, 2019, 6:10 a.m. UTC | #4
> Il giorno 12 giu 2019, alle ore 15:39, Tejun Heo <tj@kernel.org> ha scritto:
> 
> On Wed, Jun 12, 2019 at 09:32:07AM +0200, Paolo Valente wrote:
>> Could you elaborate a little more on this?
> 
> Doesn't seem like you did.
> 
>> bfq code for setting up and handling io.weight (more precisely
>> io.bfq.weight) is a copy and paste of cfq code.
> 
> Please take a look at the documentations under
> Documentation/cgroup-v1/blk-iocontroller.txt and
> Documentation/admin-guide/cgroup-v2.rst.
> 

I'm sorry, but I don't see what doesn't match between BFQ's and CFQ's
implementations of the weight parameter.

So, if you agree, let me paste the two snippets for v1 that involve
weights.  So maybe you can at least tell me 'here' (then I'll try the
same with v2).

----------------------------------------------------------------------

	mount -t tmpfs cgroup_root /sys/fs/cgroup
	mkdir /sys/fs/cgroup/blkio
	mount -t cgroup -o blkio none /sys/fs/cgroup/blkio

- Create two cgroups
	mkdir -p /sys/fs/cgroup/blkio/test1/ /sys/fs/cgroup/blkio/test2

- Set weights of group test1 and test2
	echo 1000 > /sys/fs/cgroup/blkio/test1/blkio.weight
	echo 500 > /sys/fs/cgroup/blkio/test2/blkio.weight

-> This is exactly how you set weights with BFQ

----------------------------------------------------------------------

Details of cgroup files
=======================
Proportional weight policy files
--------------------------------
- blkio.weight
	- Specifies per cgroup weight. This is default weight of the group
	  on all the devices until and unless overridden by per device rule.
	  (See blkio.weight_device).
	  Currently allowed range of weights is from 10 to 1000.

-> This is the exact implementation of the weight parameter in BFQ

BFQ does not implement weight_device, but we are not talking about
weight_device here.  More precisely, *nothing* implements weight_device
any longer in cgroups-v1, so the documentation is wrong altogether.

Looking forward to your feedback,
Paolo



> Thanks.
> 
> --
> tejun
Tejun Heo June 14, 2019, 8:22 p.m. UTC | #5
Hello,

On Thu, Jun 13, 2019 at 08:10:38AM +0200, Paolo Valente wrote:
> BFQ does not implement weight_device, but we are not talking about
> weight_device here.  More precisely, *nothing* implements weight_device
> any longer in cgroups-v1, so the documentation is wrong altogether.

I can't see how renaming an interface file which has different
behaviors on the same input to the same name is something acceptable.
Imagine how confusing that'd be to userspace.  If you want to rename
the control knob to io.weight, please implement full semantics for
both cgroup1 and cgroup2.

I just posted a separate io.weight implementation for cgroup2 but it's
easy to provide a way to override the interface files when bfq gets
selected, so let me know when you have a working implementation of the
interface.

Thanks.
Paolo Valente June 14, 2019, 9:05 p.m. UTC | #6
> Il giorno 14 giu 2019, alle ore 22:22, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello,
> 
> On Thu, Jun 13, 2019 at 08:10:38AM +0200, Paolo Valente wrote:
>> BFQ does not implement weight_device, but we are not talking about
>> weight_device here.  More precisely, *nothing* implements weight_device
>> any longer in cgroups-v1, so the documentation is wrong altogether.
> 
> I can't see how renaming an interface file which has different
> behaviors on the same input to the same name is something acceptable.
> Imagine how confusing that'd be to userspace.  If you want to rename
> the control knob to io.weight, please implement full semantics for
> both cgroup1 and cgroup2.
> 

I want to, and I've tried to make it easier for you to point me to
what is missing exactly.  Once again, the only missing piece I see are
per-device weights.

> I just posted a separate io.weight implementation for cgroup2 but it's
> easy to provide a way to override the interface files when bfq gets
> selected, so let me know when you have a working implementation of the
> interface.
> 

Ok, thanks.  I'll ping you when also per-device weights are available
in BFQ's interface.

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun
Paolo Valente Aug. 20, 2019, 6:58 a.m. UTC | #7
> Il giorno 14 giu 2019, alle ore 23:05, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> 
> 
>> Il giorno 14 giu 2019, alle ore 22:22, Tejun Heo <tj@kernel.org> ha scritto:
>> 
>> Hello,
>> 
>> On Thu, Jun 13, 2019 at 08:10:38AM +0200, Paolo Valente wrote:
>>> BFQ does not implement weight_device, but we are not talking about
>>> weight_device here.  More precisely, *nothing* implements weight_device
>>> any longer in cgroups-v1, so the documentation is wrong altogether.
>> 
>> I can't see how renaming an interface file which has different
>> behaviors on the same input to the same name is something acceptable.
>> Imagine how confusing that'd be to userspace.  If you want to rename
>> the control knob to io.weight, please implement full semantics for
>> both cgroup1 and cgroup2.
>> 
> 
> I want to, and I've tried to make it easier for you to point me to
> what is missing exactly.  Once again, the only missing piece I see are
> per-device weights.
> 
>> I just posted a separate io.weight implementation for cgroup2 but it's
>> easy to provide a way to override the interface files when bfq gets
>> selected, so let me know when you have a working implementation of the
>> interface.
>> 
> 
> Ok, thanks.  I'll ping you when also per-device weights are available
> in BFQ's interface.
> 

Hi Tejun,
have you seen the patch introducing per-device weights [1]?

[1] https://lkml.org/lkml/2019/8/5/83

Thanks,
Paolo

> Thanks,
> Paolo
> 
>> Thanks.
>> 
>> -- 
>> tejun
>

Patch
diff mbox series

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index b3796a40a61a..c68c6aa22154 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -1045,8 +1045,8 @@  struct blkcg_policy blkcg_policy_bfq = {
 
 struct cftype bfq_blkcg_legacy_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "io.weight",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NO_PREFIX,
 		.seq_show = bfq_io_show_weight,
 		.write_u64 = bfq_io_set_weight_legacy,
 	},
@@ -1165,8 +1165,8 @@  struct cftype bfq_blkcg_legacy_files[] = {
 
 struct cftype bfq_blkg_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "io.weight",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NO_PREFIX,
 		.seq_show = bfq_io_show_weight,
 		.write = bfq_io_set_weight,
 	},