| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210621162243.GA29874@vingu-book>
Date: Mon, 21 Jun 2021 18:22:43 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Odin Ugedal <odin@...d.al>
Cc: Sachin Sant <sachinp@...ux.vnet.ibm.com>,
open list <linux-kernel@...r.kernel.org>,
linuxppc-dev@...ts.ozlabs.org,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [powerpc][5.13.0-rc7] Kernel warning (kernel/sched/fair.c:401)
while running LTP tests
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(+)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ea7de54cb022..e751061a9449 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3272,6 +3272,31 @@ static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq, int flags)
#ifdef CONFIG_SMP
#ifdef CONFIG_FAIR_GROUP_SCHED
+/*
+ * Because list_add_leaf_cfs_rq always places a child cfs_rq on the list
+ * immediately before a parent cfs_rq, and cfs_rqs are removed from the list
+ * bottom-up, we only have to test whether the cfs_rq before us on the list
+ * is our child.
+ * If cfs_rq is not on the list, test wether a child needs its to be added to
+ * connect a branch to the tree * (see list_add_leaf_cfs_rq() for details).
+ */
+static inline bool child_cfs_rq_on_list(struct cfs_rq *cfs_rq)
+{
+ struct cfs_rq *prev_cfs_rq;
+ struct list_head *prev;
+
+ if (cfs_rq->on_list) {
+ prev = cfs_rq->leaf_cfs_rq_list.prev;
+ } else {
+ struct rq *rq = rq_of(cfs_rq);
+
+ prev = rq->tmp_alone_branch;
+ }
+
+ prev_cfs_rq = container_of(prev, struct cfs_rq, leaf_cfs_rq_list);
+
+ return (prev_cfs_rq->tg->parent == cfs_rq->tg);
+}
static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
{
@@ -3287,6 +3312,9 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
if (cfs_rq->avg.runnable_sum)
return false;
+ if (child_cfs_rq_on_list(cfs_rq))
+ return false;
+
return true;
}
--
2.17.1
>
> Thanks
> Odin
>
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index bfaa6e1f6067..aa32e9c29efd 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -376,7 +376,8 @@ static inline bool list_add_leaf_cfs_rq(struct
> cfs_rq *cfs_rq)
> return false;
> }
>
> -static inline void list_del_leaf_cfs_rq(struct cfs_rq *cfs_rq)
> +/* Returns 1 if cfs_rq was present in the list and removed */
> +static inline bool list_del_leaf_cfs_rq(struct cfs_rq *cfs_rq)
> {
> if (cfs_rq->on_list) {
> struct rq *rq = rq_of(cfs_rq);
> @@ -393,7 +394,9 @@ static inline void list_del_leaf_cfs_rq(struct
> cfs_rq *cfs_rq)
>
> list_del_rcu(&cfs_rq->leaf_cfs_rq_list);
> cfs_rq->on_list = 0;
> + return 1;
> }
> + return 0;
> }
>
> static inline void assert_list_leaf_cfs_rq(struct rq *rq)
> @@ -3298,24 +3301,6 @@ static inline void cfs_rq_util_change(struct
> cfs_rq *cfs_rq, int flags)
>
> #ifdef CONFIG_SMP
> #ifdef CONFIG_FAIR_GROUP_SCHED
> -
> -static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
> -{
> - if (cfs_rq->load.weight)
> - return false;
> -
> - if (cfs_rq->avg.load_sum)
> - return false;
> -
> - if (cfs_rq->avg.util_sum)
> - return false;
> -
> - if (cfs_rq->avg.runnable_sum)
> - return false;
> -
> - return true;
> -}
> -
> /**
> * update_tg_load_avg - update the tg's load avg
> * @cfs_rq: the cfs_rq whose avg changed
> @@ -4109,11 +4094,6 @@ static inline void update_misfit_status(struct
> task_struct *p, struct rq *rq)
>
> #else /* CONFIG_SMP */
>
> -static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
> -{
> - return true;
> -}
> -
> #define UPDATE_TG 0x0
> #define SKIP_AGE_LOAD 0x0
> #define DO_ATTACH 0x0
> @@ -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++;
>
> @@ -8019,6 +8000,23 @@ static bool __update_blocked_others(struct rq
> *rq, bool *done)
>
> #ifdef CONFIG_FAIR_GROUP_SCHED
>
> +static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
> +{
> + if (cfs_rq->load.weight)
> + return false;
> +
> + if (cfs_rq->avg.load_sum)
> + return false;
> +
> + if (cfs_rq->avg.util_sum)
> + return false;
> +
> + if (cfs_rq->avg.runnable_sum)
> + return false;
> +
> + return true;
> +}
> +
> static bool __update_blocked_fair(struct rq *rq, bool *done)
> {
> struct cfs_rq *cfs_rq, *pos;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index a189bec13729..12a707d99ee6 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -602,6 +602,7 @@ struct cfs_rq {
> u64 throttled_clock_task_time;
> int throttled;
> int throttle_count;
> + int insert_on_unthrottle;
> struct list_head throttled_list;
> #endif /* CONFIG_CFS_BANDWIDTH */
> #endif /* CONFIG_FAIR_GROUP_SCHED */
Powered by blists - more mailing lists