The member back in struct sched_rt_entity only related to RT_GROUP_SCHED,
it should not place out of RT_GROUP_SCHED, move back to RT_GROUP_SCHED
and rename it child.
Init child when parent isn't NULL in init_tg_rt_entry().
Introduce for_each_sched_rt_entity_reverse() to iterate rt_se from
top to down.
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202308031034.4369b15b-oliver.sang@intel.com
---
V3 -> V4: Missed rt_se = root in dequeue_rt_stack().
V2 -> V3: Keep parent is NULL in init_tg_rt_entry().
V1 -> V2: Add WARN_ON_ONCE in init_tg_rt_entry().
---
include/linux/sched.h | 2 +-
kernel/sched/rt.c | 25 ++++++++++++++++---------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 177b3f3676ef..5635655d6c35 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -594,8 +594,8 @@ struct sched_rt_entity {
unsigned short on_rq;
unsigned short on_list;
- struct sched_rt_entity *back;
#ifdef CONFIG_RT_GROUP_SCHED
+ struct sched_rt_entity *child;
struct sched_rt_entity *parent;
/* rq on which this entity is (to be) queued: */
struct rt_rq *rt_rq;
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 00e0e5074115..5e3edd7b8be4 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -230,8 +230,10 @@ void init_tg_rt_entry(struct task_group *tg, struct rt_rq *rt_rq,
if (!parent)
rt_se->rt_rq = &rq->rt;
- else
+ else {
rt_se->rt_rq = parent->my_q;
+ parent->child = rt_se;
+ }
rt_se->my_q = rt_rq;
rt_se->parent = parent;
@@ -564,6 +566,9 @@ static inline struct task_group *next_task_group(struct task_group *tg)
#define for_each_sched_rt_entity(rt_se) \
for (; rt_se; rt_se = rt_se->parent)
+#define for_each_sched_rt_entity_reverse(rt_se) \
+ for (; rt_se; rt_se = rt_se->child)
+
static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
{
return rt_se->my_q;
@@ -669,6 +674,9 @@ typedef struct rt_rq *rt_rq_iter_t;
#define for_each_sched_rt_entity(rt_se) \
for (; rt_se; rt_se = NULL)
+#define for_each_sched_rt_entity_reverse(rt_se) \
+ for_each_sched_rt_entity(rt_se)
+
static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
{
return NULL;
@@ -1481,22 +1489,21 @@ static void __dequeue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flag
*/
static void dequeue_rt_stack(struct sched_rt_entity *rt_se, unsigned int flags)
{
- struct sched_rt_entity *back = NULL;
+ struct sched_rt_entity *root;
unsigned int rt_nr_running;
- for_each_sched_rt_entity(rt_se) {
- rt_se->back = back;
- back = rt_se;
- }
+ for_each_sched_rt_entity(rt_se)
+ root = rt_se;
- rt_nr_running = rt_rq_of_se(back)->rt_nr_running;
+ rt_nr_running = rt_rq_of_se(root)->rt_nr_running;
- for (rt_se = back; rt_se; rt_se = rt_se->back) {
+ rt_se = root;
+ for_each_sched_rt_entity_reverse(rt_se) {
if (on_rt_rq(rt_se))
__dequeue_rt_entity(rt_se, flags);
}
- dequeue_top_rt_rq(rt_rq_of_se(back), rt_nr_running);
+ dequeue_top_rt_rq(rt_rq_of_se(root), rt_nr_running);
}
static void enqueue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flags)
--
2.25.1On Thu, Aug 03, 2023 at 01:03:17PM +0800, Yajun Deng wrote:
> @@ -564,6 +566,9 @@ static inline struct task_group *next_task_group(struct task_group *tg)
> #define for_each_sched_rt_entity(rt_se) \
> for (; rt_se; rt_se = rt_se->parent)
>
> +#define for_each_sched_rt_entity_reverse(rt_se) \
> + for (; rt_se; rt_se = rt_se->child)
> +
> static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
> {
> return rt_se->my_q;
> @@ -669,6 +674,9 @@ typedef struct rt_rq *rt_rq_iter_t;
> #define for_each_sched_rt_entity(rt_se) \
> for (; rt_se; rt_se = NULL)
>
> +#define for_each_sched_rt_entity_reverse(rt_se) \
> + for_each_sched_rt_entity(rt_se)
> +
> static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
> {
> return NULL;
> @@ -1481,22 +1489,21 @@ static void __dequeue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flag
> */
> static void dequeue_rt_stack(struct sched_rt_entity *rt_se, unsigned int flags)
> {
> - struct sched_rt_entity *back = NULL;
> + struct sched_rt_entity *root;
> unsigned int rt_nr_running;
>
> - for_each_sched_rt_entity(rt_se) {
> - rt_se->back = back;
> - back = rt_se;
> - }
> + for_each_sched_rt_entity(rt_se)
> + root = rt_se;
>
> - rt_nr_running = rt_rq_of_se(back)->rt_nr_running;
> + rt_nr_running = rt_rq_of_se(root)->rt_nr_running;
>
> - for (rt_se = back; rt_se; rt_se = rt_se->back) {
> + rt_se = root;
> + for_each_sched_rt_entity_reverse(rt_se) {
> if (on_rt_rq(rt_se))
> __dequeue_rt_entity(rt_se, flags);
> }
>
> - dequeue_top_rt_rq(rt_rq_of_se(back), rt_nr_running);
> + dequeue_top_rt_rq(rt_rq_of_se(root), rt_nr_running);
> }
Urgh, please don't do this. The whole thing is super fragile. Creating
these abstractions makes it appear like
for_each_sched_rt_entity_reverse() is somehow a sane thing to do. Aside
from the name being insanely long, the whole thing makes no sense
what-so-ever unless you first went and did the normal iteration and set
the back pointers.
It's called back for a reason, it walks back the path it first walked.
Normals and unambiguous iteration is up the tree, towards the root,
walking back down is not.
* Yajun Deng <yajun.deng@linux.dev> wrote: > The member back in struct sched_rt_entity only related to RT_GROUP_SCHED, > it should not place out of RT_GROUP_SCHED, move back to RT_GROUP_SCHED > and rename it child. > > Init child when parent isn't NULL in init_tg_rt_entry(). > > Introduce for_each_sched_rt_entity_reverse() to iterate rt_se from > top to down. > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202308031034.4369b15b-oliver.sang@intel.com Yeah, so I agree with these changes, but could you please split up this patch into 3 separate patches: sched/rt: Move sched_rt_entity::back to under the CONFIG_RT_GROUP_SCHED block sched/rt: Rename sched_rt_entity::back to sched_rt_entity::child sched/rt: Introduce for_each_sched_rt_entity_reverse() & use it Thanks, Ingo
On 2023/10/3 17:51, Ingo Molnar wrote: > * Yajun Deng <yajun.deng@linux.dev> wrote: > >> The member back in struct sched_rt_entity only related to RT_GROUP_SCHED, >> it should not place out of RT_GROUP_SCHED, move back to RT_GROUP_SCHED >> and rename it child. >> >> Init child when parent isn't NULL in init_tg_rt_entry(). >> >> Introduce for_each_sched_rt_entity_reverse() to iterate rt_se from >> top to down. >> >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >> Reported-by: kernel test robot <oliver.sang@intel.com> >> Closes: https://lore.kernel.org/oe-lkp/202308031034.4369b15b-oliver.sang@intel.com > Yeah, so I agree with these changes, but could you please split up this > patch into 3 separate patches: > > sched/rt: Move sched_rt_entity::back to under the CONFIG_RT_GROUP_SCHED block > sched/rt: Rename sched_rt_entity::back to sched_rt_entity::child > sched/rt: Introduce for_each_sched_rt_entity_reverse() & use it Okay, This patch makes sense, it saves a few bytes. But Peter seems to have a different opinion. @Peter, I think I split up this patch into 2 separate patches: sched/rt: Introduce for_each_sched_rt_entity_back() & use it sched/rt: Move sched_rt_entity::back to under the CONFIG_RT_GROUP_SCHED block Comments, please... > > Thanks, > > Ingo
On Tue, Oct 03, 2023 at 10:55:50PM +0800, Yajun Deng wrote: > @Peter, I think I split up this patch into 2 separate patches: > > sched/rt: Introduce for_each_sched_rt_entity_back() & use it Why ?? Having that macro makes no sense what so ever. You can't use it, unless you set up the back pointers first. It is not a self contained piece of functionality. > sched/rt: Move sched_rt_entity::back to under the > CONFIG_RT_GROUP_SCHED block This one sure.
On 2023/10/4 00:37, Peter Zijlstra wrote:
> On Tue, Oct 03, 2023 at 10:55:50PM +0800, Yajun Deng wrote:
>
>> @Peter, I think I split up this patch into 2 separate patches:
>>
>> sched/rt: Introduce for_each_sched_rt_entity_back() & use it
> Why ?? Having that macro makes no sense what so ever. You can't use it,
> unless you set up the back pointers first. It is not a self contained
> piece of functionality.
The 2nd patch rely on this patch. It will be used in dequeue_rt_stack.
We need to add a macro first,
because the 'back' will be not exist after the 2nd patch when disable
CONFIG_RT_GROUP_SCHED.
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 88fc98601413..0b6b4a715d6e 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -520,6 +520,9 @@ static inline struct task_group
*next_task_group(struct task_group *tg)
#define for_each_sched_rt_entity(rt_se) \
for (; rt_se; rt_se = rt_se->parent)
+#define for_each_sched_rt_back(rt_se) \
+ for (; rt_se; rt_se = rt_se->back)
+
static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
{
return rt_se->my_q;
@@ -625,6 +628,9 @@ typedef struct rt_rq *rt_rq_iter_t;
#define for_each_sched_rt_entity(rt_se) \
for (; rt_se; rt_se = NULL)
+#define for_each_sched_rt_entity_back(rt_se) \
+ for_each_sched_rt_entity(rt_se)
+
static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
{
return NULL;
@@ -1445,7 +1451,8 @@ static void dequeue_rt_stack(struct
sched_rt_entity *rt_se, unsigned int flags)
rt_nr_running = rt_rq_of_se(back)->rt_nr_running;
- for (rt_se = back; rt_se; rt_se = rt_se->back) {
+ rt_se = back;
+ for_each_sched_rt_entity_back(rt_se) {
if (on_rt_rq(rt_se))
__dequeue_rt_entity(rt_se, flags);
}
>
>> sched/rt: Move sched_rt_entity::back to under the
>> CONFIG_RT_GROUP_SCHED block
> This one sure.
© 2016 - 2026 Red Hat, Inc.