From: Zecheng Li <zecheng@google.com>
Improve data locality and reduce pointer chasing by allocating struct
cfs_rq and struct sched_entity together for non-root task groups. This
is achieved by introducing a new combined struct cfs_tg_state that
holds both objects in a single allocation.
This patch:
- Introduces struct cfs_tg_state that embeds cfs_rq, sched_entity, and
sched_statistics together in a single structure.
- Updates __schedstats_from_se() in stats.h to use cfs_tg_state for
accessing sched_statistics from a group sched_entity.
- Modifies alloc_fair_sched_group() and free_fair_sched_group() to
allocate and free the new struct as a single unit.
- Modifies the per-CPU pointers in task_group->se and task_group->cfs_rq
to point to the members in the new combined structure.
Signed-off-by: Zecheng Li <zecheng@google.com>
Signed-off-by: Zecheng Li <zli94@ncsu.edu>
Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
kernel/sched/fair.c | 18 ++++++------------
kernel/sched/sched.h | 12 ++++++++++++
kernel/sched/stats.h | 9 +--------
3 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0a35a82e4792..79160d419c9f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -13877,8 +13877,6 @@ void free_fair_sched_group(struct task_group *tg)
for_each_possible_cpu(i) {
if (tg->cfs_rq)
kfree(tg->cfs_rq[i]);
- if (tg->se)
- kfree(tg->se[i]);
}
kfree(tg->cfs_rq);
@@ -13887,6 +13885,7 @@ void free_fair_sched_group(struct task_group *tg)
int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
{
+ struct cfs_tg_state *state;
struct sched_entity *se;
struct cfs_rq *cfs_rq;
int i;
@@ -13903,16 +13902,13 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
init_cfs_bandwidth(tg_cfs_bandwidth(tg), tg_cfs_bandwidth(parent));
for_each_possible_cpu(i) {
- cfs_rq = kzalloc_node(sizeof(struct cfs_rq),
- GFP_KERNEL, cpu_to_node(i));
- if (!cfs_rq)
+ state = kzalloc_node(sizeof(*state),
+ GFP_KERNEL, cpu_to_node(i));
+ if (!state)
goto err;
- se = kzalloc_node(sizeof(struct sched_entity_stats),
- GFP_KERNEL, cpu_to_node(i));
- if (!se)
- goto err_free_rq;
-
+ cfs_rq = &state->cfs_rq;
+ se = &state->se;
init_cfs_rq(cfs_rq);
init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
init_entity_runnable_average(se);
@@ -13920,8 +13916,6 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
return 1;
-err_free_rq:
- kfree(cfs_rq);
err:
return 0;
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b863bbda6de8..826f21fa3f36 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2249,6 +2249,18 @@ static inline struct task_group *task_group(struct task_struct *p)
return p->sched_task_group;
}
+#ifdef CONFIG_FAIR_GROUP_SCHED
+/*
+ * Defined here to be available before stats.h is included, since
+ * stats.h has dependencies on things defined later in this file.
+ */
+struct cfs_tg_state {
+ struct cfs_rq cfs_rq;
+ struct sched_entity se;
+ struct sched_statistics stats;
+} __no_randomize_layout;
+#endif
+
/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
{
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index a612cf253c87..ebe0a7765f98 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -89,19 +89,12 @@ static inline void rq_sched_info_depart (struct rq *rq, unsigned long long delt
#endif /* CONFIG_SCHEDSTATS */
-#ifdef CONFIG_FAIR_GROUP_SCHED
-struct sched_entity_stats {
- struct sched_entity se;
- struct sched_statistics stats;
-} __no_randomize_layout;
-#endif
-
static inline struct sched_statistics *
__schedstats_from_se(struct sched_entity *se)
{
#ifdef CONFIG_FAIR_GROUP_SCHED
if (!entity_is_task(se))
- return &container_of(se, struct sched_entity_stats, se)->stats;
+ return &container_of(se, struct cfs_tg_state, se)->stats;
#endif
return &task_of(se)->stats;
}
--
2.53.0On Thu, Mar 19, 2026 at 10:52 AM Zecheng Li <zli94@ncsu.edu> wrote:
>
> From: Zecheng Li <zecheng@google.com>
>
> Improve data locality and reduce pointer chasing by allocating struct
> cfs_rq and struct sched_entity together for non-root task groups. This
> is achieved by introducing a new combined struct cfs_tg_state that
> holds both objects in a single allocation.
>
> This patch:
>
> - Introduces struct cfs_tg_state that embeds cfs_rq, sched_entity, and
> sched_statistics together in a single structure.
>
> - Updates __schedstats_from_se() in stats.h to use cfs_tg_state for
> accessing sched_statistics from a group sched_entity.
>
> - Modifies alloc_fair_sched_group() and free_fair_sched_group() to
> allocate and free the new struct as a single unit.
>
> - Modifies the per-CPU pointers in task_group->se and task_group->cfs_rq
> to point to the members in the new combined structure.
>
> Signed-off-by: Zecheng Li <zecheng@google.com>
> Signed-off-by: Zecheng Li <zli94@ncsu.edu>
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> kernel/sched/fair.c | 18 ++++++------------
> kernel/sched/sched.h | 12 ++++++++++++
> kernel/sched/stats.h | 9 +--------
> 3 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0a35a82e4792..79160d419c9f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13877,8 +13877,6 @@ void free_fair_sched_group(struct task_group *tg)
> for_each_possible_cpu(i) {
> if (tg->cfs_rq)
> kfree(tg->cfs_rq[i]);
> - if (tg->se)
> - kfree(tg->se[i]);
> }
To be more explicit, suggest converting tg->cfs_rq[i] to an explicit
cfs_tg_state pointer via container_of, to not depend on cfs_rq being
the first member of the struct.
>
> kfree(tg->cfs_rq);
> @@ -13887,6 +13885,7 @@ void free_fair_sched_group(struct task_group *tg)
>
> int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
> {
> + struct cfs_tg_state *state;
> struct sched_entity *se;
> struct cfs_rq *cfs_rq;
> int i;
> @@ -13903,16 +13902,13 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
> init_cfs_bandwidth(tg_cfs_bandwidth(tg), tg_cfs_bandwidth(parent));
>
> for_each_possible_cpu(i) {
> - cfs_rq = kzalloc_node(sizeof(struct cfs_rq),
> - GFP_KERNEL, cpu_to_node(i));
> - if (!cfs_rq)
> + state = kzalloc_node(sizeof(*state),
> + GFP_KERNEL, cpu_to_node(i));
> + if (!state)
> goto err;
>
> - se = kzalloc_node(sizeof(struct sched_entity_stats),
> - GFP_KERNEL, cpu_to_node(i));
> - if (!se)
> - goto err_free_rq;
> -
> + cfs_rq = &state->cfs_rq;
> + se = &state->se;
> init_cfs_rq(cfs_rq);
> init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
> init_entity_runnable_average(se);
> @@ -13920,8 +13916,6 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
>
> return 1;
>
> -err_free_rq:
> - kfree(cfs_rq);
> err:
> return 0;
> }
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b863bbda6de8..826f21fa3f36 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2249,6 +2249,18 @@ static inline struct task_group *task_group(struct task_struct *p)
> return p->sched_task_group;
> }
>
> +#ifdef CONFIG_FAIR_GROUP_SCHED
> +/*
> + * Defined here to be available before stats.h is included, since
> + * stats.h has dependencies on things defined later in this file.
> + */
> +struct cfs_tg_state {
> + struct cfs_rq cfs_rq;
> + struct sched_entity se;
> + struct sched_statistics stats;
> +} __no_randomize_layout;
> +#endif
> +
> /* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
> static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> {
> diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
> index a612cf253c87..ebe0a7765f98 100644
> --- a/kernel/sched/stats.h
> +++ b/kernel/sched/stats.h
> @@ -89,19 +89,12 @@ static inline void rq_sched_info_depart (struct rq *rq, unsigned long long delt
>
> #endif /* CONFIG_SCHEDSTATS */
>
> -#ifdef CONFIG_FAIR_GROUP_SCHED
> -struct sched_entity_stats {
> - struct sched_entity se;
> - struct sched_statistics stats;
> -} __no_randomize_layout;
> -#endif
> -
> static inline struct sched_statistics *
> __schedstats_from_se(struct sched_entity *se)
> {
> #ifdef CONFIG_FAIR_GROUP_SCHED
> if (!entity_is_task(se))
> - return &container_of(se, struct sched_entity_stats, se)->stats;
> + return &container_of(se, struct cfs_tg_state, se)->stats;
> #endif
> return &task_of(se)->stats;
> }
> --
> 2.53.0
>
Reviewed-by: Josh Don <joshdon@google.com>
Hello Josh,
On 4/1/2026 1:05 AM, Josh Don wrote:
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 0a35a82e4792..79160d419c9f 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -13877,8 +13877,6 @@ void free_fair_sched_group(struct task_group *tg)
>> for_each_possible_cpu(i) {
>> if (tg->cfs_rq)
>> kfree(tg->cfs_rq[i]);
>> - if (tg->se)
>> - kfree(tg->se[i]);
>> }
>
> To be more explicit, suggest converting tg->cfs_rq[i] to an explicit
> cfs_tg_state pointer via container_of, to not depend on cfs_rq being
> the first member of the struct.
I suggested the __no_randomize_layout trick since it is similar to what
"struct sched_entity_stats" does and we save on checking if
"tg->cfs_rq[i]" NULL before doing container_of() + kfree().
I isn't any more obscure than how kfree(tg->se[i]) would have previously
freed up the stats member too in one go but perhaps a comment on top
can address your concerns?
--
Thanks and Regards,
Prateek
On Tue, Mar 31, 2026 at 7:58 PM K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>
> Hello Josh,
>
> On 4/1/2026 1:05 AM, Josh Don wrote:
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 0a35a82e4792..79160d419c9f 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -13877,8 +13877,6 @@ void free_fair_sched_group(struct task_group *tg)
> >> for_each_possible_cpu(i) {
> >> if (tg->cfs_rq)
> >> kfree(tg->cfs_rq[i]);
> >> - if (tg->se)
> >> - kfree(tg->se[i]);
> >> }
> >
> > To be more explicit, suggest converting tg->cfs_rq[i] to an explicit
> > cfs_tg_state pointer via container_of, to not depend on cfs_rq being
> > the first member of the struct.
>
> I suggested the __no_randomize_layout trick since it is similar to what
> "struct sched_entity_stats" does and we save on checking if
> "tg->cfs_rq[i]" NULL before doing container_of() + kfree().
>
> I isn't any more obscure than how kfree(tg->se[i]) would have previously
> freed up the stats member too in one go but perhaps a comment on top
> can address your concerns?
This isn't a hot path that requires optimizing to that level, we're
talking about cgroup free here :)
The downside of the current approach is the brittleness of requiring
the field be the very first member of the struct +
no_randomize_layout. At minimum that needs a comment, but again I
struggle to see the justification for the complexity.
Up to Peter, but at minimum I think we need a comment on the struct if
we keep it as-is.
>
> --
> Thanks and Regards,
> Prateek
>
© 2016 - 2026 Red Hat, Inc.