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; } --
