Hi,
Thanks for the report! I have a theory about what is possibly causing
this, so I will try to reproduce it and see if my assumptions are
correct.
Odin
Hi,
Thanks for the report! I have a theory about what is possibly causing
this, so I will try to reproduce it and see if my assumptions are
correct.
This means that a child's load was not null and it was inserted
whereas parent's load was null. This should not happen unless the
propagation failed somewhere
man. 21. jun. 2021 kl. 11:50 skrev Vincent Guittot [off-list ref]:
This means that a child's load was not null and it was inserted
whereas parent's load was null. This should not happen unless the
propagation failed somewhere
My initial thought is that the patch below will fix it, if that is the
issue (that a leaf is inserted, but the propagation is not "completed"
in unthrottle). Might that be the case? Still working on reproducing
the issue tho.
@@ -4930,12 +4930,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)if(cfs_rq_throttled(cfs_rq))gotounthrottle_throttle;-/*-*Oneparenthasbeenthrottledandcfs_rqremovedfromthe-*list.Additbacktonotbreaktheleaflist.-*/-if(throttled_hierarchy(cfs_rq))-list_add_leaf_cfs_rq(cfs_rq);+list_add_leaf_cfs_rq(cfs_rq);}/* At this point se is NULL and we are at root level*/
From: Sachin Sant <hidden> Date: 2021-06-21 10:57:54
On 21-Jun-2021, at 3:24 PM, Odin Ugedal [off-list ref] wrote:
man. 21. jun. 2021 kl. 11:50 skrev Vincent Guittot [off-list ref]:
quoted
This means that a child's load was not null and it was inserted
whereas parent's load was null. This should not happen unless the
propagation failed somewhere
My initial thought is that the patch below will fix it, if that is the
issue (that a leaf is inserted, but the propagation is not "completed"
in unthrottle). Might that be the case? Still working on reproducing
the issue tho.
Unfortunately this does not help. I can still recreate the failure.
Have attached the o/p from test run.
Thanks
-Sachin
@@ -4930,12 +4930,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)if(cfs_rq_throttled(cfs_rq))gotounthrottle_throttle;-/*-*Oneparenthasbeenthrottledandcfs_rqremovedfromthe-*list.Additbacktonotbreaktheleaflist.-*/-if(throttled_hierarchy(cfs_rq))-list_add_leaf_cfs_rq(cfs_rq);+list_add_leaf_cfs_rq(cfs_rq);}/* At this point se is NULL and we are at root level*/
Hi,
Did some more research, and it looks like this is what happens:
$ tree /sys/fs/cgroup/ltp/ -d --charset=ascii
/sys/fs/cgroup/ltp/
|-- drain
`-- test-6851
`-- level2
|-- level3a
| |-- worker1
| `-- worker2
`-- level3b
`-- worker3
Timeline (ish):
- worker3 gets throttled
- level3b is decayed, since it has no more load
- level2 get throttled
- worker3 get unthrottled
- level2 get unthrottled
- worker3 is added to list
- level3b is not added to list, since nr_running==0 and is decayed
The attached diff (based on
https://lore.kernel.org/lkml/20210518125202.78658-3-odin@uged.al/)
fixes the issue for me. Not the most elegant solution, but the
simplest one as of now, and to show what is wrong.
Any thoughts Vincent?
Thanks
Odin
@@ -4771,10 +4751,11 @@ static int tg_unthrottle_up(struct task_group
*tg, void *data)
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
cfs_rq->throttled_clock_task;
-
- /* Add cfs_rq with load or one or more already running
entities to the list */
- if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
+ if (cfs_rq->insert_on_unthrottle) {
list_add_leaf_cfs_rq(cfs_rq);
+ if (tg->parent)
+
tg->parent->cfs_rq[cpu_of(rq)]->insert_on_unthrottle = true;
+ }
}
return 0;
@@ -4788,7 +4769,7 @@ static int tg_throttle_down(struct task_group
*tg, void *data)
/* group is entering throttled state, stop time */
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task = rq_clock_task(rq);
- list_del_leaf_cfs_rq(cfs_rq);
+ cfs_rq->insert_on_unthrottle = list_del_leaf_cfs_rq(cfs_rq);
}
cfs_rq->throttle_count++;
From: Vincent Guittot <vincent.guittot@linaro.org> Date: 2021-06-21 16:41:15
Le lundi 21 juin 2021 à 14:42:23 (+0200), Odin Ugedal a écrit :
Hi,
Did some more research, and it looks like this is what happens:
$ tree /sys/fs/cgroup/ltp/ -d --charset=ascii
/sys/fs/cgroup/ltp/
|-- drain
`-- test-6851
`-- level2
|-- level3a
| |-- worker1
| `-- worker2
`-- level3b
`-- worker3
Timeline (ish):
- worker3 gets throttled
- level3b is decayed, since it has no more load
- level2 get throttled
- worker3 get unthrottled
- level2 get unthrottled
- worker3 is added to list
- level3b is not added to list, since nr_running==0 and is decayed
The attached diff (based on
https://lore.kernel.org/lkml/20210518125202.78658-3-odin@uged.al/)
fixes the issue for me. Not the most elegant solution, but the
simplest one as of now, and to show what is wrong.
Any thoughts Vincent?
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
---
kernel/sched/fair.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
@@ -4771,10 +4751,11 @@ static int tg_unthrottle_up(struct task_group
*tg, void *data)
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
cfs_rq->throttled_clock_task;
-
- /* Add cfs_rq with load or one or more already running
entities to the list */
- if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
+ if (cfs_rq->insert_on_unthrottle) {
list_add_leaf_cfs_rq(cfs_rq);
+ if (tg->parent)
+
tg->parent->cfs_rq[cpu_of(rq)]->insert_on_unthrottle = true;
+ }
}
return 0;
@@ -4788,7 +4769,7 @@ static int tg_throttle_down(struct task_group
*tg, void *data)
/* group is entering throttled state, stop time */
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task = rq_clock_task(rq);
- list_del_leaf_cfs_rq(cfs_rq);
+ cfs_rq->insert_on_unthrottle = list_del_leaf_cfs_rq(cfs_rq);
}
cfs_rq->throttle_count++;
man. 21. jun. 2021 kl. 18:22 skrev Vincent Guittot [off-list ref]:
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
Oh, yeah, that is a much more elegant solution! It fixes the issue as well!
Feel free to add this when/if you submit it as a patch:
Acked-by: Odin Ugedal <redacted>
Odin
From: Vincent Guittot <vincent.guittot@linaro.org> Date: 2021-06-21 17:08:12
On Mon, 21 Jun 2021 at 18:45, Odin Ugedal [off-list ref] wrote:
man. 21. jun. 2021 kl. 18:22 skrev Vincent Guittot [off-list ref]:
quoted
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
Oh, yeah, that is a much more elegant solution! It fixes the issue as well!
Feel free to add this when/if you submit it as a patch:
Acked-by: Odin Ugedal <redacted>
From: Vincent Guittot <vincent.guittot@linaro.org> Date: 2021-06-21 17:09:38
Hi Sacha
On Mon, 21 Jun 2021 at 18:22, Vincent Guittot
[off-list ref] wrote:
Le lundi 21 juin 2021 à 14:42:23 (+0200), Odin Ugedal a écrit :
quoted
Hi,
Did some more research, and it looks like this is what happens:
$ tree /sys/fs/cgroup/ltp/ -d --charset=ascii
/sys/fs/cgroup/ltp/
|-- drain
`-- test-6851
`-- level2
|-- level3a
| |-- worker1
| `-- worker2
`-- level3b
`-- worker3
Timeline (ish):
- worker3 gets throttled
- level3b is decayed, since it has no more load
- level2 get throttled
- worker3 get unthrottled
- level2 get unthrottled
- worker3 is added to list
- level3b is not added to list, since nr_running==0 and is decayed
The attached diff (based on
https://lore.kernel.org/lkml/20210518125202.78658-3-odin@uged.al/)
fixes the issue for me. Not the most elegant solution, but the
simplest one as of now, and to show what is wrong.
Any thoughts Vincent?
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
Could you confirm that this patch fixes the problem for you too ?
@@ -4771,10 +4751,11 @@ static int tg_unthrottle_up(struct task_group
*tg, void *data)
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
cfs_rq->throttled_clock_task;
-
- /* Add cfs_rq with load or one or more already running
entities to the list */
- if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
+ if (cfs_rq->insert_on_unthrottle) {
list_add_leaf_cfs_rq(cfs_rq);
+ if (tg->parent)
+
tg->parent->cfs_rq[cpu_of(rq)]->insert_on_unthrottle = true;
+ }
}
return 0;
@@ -4788,7 +4769,7 @@ static int tg_throttle_down(struct task_group
*tg, void *data)
/* group is entering throttled state, stop time */
if (!cfs_rq->throttle_count) {
cfs_rq->throttled_clock_task = rq_clock_task(rq);
- list_del_leaf_cfs_rq(cfs_rq);
+ cfs_rq->insert_on_unthrottle = list_del_leaf_cfs_rq(cfs_rq);
}
cfs_rq->throttle_count++;
From: Sachin Sant <hidden> Date: 2021-06-21 17:32:10
quoted
quoted
Any thoughts Vincent?
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
Could you confirm that this patch fixes the problem for you too ?
Thanks for the fix.
The patch fixes the reported problem. The test ran to completion without
any failure.
Reported-by: Sachin Sant <redacted>
Tested-by: Sachin Sant <redacted>
-Sachin
From: Vincent Guittot <vincent.guittot@linaro.org> Date: 2021-06-21 17:44:31
On Mon, 21 Jun 2021 at 19:32, Sachin Sant [off-list ref] wrote:
quoted
quoted
quoted
Any thoughts Vincent?
I would prefer that we use the reason of adding the cfs in the list instead.
Something like the below should also fixed the problem. It is based on a
proposal I made to Rik sometimes ago when he tried to flatten the rq:
https://lore.kernel.org/lkml/20190906191237.27006-6-riel@surriel.com/
This will ensure that a cfs is added in the list whenever one of its child
is still in the list.
Could you confirm that this patch fixes the problem for you too ?
Thanks for the fix.
The patch fixes the reported problem. The test ran to completion without
any failure.
Reported-by: Sachin Sant <redacted>
Tested-by: Sachin Sant <redacted>