Re: [PATCH v8 1/6] cgroup/dmem: Add queries for protection values

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing ancestry check can result in using stale protection values for non-descendants.

Pre-existing issues:
- [High] O(N) cgroup tree traversal per isolated query causes massive overhead on eviction paths.
--

--- Patch [1]: [PATCH v8 1/6] cgroup/dmem: Add queries for protection values ---
commit def4bae1e53d26c789e6edbd5c551a0f50ade339
Author: Natalie Vock <[email protected]>

cgroup/dmem: Add queries for protection values
    
Callers can use this feedback to be more aggressive in making space for
allocations of a cgroup if they know it is protected.
    
These are counterparts to memcg's mem_cgroup_below_{min,low}.

> diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
> index 39930c59cb769..9df3b33c65046 100644
> --- a/kernel/cgroup/dmem.c
> +++ b/kernel/cgroup/dmem.c

[ ... ]

> @@ -700,6 +700,68 @@ int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
>  }
>  EXPORT_SYMBOL_GPL(dmem_cgroup_try_charge);
>  
> +/**
> + * dmem_cgroup_below_min() - Tests whether current usage is within min limit.
> + *
> + * @root: Root of the subtree to calculate protection for, or NULL to calculate global protection.
> + * @test: The pool to test the usage/min limit of.
> + *
> + * Return: true if usage is below min and the cgroup is protected, false otherwise.
> + */
> +bool dmem_cgroup_below_min(struct dmem_cgroup_pool_state *root,
> +			   struct dmem_cgroup_pool_state *test)
> +{
> +	if (root == test || !pool_parent(test))
> +		return false;
> +
> +	if (!root) {
> +		for (root = test; pool_parent(root); root = pool_parent(root))
> +			{}
> +	}
> +
> +	/*
> +	 * In mem_cgroup_below_min(), the memcg pendant, this call is missing.
> +	 * mem_cgroup_below_min() gets called during traversal of the cgroup tree, where
> +	 * protection is already calculated as part of the traversal. dmem cgroup eviction
> +	 * does not traverse the cgroup tree, so we need to recalculate effective protection
> +	 * here.
> +	 */
> +	dmem_cgroup_calculate_protection(root, test);

[Severity: High]
This isn't a bug introduced by this patch, but does calling 
dmem_cgroup_calculate_protection() here for isolated per-item queries cause 
massive overhead on eviction paths?

Looking at the pre-existing dmem_cgroup_calculate_protection() implementation:

kernel/cgroup/dmem.c:dmem_cgroup_calculate_protection() {
    ...
    css_for_each_descendant_pre(css, &limit_pool->cs->css) {
        ...
        page_counter_calculate_protection(climit, &found_pool->cnt, true);

        if (found_pool == test_pool)
            break;
    }
    ...
}

It performs a top-down pre-order traversal starting from root. When root is 
NULL and falls back to the global root, it forces a traversal over almost the 
entire system's cgroups. Executing this full subtree walk repeatedly during 
eviction scanning could lead to severe performance degradation.

> +	return page_counter_read(&test->cnt) <= READ_ONCE(test->cnt.emin);

[Severity: High]
If the caller passes a root and a test pool where test is not a descendant 
of root, dmem_cgroup_calculate_protection() will iterate only the descendants 
of root, never find test, and exit without updating test->cnt.emin.

Could this cause the function to read a stale emin value from a prior 
calculation and incorrectly return true, incorrectly shielding the cgroup 
from eviction?

Since dmem uses global lists and doesn't have per-cgroup LRUs, encountering 
non-descendants during eviction seems likely. Would it be safer to verify 
that test is a descendant of root and return false otherwise?

> +}
> +EXPORT_SYMBOL_GPL(dmem_cgroup_below_min);
> +
> +/**
> + * dmem_cgroup_below_low() - Tests whether current usage is within low limit.
> + *
> + * @root: Root of the subtree to calculate protection for, or NULL to calculate global protection.
> + * @test: The pool to test the usage/low limit of.
> + *
> + * Return: true if usage is below low and the cgroup is protected, false otherwise.
> + */
> +bool dmem_cgroup_below_low(struct dmem_cgroup_pool_state *root,
> +			   struct dmem_cgroup_pool_state *test)
> +{
> +	if (root == test || !pool_parent(test))
> +		return false;
> +
> +	if (!root) {
> +		for (root = test; pool_parent(root); root = pool_parent(root))
> +			{}
> +	}
> +
> +	/*
> +	 * In mem_cgroup_below_low(), the memcg pendant, this call is missing.
> +	 * mem_cgroup_below_low() gets called during traversal of the cgroup tree, where
> +	 * protection is already calculated as part of the traversal. dmem cgroup eviction
> +	 * does not traverse the cgroup tree, so we need to recalculate effective protection
> +	 * here.
> +	 */
> +	dmem_cgroup_calculate_protection(root, test);
> +	return page_counter_read(&test->cnt) <= READ_ONCE(test->cnt.elow);

[Severity: High]
Similarly to dmem_cgroup_below_min() above, does this missing ancestry check 
also expose dmem_cgroup_below_low() to using stale elow protection values if 
test is not a descendant of root?

> +}
> +EXPORT_SYMBOL_GPL(dmem_cgroup_below_low);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1