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
