Re: [PATCH 03/10] sched/fair: Add cgroup LCA finder for hierarchical yield

[Wanpeng Li]

Hi Prateek，

On Wed, 12 Nov 2025 at 14:50, K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
>
> Hello Wanpeng,
>
> On 11/10/2025 9:02 AM, Wanpeng Li wrote:
> > +/*
> > + * Find the lowest common ancestor (LCA) in the cgroup hierarchy for EEVDF.
> > + * We walk up both entity hierarchies under rq->lock protection.
> > + * Task migration requires task_rq_lock, ensuring parent chains remain stable.
> > + * We locate the first common cfs_rq where both entities coexist, representing
> > + * the appropriate level for vruntime adjustments and EEVDF field updates
> > + * (deadline, vlag) to maintain scheduler consistency.
> > + */
> > +static bool __maybe_unused yield_deboost_find_lca(struct sched_entity *se_y, struct sched_entity *se_t,
> > +                                 struct sched_entity **se_y_lca_out,
> > +                                 struct sched_entity **se_t_lca_out,
> > +                                 struct cfs_rq **cfs_rq_common_out)
> > +{
> > +     struct sched_entity *se_y_lca, *se_t_lca;
> > +     struct cfs_rq *cfs_rq_common;
> > +
> > +#ifdef CONFIG_FAIR_GROUP_SCHED
> > +     se_t_lca = se_t;
> > +     se_y_lca = se_y;
> > +
> > +     while (se_t_lca && se_y_lca && se_t_lca->depth != se_y_lca->depth) {
> > +             if (se_t_lca->depth > se_y_lca->depth)
> > +                     se_t_lca = se_t_lca->parent;
> > +             else
> > +                     se_y_lca = se_y_lca->parent;
> > +     }
> > +
> > +     while (se_t_lca && se_y_lca) {
> > +             if (cfs_rq_of(se_t_lca) == cfs_rq_of(se_y_lca)) {
> > +                     cfs_rq_common = cfs_rq_of(se_t_lca);
> > +                     goto found_lca;
> > +             }
> > +             se_t_lca = se_t_lca->parent;
> > +             se_y_lca = se_y_lca->parent;
> > +     }
> > +     return false;
> > +#else
> > +     if (cfs_rq_of(se_y) != cfs_rq_of(se_t))
> > +             return false;
> > +     cfs_rq_common = cfs_rq_of(se_y);
> > +     se_y_lca = se_y;
> > +     se_t_lca = se_t;
> > +#endif
> > +
> > +found_lca:
> > +     if (!se_y_lca || !se_t_lca)
> > +             return false;
>
> Can that even happen? They should meet at the root cfs_rq.

You're right. Tasks on the same rq will always meet at root cfs_rq at
worst, so the !se_y_lca || !se_t_lca check is indeed redundant.

> Also all of this seems to be just find_matching_se() from
> fair.c. Can't we just reuse that?

Yes, it does exactly what we need. The existing code duplicates its
depth-alignment and parent-walking logic. I'll replace our custom
LCA-finding with a call to find_matching_se(&se_y_lca, &se_t_lca) ,
then use cfs_rq_of(se_y_lca) to get the common cfs_rq.

>
> > +
> > +     if (cfs_rq_common->nr_queued <= 1)
> > +             return false;
> > +
> > +     if (!se_y_lca->slice)
> > +             return false;
>
> Is that even possible?

No, it's not possible. The check was defensive but unnecessary. As you
noted in question above, entities on the same rq must meet at root
cfs_rq at the latest, and the while loop condition se_t_lca &&
se_y_lca already ensures both are non-NULL before the goto found_lca .
Will remove this check.

>
> > +
> > +     *se_y_lca_out = se_y_lca;
> > +     *se_t_lca_out = se_t_lca;
> > +     *cfs_rq_common_out = cfs_rq_common;
>
> Again, find_matching_se() does pretty much similar thing
> and you can just use cfs_rq_of(se) to get the common cfs_rq.

Agreed. :)

Regards,
Wanpeng