Re: [PATCH v6 1/3] sched/fair: Co-locate cfs_rq and sched_entity

[Josh Don]

Hi Zecheng,

Thanks for this, looks generally reasonable.

On Mon, Jan 12, 2026 at 10:51 AM Zecheng Li <zli94@xxxxxxxx> wrote:
>
> From: Zecheng Li <zecheng@xxxxxxxxxx>
>
> 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_rq_with_se that
> holds both objects in a single allocation.
>
> This patch:
>
> - Moves struct sched_entity_stats from stats.h to sched.h to allow
> cfs_rq_with_se to use it while avoiding include order issues.
>
> - Defines the new struct cfs_rq_with_se containing cfs_rq and
> sched_entity_stats for type safety.
>
> - 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@xxxxxxxxxx>
> Signed-off-by: Zecheng Li <zli94@xxxxxxxx>
> ---
>  kernel/sched/fair.c  | 25 +++++++++++--------------
>  kernel/sched/sched.h | 17 +++++++++++++++++
>  kernel/sched/stats.h |  7 -------
>  3 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7377f9117501..eef10f2ef2a9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13617,10 +13617,11 @@ void free_fair_sched_group(struct task_group *tg)
>         int i;
>
>         for_each_possible_cpu(i) {
> -               if (tg->cfs_rq)
> -                       kfree(tg->cfs_rq[i]);
> -               if (tg->se)
> -                       kfree(tg->se[i]);
> +               if (tg->cfs_rq && tg->cfs_rq[i]) {
> +                       struct cfs_rq_with_se *combined =
> +                               container_of(tg->cfs_rq[i], struct cfs_rq_with_se, cfs_rq);
> +                       kfree(combined);
> +               }
>         }
>
>         kfree(tg->cfs_rq);
> @@ -13629,6 +13630,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_rq_with_se *combined;
>         struct sched_entity *se;
>         struct cfs_rq *cfs_rq;
>         int i;
> @@ -13645,16 +13647,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)
> +               combined = kzalloc_node(sizeof(*combined),
> +                                       GFP_KERNEL, cpu_to_node(i));
> +               if (!combined)
>                         goto err;
>
> -               se = kzalloc_node(sizeof(struct sched_entity_stats),
> -                                 GFP_KERNEL, cpu_to_node(i));
> -               if (!se)
> -                       goto err_free_rq;
> -
> +               cfs_rq = &combined->cfs_rq;
> +               se = &combined->ses.se;
>                 init_cfs_rq(cfs_rq);
>                 init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
>                 init_entity_runnable_average(se);
> @@ -13662,8 +13661,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 3ceaa9dc9a9e..be32810f7475 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2172,6 +2172,23 @@ 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
> + * cfs_rq_with_se needs it but stats.h has dependencies on things
> + * defined later in this file.
> + */
> +struct sched_entity_stats {
> +       struct sched_entity     se;
> +       struct sched_statistics stats;
> +} __no_randomize_layout;

Now that we have a general purpose container struct (cfs_rq_with_se),
I wonder if it makes more sense to get rid of sched_entity_stats and
directly embed se and stats in cfs_rq_with_se.

> +
> +struct cfs_rq_with_se {
> +       struct cfs_rq                   cfs_rq;
> +       struct sched_entity_stats       ses;
> +};
> +#endif

nit: The struct name feels a bit awkward, perhaps something like tg_cfs_state?

> +
>  /* 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 c903f1a42891..6d80d5c7a202 100644
> --- a/kernel/sched/stats.h
> +++ b/kernel/sched/stats.h
> @@ -89,13 +89,6 @@ 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)
>  {
> --
> 2.52.0
>