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

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?

Natalie

> 
> Reviewed-by: Maarten Lankhorst <[email protected]>
>