linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
@ 2021-09-09 14:08 Yu Kuai
  2021-09-15  8:20 ` yukuai (C)
  2021-09-17 17:41 ` Michal Koutný
  0 siblings, 2 replies; 7+ messages in thread
From: Yu Kuai @ 2021-09-09 14:08 UTC (permalink / raw)
  To: tj, axboe; +Cc: cgroups, linux-block, linux-kernel, yukuai3, yi.zhang

We want to limit the overall iops/bps of the device in cgroup v2,
however "io.max" or "io.min" do not exist in root cgroup currently,
which makes it impossible.

This patch enables io throttle for root cgroup:
 - enable "io.max" and "io.min" in root
 - don't skip root group in tg_iops_limit() and tg_bps_limit()
 - don't skip root group in tg_conf_updated()

I'm not sure why this feature is disabled in the first place, is
there any problem or design constraint?

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 55c49015e533..fffe072de538 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -302,9 +302,6 @@ static uint64_t tg_bps_limit(struct throtl_grp *tg, int rw)
 	struct throtl_data *td;
 	uint64_t ret;
 
-	if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
-		return U64_MAX;
-
 	td = tg->td;
 	ret = tg->bps[rw][td->limit_index];
 	if (ret == 0 && td->limit_index == LIMIT_LOW) {
@@ -332,9 +329,6 @@ static unsigned int tg_iops_limit(struct throtl_grp *tg, int rw)
 	struct throtl_data *td;
 	unsigned int ret;
 
-	if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
-		return UINT_MAX;
-
 	td = tg->td;
 	ret = tg->iops[rw][td->limit_index];
 	if (ret == 0 && tg->td->limit_index == LIMIT_LOW) {
@@ -1430,9 +1424,8 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 		struct throtl_grp *parent_tg;
 
 		tg_update_has_rules(this_tg);
-		/* ignore root/second level */
-		if (!cgroup_subsys_on_dfl(io_cgrp_subsys) || !blkg->parent ||
-		    !blkg->parent->parent)
+		/* ignore root level */
+		if (!cgroup_subsys_on_dfl(io_cgrp_subsys) || !blkg->parent)
 			continue;
 		parent_tg = blkg_to_tg(blkg->parent);
 		/*
@@ -1771,7 +1764,6 @@ static struct cftype throtl_files[] = {
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	{
 		.name = "low",
-		.flags = CFTYPE_NOT_ON_ROOT,
 		.seq_show = tg_print_limit,
 		.write = tg_set_limit,
 		.private = LIMIT_LOW,
@@ -1779,7 +1771,6 @@ static struct cftype throtl_files[] = {
 #endif
 	{
 		.name = "max",
-		.flags = CFTYPE_NOT_ON_ROOT,
 		.seq_show = tg_print_limit,
 		.write = tg_set_limit,
 		.private = LIMIT_MAX,
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-09 14:08 [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2 Yu Kuai
@ 2021-09-15  8:20 ` yukuai (C)
  2021-09-17 17:41 ` Michal Koutný
  1 sibling, 0 replies; 7+ messages in thread
From: yukuai (C) @ 2021-09-15  8:20 UTC (permalink / raw)
  To: tj, axboe; +Cc: cgroups, linux-block, linux-kernel, yi.zhang

On 2021/09/09 22:08, Yu Kuai wrote:
> We want to limit the overall iops/bps of the device in cgroup v2,
> however "io.max" or "io.min" do not exist in root cgroup currently,
> which makes it impossible.
> 
> This patch enables io throttle for root cgroup:
>   - enable "io.max" and "io.min" in root
>   - don't skip root group in tg_iops_limit() and tg_bps_limit()
>   - don't skip root group in tg_conf_updated()
> 
> I'm not sure why this feature is disabled in the first place, is
> there any problem or design constraint?
> 
Ping...

BTW, sorry about the misspell of "io.low".
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   block/blk-throttle.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 55c49015e533..fffe072de538 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -302,9 +302,6 @@ static uint64_t tg_bps_limit(struct throtl_grp *tg, int rw)
>   	struct throtl_data *td;
>   	uint64_t ret;
>   
> -	if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
> -		return U64_MAX;
> -
>   	td = tg->td;
>   	ret = tg->bps[rw][td->limit_index];
>   	if (ret == 0 && td->limit_index == LIMIT_LOW) {
> @@ -332,9 +329,6 @@ static unsigned int tg_iops_limit(struct throtl_grp *tg, int rw)
>   	struct throtl_data *td;
>   	unsigned int ret;
>   
> -	if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
> -		return UINT_MAX;
> -
>   	td = tg->td;
>   	ret = tg->iops[rw][td->limit_index];
>   	if (ret == 0 && tg->td->limit_index == LIMIT_LOW) {
> @@ -1430,9 +1424,8 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
>   		struct throtl_grp *parent_tg;
>   
>   		tg_update_has_rules(this_tg);
> -		/* ignore root/second level */
> -		if (!cgroup_subsys_on_dfl(io_cgrp_subsys) || !blkg->parent ||
> -		    !blkg->parent->parent)
> +		/* ignore root level */
> +		if (!cgroup_subsys_on_dfl(io_cgrp_subsys) || !blkg->parent)
>   			continue;
>   		parent_tg = blkg_to_tg(blkg->parent);
>   		/*
> @@ -1771,7 +1764,6 @@ static struct cftype throtl_files[] = {
>   #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>   	{
>   		.name = "low",
> -		.flags = CFTYPE_NOT_ON_ROOT,
>   		.seq_show = tg_print_limit,
>   		.write = tg_set_limit,
>   		.private = LIMIT_LOW,
> @@ -1779,7 +1771,6 @@ static struct cftype throtl_files[] = {
>   #endif
>   	{
>   		.name = "max",
> -		.flags = CFTYPE_NOT_ON_ROOT,
>   		.seq_show = tg_print_limit,
>   		.write = tg_set_limit,
>   		.private = LIMIT_MAX,
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-09 14:08 [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2 Yu Kuai
  2021-09-15  8:20 ` yukuai (C)
@ 2021-09-17 17:41 ` Michal Koutný
  2021-09-17 19:58   ` Khazhy Kumykov
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Koutný @ 2021-09-17 17:41 UTC (permalink / raw)
  To: Yu Kuai; +Cc: tj, axboe, cgroups, linux-block, linux-kernel, yi.zhang

Hello Yu.

On Thu, Sep 09, 2021 at 10:08:15PM +0800, Yu Kuai <yukuai3@huawei.com> wrote:
> I'm not sure why this feature is disabled in the first place, is
> there any problem or design constraint?

The idea for v2 is that in the root cgroup remain only kernel threads that
provide "global" services and any user workload that should be
constrained is put into non-root cgroups. Additionally, if kernel
threads carry out work associated with a cgroup they can charge it to
the respective cgroup.

[snip]
> We want to limit the overall iops/bps of the device in cgroup v2,

Cui bono? (I mean what is the reason for throttling on the global level
when there's no other entity utiliting the residual?
<joke>Your drives are too fast?</joke>)

Michal

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-17 17:41 ` Michal Koutný
@ 2021-09-17 19:58   ` Khazhy Kumykov
  2021-09-19 10:31     ` yukuai (C)
  0 siblings, 1 reply; 7+ messages in thread
From: Khazhy Kumykov @ 2021-09-17 19:58 UTC (permalink / raw)
  To: Michal Koutný
  Cc: Yu Kuai, tj, axboe, cgroups, linux-block, linux-kernel, yi.zhang

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On Fri, Sep 17, 2021 at 10:41 AM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hello Yu.
>
> On Thu, Sep 09, 2021 at 10:08:15PM +0800, Yu Kuai <yukuai3@huawei.com> wrote:
> > I'm not sure why this feature is disabled in the first place, is
> > there any problem or design constraint?
>
> The idea for v2 is that in the root cgroup remain only kernel threads that
> provide "global" services and any user workload that should be
> constrained is put into non-root cgroups. Additionally, if kernel
> threads carry out work associated with a cgroup they can charge it to
> the respective cgroup.
>
> [snip]
> > We want to limit the overall iops/bps of the device in cgroup v2,
>
> Cui bono? (I mean what is the reason for throttling on the global level
> when there's no other entity utiliting the residual?
> <joke>Your drives are too fast?</joke>)

We'd be interested in something like this as well. (at least for
io.max). Our use case is providing remote devices which are a shared
resource. A "global" throttle like this (which is set by a local
management daemon) allows for throttling before sending network
traffic. It's also useful since we can put this throttle on a dm, so
we can enforce an aggregate throttle without needing backchannels to
coordinate multiple targets.
(This does also bring up: if this is a useful thing, would it make
sense to tie to the device, vs. requiring cgroup. We happen to use
cgroups so that requirement doesn't affect us).

Khazhy
>
> Michal

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3996 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-17 19:58   ` Khazhy Kumykov
@ 2021-09-19 10:31     ` yukuai (C)
  2021-09-21 13:44       ` Michal Koutný
  0 siblings, 1 reply; 7+ messages in thread
From: yukuai (C) @ 2021-09-19 10:31 UTC (permalink / raw)
  To: Khazhy Kumykov, Michal Koutný
  Cc: tj, axboe, cgroups, linux-block, linux-kernel, yi.zhang

On 2021/09/18 3:58, Khazhy Kumykov wrote:
> On Fri, Sep 17, 2021 at 10:41 AM Michal Koutný <mkoutny@suse.com> wrote:
>>
>> Hello Yu.
>>
>> On Thu, Sep 09, 2021 at 10:08:15PM +0800, Yu Kuai <yukuai3@huawei.com> wrote:
>>> I'm not sure why this feature is disabled in the first place, is
>>> there any problem or design constraint?
>>
>> The idea for v2 is that in the root cgroup remain only kernel threads that
>> provide "global" services and any user workload that should be
>> constrained is put into non-root cgroups. Additionally, if kernel
>> threads carry out work associated with a cgroup they can charge it to
>> the respective cgroup.
>>
>> [snip]
>>> We want to limit the overall iops/bps of the device in cgroup v2,
>>
>> Cui bono? (I mean what is the reason for throttling on the global level
>> when there's no other entity utiliting the residual?
>> <joke>Your drives are too fast?</joke>)
> 
> We'd be interested in something like this as well. (at least for
> io.max). Our use case is providing remote devices which are a shared
> resource. A "global" throttle like this (which is set by a local

Our use case is similair to this, a host can provide several remote
devices to difierent client. If one client is under high io pressure,
other client might be affected. Thus we want to limit the overall
iops/bps from the client.

Thanks,
Kuai

> management daemon) allows for throttling before sending network
> traffic. It's also useful since we can put this throttle on a dm, so
> we can enforce an aggregate throttle without needing backchannels to
> coordinate multiple targets.
> (This does also bring up: if this is a useful thing, would it make
> sense to tie to the device, vs. requiring cgroup. We happen to use
> cgroups so that requirement doesn't affect us).
> 
> Khazhy
>>
>> Michal

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-19 10:31     ` yukuai (C)
@ 2021-09-21 13:44       ` Michal Koutný
  2021-09-27 17:08         ` Tejun Heo
  0 siblings, 1 reply; 7+ messages in thread
From: Michal Koutný @ 2021-09-21 13:44 UTC (permalink / raw)
  To: yukuai (C), Khazhy Kumykov
  Cc: tj, axboe, cgroups, linux-block, linux-kernel, yi.zhang

On Sun, Sep 19, 2021 at 06:31:38PM +0800, "yukuai (C)" <yukuai3@huawei.com> wrote:
> Our use case is similair to this, a host can provide several remote
> devices to difierent client. If one client is under high io pressure,
> other client might be affected. Thus we want to limit the overall
> iops/bps from the client.

I see where are you coming from now. (Perhaps I'd suggest
allocating/prioritizing the allowances on the hosting side. If simply
wrapping "everything" into a non-root cgroup is not enough.)


On 2021/09/18 3:58, Khazhy Kumykov wrote:
> (This does also bring up: if this is a useful thing, would it make
> sense to tie to the device, vs. requiring cgroup. We happen to use
> cgroups so that requirement doesn't affect us).

Good point, That's IMO a better idea, it'd be more consistent with other
resources for which there exist global (cgroup independent) kernel
constraints (e.g. threads-max sysctl, mem= cmdline, cpu hotplug) that
double the root cgroup contraint role.

OTOH, this also deepens the precedent of init NS root cgroup being
special (more special than a container's root cgroup).

My .02€,
Michal

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2
  2021-09-21 13:44       ` Michal Koutný
@ 2021-09-27 17:08         ` Tejun Heo
  0 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2021-09-27 17:08 UTC (permalink / raw)
  To: Michal Koutný
  Cc: yukuai (C),
	Khazhy Kumykov, axboe, cgroups, linux-block, linux-kernel,
	yi.zhang

Hello,

On Tue, Sep 21, 2021 at 03:44:14PM +0200, Michal Koutný wrote:
> On 2021/09/18 3:58, Khazhy Kumykov wrote:
> > (This does also bring up: if this is a useful thing, would it make
> > sense to tie to the device, vs. requiring cgroup. We happen to use
> > cgroups so that requirement doesn't affect us).
> 
> Good point, That's IMO a better idea, it'd be more consistent with other
> resources for which there exist global (cgroup independent) kernel
> constraints (e.g. threads-max sysctl, mem= cmdline, cpu hotplug) that
> double the root cgroup contraint role.

This is why I usually try to push root-cgroup level features outside cgroup
because it really doesn't have much to do with cgroups at the root level.
For visibility stuff, we do replicate quite a bit in the root level because
not doing so becomes too painful for users but for control I'm more
hesitant.

One side-way solution could be using iocost. It doesn't have root-level
control per-se but it does configure per-device attributes which define what
the device can and is allowed to do so that it can be used as the basis for
weighted fair distribution. Even if IO control is disabled from the root
level, it'll still modulate the device according to the parameters.

> OTOH, this also deepens the precedent of init NS root cgroup being
> special (more special than a container's root cgroup).

While it does seem like an aesthetical wrinkle, I don't think this is a
practical problem. System root being different is a given whether
aesthetically pleasing or not (not the most important but we have
CONFIG_CGROUPS after all). I don't think it'll lead anywhere good to try to
mask the differences.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-27 17:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-09 14:08 [RFC PATCH] blk-throttle: enable io throttle for root in cgroup v2 Yu Kuai
2021-09-15  8:20 ` yukuai (C)
2021-09-17 17:41 ` Michal Koutný
2021-09-17 19:58   ` Khazhy Kumykov
2021-09-19 10:31     ` yukuai (C)
2021-09-21 13:44       ` Michal Koutný
2021-09-27 17:08         ` Tejun Heo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).