| Message ID | 20190611194959.GJ3341036@devbig004.ftw2.facebook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight | expand |
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.
> 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
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.
> 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
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.
> 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
> 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 >
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, },
(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(-)