Re: [PATCH v8 2/6] cgroup,cgroup/dmem: Add (dmem_)cgroup_common_ancestor helper

Hey,

On 8/4/26 22:26, Natalie Vock wrote:
> On 8/4/26 22:18, Maarten Lankhorst wrote:
>>
>>
>> On 8/4/26 10:25, Natalie Vock wrote:
>>> This helps to find a common subtree of two resources, which is important
>>> when determining whether it's helpful to evict one resource in favor of
>>> another.
>>>
>>> To facilitate this, add a common helper to find the ancestor of two
>>> cgroups using each cgroup's ancestor array.
>>>
>>> Signed-off-by: Natalie Vock <[email protected]>
>>> ---
>>>   include/linux/cgroup.h      | 21 +++++++++++++++++++++
>>>   include/linux/cgroup_dmem.h |  9 +++++++++
>>>   kernel/cgroup/dmem.c        | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 71 insertions(+)
>>>
>>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>>> index f2aa46a4f871e..83a17ded1c516 100644
>>> --- a/include/linux/cgroup.h
>>> +++ b/include/linux/cgroup.h
>>> @@ -623,6 +623,27 @@ static inline struct cgroup *cgroup_ancestor(struct cgroup *cgrp,
>>>       return cgrp->ancestors[ancestor_level];
>>>   }
>>>   +/**
>>> + * cgroup_common_ancestor - find common ancestor of two cgroups
>>> + * @a: first cgroup to find common ancestor of
>>> + * @b: second cgroup to find common ancestor of
>>> + *
>>> + * Find the first cgroup that is an ancestor of both @a and @b, if it exists
>>> + * and return a pointer to it. If such a cgroup doesn't exist, return NULL.
>>> + *
>>> + * This function is safe to call as long as both @a and @b are accessible.
>>> + */
>>> +static inline struct cgroup *cgroup_common_ancestor(struct cgroup *a,
>>> +                            struct cgroup *b)
>>> +{
>>> +    int level;
>>> +
>>> +    for (level = min(a->level, b->level); level >= 0; level--)
>>> +        if (a->ancestors[level] == b->ancestors[level])
>>> +            return a->ancestors[level];
>>> +    return NULL;
>>> +}
>>> +
>>>   /**
>>>    * task_under_cgroup_hierarchy - test task's membership of cgroup ancestry
>>>    * @task: the task to be tested
>>> diff --git a/include/linux/cgroup_dmem.h b/include/linux/cgroup_dmem.h
>>> index 1a88cd0c9eb00..9d72457c4cb9d 100644
>>> --- a/include/linux/cgroup_dmem.h
>>> +++ b/include/linux/cgroup_dmem.h
>>> @@ -28,6 +28,8 @@ bool dmem_cgroup_below_min(struct dmem_cgroup_pool_state *root,
>>>                  struct dmem_cgroup_pool_state *test);
>>>   bool dmem_cgroup_below_low(struct dmem_cgroup_pool_state *root,
>>>                  struct dmem_cgroup_pool_state *test);
>>> +struct dmem_cgroup_pool_state *dmem_cgroup_get_common_ancestor(struct dmem_cgroup_pool_state *a,
>>> +                                   struct dmem_cgroup_pool_state *b);
>>>     void dmem_cgroup_pool_state_put(struct dmem_cgroup_pool_state *pool);
>>>   #else
>>> @@ -75,6 +77,13 @@ static inline bool dmem_cgroup_below_low(struct dmem_cgroup_pool_state *root,
>>>       return false;
>>>   }
>>>   +static inline
>>> +struct dmem_cgroup_pool_state *dmem_cgroup_get_common_ancestor(struct dmem_cgroup_pool_state *a,
>>> +                                   struct dmem_cgroup_pool_state *b)
>>> +{
>>> +    return NULL;
>>> +}
>>> +
>>>   static inline void dmem_cgroup_pool_state_put(struct dmem_cgroup_pool_state *pool)
>>>   { }
>>>   diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
>>> index 9df3b33c65046..a587611ca2235 100644
>>> --- a/kernel/cgroup/dmem.c
>>> +++ b/kernel/cgroup/dmem.c
>>> @@ -762,6 +762,47 @@ bool dmem_cgroup_below_low(struct dmem_cgroup_pool_state *root,
>>>   }
>>>   EXPORT_SYMBOL_GPL(dmem_cgroup_below_low);
>>>   +/**
>>> + * dmem_cgroup_get_common_ancestor(): Find the first common ancestor of two pools.
>>> + * @a: First pool to find the common ancestor of.
>>> + * @b: First pool to find the common ancestor of.
>>> + *
>>> + * Return: The first pool that is a parent of both @a and @b, or NULL if either @a or @b are NULL,
>>> + * or if such a pool does not exist. A reference to the returned pool is grabbed and must be
>>> + * released by the caller when it is done using the pool.
>>> + */
>>> +struct dmem_cgroup_pool_state *dmem_cgroup_get_common_ancestor(struct dmem_cgroup_pool_state *a,
>>> +                                   struct dmem_cgroup_pool_state *b)
>>> +{
>>> +    struct cgroup *ancestor_cgroup;
>>> +    struct cgroup_subsys_state *ancestor_css;
>>> +    struct dmemcg_state *ancestor_dmemcs = NULL;
>>> +    struct dmem_cgroup_pool_state *pool = NULL;
>>> +
>>> +    if (!a || !b)
>>> +        return NULL;
>>> +
>>> +    ancestor_cgroup = cgroup_common_ancestor(a->cs->css.cgroup, b->cs->css.cgroup);
>>> +    if (!ancestor_cgroup)
>>> +        return NULL;
>>> +
>>> +    rcu_read_lock();
>>> +    ancestor_css = cgroup_e_css(ancestor_cgroup, &dmem_cgrp_subsys);
>>> +    if (css_tryget(ancestor_css))
>>> +        ancestor_dmemcs = css_to_dmemcs(ancestor_css);
>>> +    rcu_read_unlock();
>>> +
>>> +    if (ancestor_dmemcs) {
>>> +        pool = get_cg_pool_unlocked(css_to_dmemcs(ancestor_css),
>>> +                        a->region);
>>> +        if (IS_ERR(pool))
>>> +            pool = NULL;
>> This should probably be a warn_on as this can never happen, sashiko is wrong here.
>>
>> If a and b are charged, all their ancestors are charged too. And when they share a
>> common ancestor, then the common ancestor has to have an existing charged pool as
>> well because there's a huge bug in the code otherwise.
>>
>>> +        css_put(ancestor_css);
>>> +    }
>>> +    return pool;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dmem_cgroup_get_common_ancestor);
>>> +
>>>   static int dmem_cgroup_region_capacity_show(struct seq_file *sf, void *v)
>>>   {
>>>       struct dmem_cgroup_region *region;
>>>
>>
>> Otherwise looks good, so feel free to extend my r-b to this patch too, and with that minor fix up commit it.
> 
> Thanks a lot! Correct me if I'm wrong here as it's been a while, but I think sashiko's current comment about the css_put(ancestor_css) being wrong seems to have merit too? Since dmem_cgroup_pool_state_put() also puts a css reference but get_cg_pool_unlocked does not get another reference to the css on its own? (Kind of a footgun if you ask me, but not something for this series)
> 
> Mind if I also drop the css_put() line while I'm at it and *then* (finally :D) commit?
I'd keep the css_put for the failure case then.

Since the get_cg_pool_unlocked() is a local function, it's less of an issue that it doesn't
obtain a reference itself, as all callers outside dmemcg.c never notice.

Maybe do a final round of testing too, in case it introduces new failures. :-)

Especially device unload/reload might show double puts. But otherwise looks good to me.

Kind regards,
~Maarten