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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-lkp/[email protected]
---
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.1
* Yajun Deng <[email protected]> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]
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 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.
On 2023/10/3 17:51, Ingo Molnar wrote:
> * Yajun Deng <[email protected]> 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 <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-lkp/[email protected]
> 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.