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

On Tue, Aug 04, 2026 at 07:07:07PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Aug 04, 2026 at 11:08:12PM +0200, Natalie Vock wrote:
> > 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.)
> > 
> 
> Well, I meant that I was not sure about it. As my version did it under the
> RCU lock, I wanted to concede my past self some benefit of the doubt, but I
> could not reason about it while I was writing down this email. At least
> under my tests back then, I did not notice any issues, but I will revisit
> it and I even noticed that get_cg_pool_unlocked takes the RCU lock, so this
> seems to be fine from a first look. As you pointed out, we have already
> picked the css ref under the RCU lock.
> 
> > > 
> > > 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?
> > 
> 
> Well, for one, there is:
> 
> [ 1285.795196] ------------[ cut here ]------------
> [ 1285.795239] percpu ref (css_release) <= 0 (-13) after switching to atomic
> [ 1285.795244] WARNING: lib/percpu-refcount.c:197 at percpu_ref_switch_to_atomic_rcu+0x22f/0x280, CPU#4: ksoftirqd/4/31
> [...]
> [ 1285.799577] percpu_ref_switch_to_atomic_rcu: percpu_ref_switch_to_atomic_rcu(): percpu_ref underflow slab kmalloc-64 start ffff889279463dc0 pointer offset 0 size 64
> 
> Which I now would attribute to this unconditional css_put. As I pointed
> out, dmem_cgroup_pool_state_put already puts it (which is why we also
> needed to get it in the first place).
> 
> And, then, I am seeing hung tasks and kmemleaks, which could be the result
> of the failing VRAM allocations for VM BOs from a quick read of the stack.
> 
> Cascardo.
> 

With the following fixup, I see no ref underflows, no hung tasks and no
kmemleaks.

With that:

Tested-by: Thadeu Lima de Souza Cascardo <[email protected]>


> 
> > Regards,
> > Natalie
> > 
> > > 
> > > Regards.
> > > Cascardo.
> > > 
> > > [1] https://lore.kernel.org/cgroups/[email protected]/
> > 

diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
index e372ebb5d36d..440e437d1bfe 100644
--- a/kernel/cgroup/dmem.c
+++ b/kernel/cgroup/dmem.c
@@ -790,9 +790,8 @@ struct dmem_cgroup_pool_state *dmem_cgroup_get_common_ancestor(struct dmem_cgrou
 	if (ancestor_dmemcs) {
 		pool = get_cg_pool_unlocked(css_to_dmemcs(ancestor_css),
 					    a->region);
-		if (IS_ERR(pool))
-			pool = NULL;
-		css_put(ancestor_css);
+		if (WARN_ON(IS_ERR_OR_NULL(pool)))
+			css_put(ancestor_css);
 	}
 	return pool;
 }
--