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

On 8/4/26 22:56, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Aug 04, 2026 at 10:41:25PM +0200, Maarten Lankhorst wrote:
>> 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
> 
> I wrote the fixup at [1] that included that css_put. I will look into the
> possibility of get_cg_pool_unlocked returning NULL or error. But I find
> that unconditionally calling css_put here is wrong. If pool is NULL, it
> needs to be called, because otherwise the css_tryget will be unbalanced.
> 
> The reason not to call it unconditionally is that it will be called when
> dmem_cgroup_pool_state_put is caled, hence, balancing the css_tryget.
> 
> I also need to consider whether doing it outside the RCU section is fine.

Why not? We acquire a reference within the RCU critical section, and if 
we have that reference we're obviously safe against concurrent frees, 
because the refcount is not zero (so it's invalid to free the object).

If css_tryget fails, the refcount was already zero and the css is about 
to be freed, in which case this does nothing. (Your initial fixup 
dropped another reference in that case, which is invalid as it pretty 
much guarantees a refcount underflow.)

> 
> I just looked here and one of the possibilities for pool to be NULL is that
> the region is unregistered in a race condition. region->unregistered is
> protected by dmemcg_lock, but that is taken by get_cg_pool_unlocked, so it
> seems possible.
> 
> By the way, while testing the whole series, I see many different issues
> here that I am still looking at as they may be otherwise hidden
> pre-existing bugs.

What's "many different" issues? I'm aware of your testing showing one 
potential issue, which is that dmem.min protection may evict own cgroups 
instead of others' cgroups. I didn't do anything about that in this 
series because I think it's a pre-existing issue that I'm fairly sure is 
at least theoretically triggerable without my patchset.

Are there any other issues I missed?

Regards,
Natalie

> 
> Regards.
> Cascardo.
> 
> [1] https://lore.kernel.org/cgroups/ahW8MY4XBox_nsmB-3vnzkFMtm8r7+2uJcM4HgikDNQbJ12ryKSQPXMW4xno@public.gmane.org/