[PATCH v8 0/6] cgroup/dmem,drm/ttm: Improve protection in contended cases

[PATCH v8 0/6] cgroup/dmem,drm/ttm: Improve protection in contended cases

[Natalie Vock]

Hi all,

I've been looking into some cases where dmem protection fails to prevent
allocations from ending up in GTT when VRAM gets scarce and apps start
competing hard.

In short, this is because other (unprotected) applications end up
filling VRAM before protected applications do. This causes TTM to back
off and try allocating in GTT before anything else, and that is where
the allocation is placed in the end. The existing eviction protection
cannot prevent this, because no attempt at evicting is ever made
(although you could consider the backing-off as an immediate eviction to
GTT).

This series tries to alleviate this by adding a special case when the
allocation is protected by cgroups: Instead of backing off immediately,
TTM will try evicting unprotected buffers from the domain to make space
for the protected one. This ensures that applications can actually use
all the memory protection awarded to them by the system, without being
prone to ping-ponging (only protected allocations can evict unprotected
ones, never the other way around).

The first two patches just add a few small utilities needed to implement
this to the dmem controller. The other patches are the TTM implementation:

"drm/ttm: Be more aggressive..." decouples cgroup charging from resource
allocation to allow us to hold on to the charge even if allocation fails
on first try, and adds a path to call ttm_bo_evict_alloc when the
charged allocation falls within min/low protection limits.

"drm/ttm: Use common ancestor..." is a more general improvement in
correctly implementing cgroup protection semantics. With recursive
protection rules, unused memory protection afforded to a parent node is
transferred to children recursively, which helps protect entire
subtrees from stealing each others' memory without needing to protect
each cgroup individually. This doesn't apply when considering direct
siblings inside the same subtree, so in order to not break
prioritization between these siblings, we need to consider the
relationship of evictor and evictee when calculating protection.
In practice, this fixes cases where a protected cgroup cannot steal
memory from unprotected siblings (which, in turn, leads to eviction
failures and new allocations being placed in GTT).

Thanks,
Natalie

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
---
Changes in v8:
- Fix a few Sashiko complaints
- Link to v7: https://patch.msgid.link/20260721-dmemcg-aggressive-protect-v7-0-97c4b1014315@gmx.de

Changes in v7:
- Fix incorrect refcounting/locking around dmem_cgroup_common_ancestor
  (Cascardo)
- Rebase on latest drm-misc-next
- Link to v6: https://lore.kernel.org/r/20260313-dmemcg-aggressive-protect-v6-0-7c71cc1492db@gmx.de

Changes in v6:
- Rename dmem_cgroup_common_ancestor to dmem_cgroup_get_common_ancestor
  (Maarten)
- Use ttm_bo_alloc_at_place in eviction cb as well (Tvrtko)
- Clean up the more aggressive eviction logic (Tvrtko)
- Link to v5: https://lore.kernel.org/r/20260302-dmemcg-aggressive-protect-v5-0-ffd3a2602309@gmx.de

Changes in v5:
- Added cgroup_common_ancestor helper to use with
  dmem_cgroup_common_ancestor (Tejun)
- Note: "drm/ttm: Use common ancestor..." needed minor changes since
  dmem_cgroup_common_ancestor now grabs a reference to the ancestor
  pool which needs to be dropped after use
- Removed extraneous whitespaces in "drm/ttm: Split cgroup charge..."
  and unnecessary changes done in "drm/ttm: Extract code..." (Tvrtko)
- Applied a comment from v3 about below_low not needing to be
  initialized in "drm/ttm: Be more aggressive..." (Tvrtko)
- Fixed uncharging the cgroup on allocation failure (Tvrtko)
- Fixed a typo in the message of "drm/ttm: Split cgroup charge..."
  (Tvrtko)
- Added case in ttm_bo_evict_cb for when charging fails, since we need
  to retry the charge (found myself)
- Link to v4: https://lore.kernel.org/r/20260225-dmemcg-aggressive-protect-v4-0-de847ab35184@gmx.de

Changes in v4:
- Split cgroup charge decoupling and eviction logic changes into
  separate commits (Tvrtko)
- Fix two cases of errno handling in ttm_bo_alloc_place and its caller
  (Tvrtko)
- Improve commit message/description of "drm/ttm: Make a helper..." (now
  "drm/ttm: Extract code...") (Tvrtko)
- Documentation improvements for new TTM eviction logic (Tvrtko)
- Formatting fixes (Tvrtko)
- Link to v3: https://lore.kernel.org/r/20251110-dmemcg-aggressive-protect-v3-0-219ffcfc54e9@gmx.de

Changes in v3:
- Improved documentation around cgroup queries and TTM eviction helpers
  (Maarten)
- Fixed up ttm_alloc_at_place charge failure logic to return either
  -EBUSY or -ENOSPC, not -EAGAIN (found this myself)
- Link to v2: https://lore.kernel.org/r/20251015-dmemcg-aggressive-protect-v2-0-36644fb4e37f@gmx.de

Changes in v2:
- Factored out the ttm logic for charging/allocating/evicting into a
  separate helper to keep things simpler
- Link to v1: https://lore.kernel.org/r/20250915-dmemcg-aggressive-protect-v1-0-2f3353bfcdac@gmx.de

---
Natalie Vock (6):
      cgroup/dmem: Add queries for protection values
      cgroup,cgroup/dmem: Add (dmem_)cgroup_common_ancestor helper
      drm/ttm: Extract code for attempting allocation in a place
      drm/ttm: Split cgroup charge and resource allocation
      drm/ttm: Be more aggressive when allocating below protection limit
      drm/ttm: Use common ancestor of evictor and evictee as limit pool

 drivers/gpu/drm/ttm/ttm_bo.c       | 223 ++++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/ttm/ttm_resource.c |  51 ++++++---
 include/drm/ttm/ttm_resource.h     |   6 +-
 include/linux/cgroup.h             |  21 ++++
 include/linux/cgroup_dmem.h        |  25 +++++
 kernel/cgroup/dmem.c               | 103 +++++++++++++++++
 6 files changed, 383 insertions(+), 46 deletions(-)
---
base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
change-id: 20250915-dmemcg-aggressive-protect-5cf37f717cdb

Best regards,
--  
Natalie Vock <natalie.vock@gmx.de>

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

[Natalie Vock]

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

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>
---
 include/linux/cgroup_dmem.h | 16 ++++++++++++
 kernel/cgroup/dmem.c        | 62 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/include/linux/cgroup_dmem.h b/include/linux/cgroup_dmem.h
index dd4869f1d736e..1a88cd0c9eb00 100644
--- a/include/linux/cgroup_dmem.h
+++ b/include/linux/cgroup_dmem.h
@@ -24,6 +24,10 @@ void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, u64 size);
 bool dmem_cgroup_state_evict_valuable(struct dmem_cgroup_pool_state *limit_pool,
 				      struct dmem_cgroup_pool_state *test_pool,
 				      bool ignore_low, bool *ret_hit_low);
+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);
 
 void dmem_cgroup_pool_state_put(struct dmem_cgroup_pool_state *pool);
 #else
@@ -59,6 +63,18 @@ bool dmem_cgroup_state_evict_valuable(struct dmem_cgroup_pool_state *limit_pool,
 	return true;
 }
 
+static inline bool dmem_cgroup_below_min(struct dmem_cgroup_pool_state *root,
+					 struct dmem_cgroup_pool_state *test)
+{
+	return false;
+}
+
+static inline bool dmem_cgroup_below_low(struct dmem_cgroup_pool_state *root,
+					 struct dmem_cgroup_pool_state *test)
+{
+	return false;
+}
+
 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 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);
+	return page_counter_read(&test->cnt) <= READ_ONCE(test->cnt.emin);
+}
+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);
+}
+EXPORT_SYMBOL_GPL(dmem_cgroup_below_low);
+
 static int dmem_cgroup_region_capacity_show(struct seq_file *sf, void *v)
 {
 	struct dmem_cgroup_region *region;

-- 
2.55.0

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

[Natalie Vock]

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 <natalie.vock@gmx.de>
---
 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;
+		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;

-- 
2.55.0

[PATCH v8 3/6] drm/ttm: Extract code for attempting allocation in a place

[Natalie Vock]

Move all code for attempting allocation for a specific place to
ttm_bo_alloc_place. With subsequent patches, this logic is going to get
more complicated, so it helps readability to have this separate.

ttm_bo_alloc_at_place takes a pointer to a struct ttm_bo_alloc_state.
This struct holds various state produced by the allocation (e.g. cgroup
resource associated with the allocation) that the caller needs to keep
track of (and potentially dispose of). This is just the limiting cgroup
pool for now, but future patches will add more state needing to be tracked.

ttm_bo_alloc_at_place also communicates via return codes if eviction
using ttm_bo_evict_alloc should be attempted. This is preparation for
attempting eviction in more cases than just force_space being set.

No functional change intended.

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 90 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 3980f376e3ba4..1cdb2172d5924 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -488,6 +488,51 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man
 	return ret;
 }
 
+struct ttm_bo_alloc_state {
+	/** @limit_pool: Which pool limit we should test against */
+	struct dmem_cgroup_pool_state *limit_pool;
+};
+
+/**
+ * ttm_bo_alloc_at_place - Attempt allocating a BO's backing store in a place
+ *
+ * @bo: The buffer to allocate the backing store of
+ * @place: The place to attempt allocation in
+ * @ctx: ttm_operation_ctx associated with this allocation
+ * @force_space: If we should evict buffers to force space
+ * @res: On allocation success, the resulting struct ttm_resource.
+ * @alloc_state: Object holding allocation state such as charged cgroups.
+ *
+ * Returns:
+ * -EBUSY: No space available, but allocation should be retried with ttm_bo_evict_alloc.
+ * -ENOSPC: No space available, allocation should not be retried.
+ * -ERESTARTSYS: An interruptible sleep was interrupted by a signal.
+ *
+ */
+static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
+				 const struct ttm_place *place,
+				 bool force_space,
+				 struct ttm_resource **res,
+				 struct ttm_bo_alloc_state *alloc_state)
+{
+	bool may_evict;
+	int ret;
+
+	may_evict = force_space && place->mem_type != TTM_PL_SYSTEM;
+
+	ret = ttm_resource_alloc(bo, place, res,
+				 force_space ? &alloc_state->limit_pool : NULL);
+
+	if (ret) {
+		if (ret == -ENOSPC && may_evict)
+			return -EBUSY;
+
+		return ret;
+	}
+
+	return 0;
+}
+
 /**
  * struct ttm_bo_evict_walk - Parameters for the evict walk.
  */
@@ -503,12 +548,13 @@ struct ttm_bo_evict_walk {
 	/** @evicted: Number of successful evictions. */
 	unsigned long evicted;
 
-	/** @limit_pool: Which pool limit we should test against */
-	struct dmem_cgroup_pool_state *limit_pool;
 	/** @try_low: Whether we should attempt to evict BO's with low watermark threshold */
 	bool try_low;
 	/** @hit_low: If we cannot evict a bo when @try_low is false (first pass) */
 	bool hit_low;
+
+	/** @alloc_state: State associated with the allocation attempt. */
+	struct ttm_bo_alloc_state *alloc_state;
 };
 
 static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
@@ -517,8 +563,9 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
 		container_of(walk, typeof(*evict_walk), walk);
 	s64 lret;
 
-	if (!dmem_cgroup_state_evict_valuable(evict_walk->limit_pool, bo->resource->css,
-					      evict_walk->try_low, &evict_walk->hit_low))
+	if (!dmem_cgroup_state_evict_valuable(evict_walk->alloc_state->limit_pool,
+					      bo->resource->css, evict_walk->try_low,
+					      &evict_walk->hit_low))
 		return 0;
 
 	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
@@ -560,7 +607,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 			      struct ttm_operation_ctx *ctx,
 			      struct ww_acquire_ctx *ticket,
 			      struct ttm_resource **res,
-			      struct dmem_cgroup_pool_state *limit_pool)
+			      struct ttm_bo_alloc_state *state)
 {
 	struct ttm_bo_evict_walk evict_walk = {
 		.walk = {
@@ -573,7 +620,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 		.place = place,
 		.evictor = evictor,
 		.res = res,
-		.limit_pool = limit_pool,
+		.alloc_state = state,
 	};
 	s64 lret;
 
@@ -724,9 +771,8 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
 
 	for (i = 0; i < placement->num_placement; ++i) {
 		const struct ttm_place *place = &placement->placement[i];
-		struct dmem_cgroup_pool_state *limit_pool = NULL;
+		struct ttm_bo_alloc_state alloc_state = {};
 		struct ttm_resource_manager *man;
-		bool may_evict;
 
 		man = ttm_manager_type(bdev, place->mem_type);
 		if (!man || !ttm_resource_manager_used(man))
@@ -736,25 +782,25 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
 				    TTM_PL_FLAG_FALLBACK))
 			continue;
 
-		may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM);
-		ret = ttm_resource_alloc(bo, place, res, force_space ? &limit_pool : NULL);
-		if (ret) {
-			if (ret != -ENOSPC) {
-				dmem_cgroup_pool_state_put(limit_pool);
-				return ret;
-			}
-			if (!may_evict) {
-				dmem_cgroup_pool_state_put(limit_pool);
-				continue;
-			}
+		ret = ttm_bo_alloc_at_place(bo, place, force_space, res,
+					    &alloc_state);
 
+		if (ret == -ENOSPC) {
+			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+			continue;
+		} else if (ret == -EBUSY) {
 			ret = ttm_bo_evict_alloc(bdev, man, place, bo, ctx,
-						 ticket, res, limit_pool);
-			dmem_cgroup_pool_state_put(limit_pool);
+						 ticket, res, &alloc_state);
+
+			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+
 			if (ret == -EBUSY)
 				continue;
-			if (ret)
+			else if (ret)
 				return ret;
+		} else if (ret) {
+			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+			return ret;
 		}
 
 		ret = ttm_bo_add_pipelined_eviction_fences(bo, man, ctx->no_wait_gpu);

-- 
2.55.0

[PATCH v8 4/6] drm/ttm: Split cgroup charge and resource allocation

[Natalie Vock]

Coupling resource allocation and cgroup charging is racy when charging
succeeds, but subsequent resource allocation fails. Certain eviction
decisions are made on the basis of whether the allocating cgroup is
protected, i.e. within its min/low limits, but with the charge being
tied to resource allocation (and uncharged when the resource allocation
fails), this check is done at a point where the allocation is not actually
charged to the cgroup.

This is subtly wrong if the allocation were to cause the cgroup to exceed
the min/low protection, but it's even more wrong if the same cgroup tries
allocating multiple buffers concurrently: In this case, the min/low
protection may pass for all allocation attempts when the real min/low
protection covers only some, or potentially none of the allocated
buffers.

Instead, charge the allocation to the cgroup once and keep the charge
for as long as we try to allocate a ttm_resource, and only undo the charge
if allocating the resource is ultimately unsuccessful and we move on to
a different ttm_place.

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/ttm/ttm_bo.c       | 57 ++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/ttm/ttm_resource.c | 51 +++++++++++++++++++++++-----------
 include/drm/ttm/ttm_resource.h     |  6 +++-
 3 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 1cdb2172d5924..3becddd2c43b8 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -489,8 +489,12 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man
 }
 
 struct ttm_bo_alloc_state {
+	/** @charge_pool: The memory pool the resource is charged to */
+	struct dmem_cgroup_pool_state *charge_pool;
 	/** @limit_pool: Which pool limit we should test against */
 	struct dmem_cgroup_pool_state *limit_pool;
+	/** @in_evict: Whether we are currently evicting buffers */
+	bool in_evict;
 };
 
 /**
@@ -518,18 +522,39 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
 	bool may_evict;
 	int ret;
 
-	may_evict = force_space && place->mem_type != TTM_PL_SYSTEM;
-
-	ret = ttm_resource_alloc(bo, place, res,
-				 force_space ? &alloc_state->limit_pool : NULL);
+	may_evict = !alloc_state->in_evict && force_space &&
+		    place->mem_type != TTM_PL_SYSTEM;
+	if (!alloc_state->charge_pool) {
+		ret = ttm_resource_try_charge(bo, place, &alloc_state->charge_pool,
+					      force_space ? &alloc_state->limit_pool
+							  : NULL);
+		if (ret) {
+			/*
+			 * -EAGAIN means the charge failed, which we treat
+			 * like an allocation failure. Therefore, return an
+			 * error code indicating the allocation failed -
+			 * either -EBUSY if the allocation should be
+			 * retried with eviction, or -ENOSPC if there should
+			 * be no second attempt.
+			 */
+			if (ret == -EAGAIN)
+				ret = may_evict ? -EBUSY : -ENOSPC;
+			return ret;
+		}
+	}
 
+	ret = ttm_resource_alloc(bo, place, res, alloc_state->charge_pool);
 	if (ret) {
 		if (ret == -ENOSPC && may_evict)
-			return -EBUSY;
-
+			ret = -EBUSY;
 		return ret;
 	}
 
+	/*
+	 * Ownership of charge_pool has been transferred to the TTM resource,
+	 * don't make the caller think we still hold a reference to it.
+	 */
+	alloc_state->charge_pool = NULL;
 	return 0;
 }
 
@@ -584,8 +609,10 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
 
 	evict_walk->evicted++;
 	if (evict_walk->res)
-		lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
-					  evict_walk->res, NULL);
+		lret = ttm_bo_alloc_at_place(evict_walk->evictor,
+					     evict_walk->place, false,
+					     evict_walk->res,
+					     evict_walk->alloc_state);
 	if (lret == 0)
 		return 1;
 out:
@@ -624,6 +651,8 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 	};
 	s64 lret;
 
+	state->in_evict = true;
+
 	evict_walk.walk.arg.trylock_only = true;
 	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
 
@@ -654,6 +683,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 		goto retry;
 	}
 out:
+	state->in_evict = false;
 	if (lret < 0)
 		return lret;
 	if (lret == 0)
@@ -786,6 +816,7 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
 					    &alloc_state);
 
 		if (ret == -ENOSPC) {
+			dmem_cgroup_uncharge(alloc_state.charge_pool, bo->base.size);
 			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
 			continue;
 		} else if (ret == -EBUSY) {
@@ -794,11 +825,15 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
 
 			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
 
-			if (ret == -EBUSY)
-				continue;
-			else if (ret)
+			if (ret) {
+				dmem_cgroup_uncharge(alloc_state.charge_pool,
+						bo->base.size);
+				if (ret == -EBUSY)
+					continue;
 				return ret;
+			}
 		} else if (ret) {
+			dmem_cgroup_uncharge(alloc_state.charge_pool, bo->base.size);
 			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
 			return ret;
 		}
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 154d6739256f8..02eca679cb68c 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -386,33 +386,52 @@ void ttm_resource_fini(struct ttm_resource_manager *man,
 }
 EXPORT_SYMBOL(ttm_resource_fini);
 
+/**
+ * ttm_resource_try_charge - charge a resource manager's cgroup pool
+ * @bo: buffer for which an allocation should be charged
+ * @place: where the allocation is attempted to be placed
+ * @ret_pool: on charge success, the pool that was charged
+ * @ret_limit_pool: on charge failure, the pool responsible for the failure
+ *
+ * Should be used to charge cgroups before attempting resource allocation.
+ * When charging succeeds, the value of ret_pool should be passed to
+ * ttm_resource_alloc.
+ *
+ * Returns: 0 on charge success, negative errno on failure.
+ */
+int ttm_resource_try_charge(struct ttm_buffer_object *bo,
+			    const struct ttm_place *place,
+			    struct dmem_cgroup_pool_state **ret_pool,
+			    struct dmem_cgroup_pool_state **ret_limit_pool)
+{
+	struct ttm_resource_manager *man =
+		ttm_manager_type(bo->bdev, place->mem_type);
+
+	if (!man->cg) {
+		*ret_pool = NULL;
+		if (ret_limit_pool)
+			*ret_limit_pool = NULL;
+		return 0;
+	}
+
+	return dmem_cgroup_try_charge(man->cg, bo->base.size, ret_pool,
+				      ret_limit_pool);
+}
+
 int ttm_resource_alloc(struct ttm_buffer_object *bo,
 		       const struct ttm_place *place,
 		       struct ttm_resource **res_ptr,
-		       struct dmem_cgroup_pool_state **ret_limit_pool)
+		       struct dmem_cgroup_pool_state *charge_pool)
 {
 	struct ttm_resource_manager *man =
 		ttm_manager_type(bo->bdev, place->mem_type);
-	struct dmem_cgroup_pool_state *pool = NULL;
 	int ret;
 
-	if (man->cg) {
-		ret = dmem_cgroup_try_charge(man->cg, bo->base.size, &pool, ret_limit_pool);
-		if (ret) {
-			if (ret == -EAGAIN)
-				ret = -ENOSPC;
-			return ret;
-		}
-	}
-
 	ret = man->func->alloc(man, bo, place, res_ptr);
-	if (ret) {
-		if (pool)
-			dmem_cgroup_uncharge(pool, bo->base.size);
+	if (ret)
 		return ret;
-	}
 
-	(*res_ptr)->css = pool;
+	(*res_ptr)->css = charge_pool;
 
 	spin_lock(&bo->bdev->lru_lock);
 	ttm_resource_add_bulk_move(*res_ptr, bo);
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index a5d386583fb6e..e567b7ec8218f 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -458,10 +458,14 @@ void ttm_resource_init(struct ttm_buffer_object *bo,
 void ttm_resource_fini(struct ttm_resource_manager *man,
 		       struct ttm_resource *res);
 
+int ttm_resource_try_charge(struct ttm_buffer_object *bo,
+			    const struct ttm_place *place,
+			    struct dmem_cgroup_pool_state **ret_pool,
+			    struct dmem_cgroup_pool_state **ret_limit_pool);
 int ttm_resource_alloc(struct ttm_buffer_object *bo,
 		       const struct ttm_place *place,
 		       struct ttm_resource **res,
-		       struct dmem_cgroup_pool_state **ret_limit_pool);
+		       struct dmem_cgroup_pool_state *charge_pool);
 void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
 bool ttm_resource_intersects(struct ttm_device *bdev,
 			     struct ttm_resource *res,

-- 
2.55.0

[PATCH v8 5/6] drm/ttm: Be more aggressive when allocating below protection limit

[Natalie Vock]

When the cgroup's memory usage is below the low/min limit and allocation
fails, try evicting some unprotected buffers to make space. Otherwise,
application buffers may be forced to go into GTT even though usage is
below the corresponding low/min limit, if other applications filled VRAM
with their allocations first.

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 53 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 3becddd2c43b8..6bbea148838e4 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -495,6 +495,10 @@ struct ttm_bo_alloc_state {
 	struct dmem_cgroup_pool_state *limit_pool;
 	/** @in_evict: Whether we are currently evicting buffers */
 	bool in_evict;
+	/** @may_try_low: If only unprotected BOs, i.e. BOs whose cgroup
+	 *  is exceeding its dmem low/min protection, should be considered for eviction
+	 */
+	bool may_try_low;
 };
 
 /**
@@ -537,12 +541,50 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
 			 * retried with eviction, or -ENOSPC if there should
 			 * be no second attempt.
 			 */
+			if (!alloc_state->in_evict)
+				alloc_state->may_try_low = may_evict;
 			if (ret == -EAGAIN)
 				ret = may_evict ? -EBUSY : -ENOSPC;
 			return ret;
 		}
 	}
 
+	/*
+	 * cgroup protection plays a special role in eviction.
+	 * Conceptually, protection of memory via the dmem cgroup controller
+	 * entitles the protected cgroup to use a certain amount of memory.
+	 * There are two types of protection - the 'low' limit is a
+	 * "best-effort" protection, whereas the 'min' limit provides a hard
+	 * guarantee that memory within the cgroup's allowance will not be
+	 * evicted under any circumstance.
+	 *
+	 * To faithfully model this concept in TTM, we also need to take cgroup
+	 * protection into account when allocating. When allocation in one
+	 * place fails, TTM will default to trying other places first before
+	 * evicting.
+	 * If the allocation is covered by dmem cgroup protection, however,
+	 * this prevents the allocation from using the memory it is "entitled"
+	 * to. To make sure unprotected allocations cannot push new protected
+	 * allocations out of places they are "entitled" to use, we should
+	 * evict buffers not covered by any cgroup protection, if this
+	 * allocation is covered by cgroup protection.
+	 *
+	 * Buffers covered by 'min' protection are a special case - the 'min'
+	 * limit is a stronger guarantee than 'low', and thus buffers protected
+	 * by 'low' but not 'min' should also be considered for eviction.
+	 * Buffers protected by 'min' will never be considered for eviction
+	 * anyway, so the regular eviction path should be triggered here.
+	 * Buffers protected by 'low' but not 'min' will take a special
+	 * eviction path that only evicts buffers covered by neither 'low' or
+	 * 'min' protections.
+	 */
+	if (!alloc_state->in_evict) {
+		may_evict |= dmem_cgroup_below_min(NULL, alloc_state->charge_pool);
+		alloc_state->may_try_low = may_evict;
+
+		may_evict |= dmem_cgroup_below_low(NULL, alloc_state->charge_pool);
+	}
+
 	ret = ttm_resource_alloc(bo, place, res, alloc_state->charge_pool);
 	if (ret) {
 		if (ret == -ENOSPC && may_evict)
@@ -656,8 +698,12 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 	evict_walk.walk.arg.trylock_only = true;
 	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
 
-	/* One more attempt if we hit low limit? */
-	if (!lret && evict_walk.hit_low) {
+	/* If we failed to find enough BOs to evict, but we skipped over
+	 * some BOs because they were covered by dmem low protection, retry
+	 * evicting these protected BOs too, except if we're told not to
+	 * consider protected BOs at all.
+	 */
+	if (!lret && evict_walk.hit_low && state->may_try_low) {
 		evict_walk.try_low = true;
 		lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
 	}
@@ -678,7 +724,8 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 	} while (!lret && evict_walk.evicted);
 
 	/* We hit the low limit? Try once more */
-	if (!lret && evict_walk.hit_low && !evict_walk.try_low) {
+	if (!lret && evict_walk.hit_low && !evict_walk.try_low &&
+			state->may_try_low) {
 		evict_walk.try_low = true;
 		goto retry;
 	}

-- 
2.55.0

[PATCH v8 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool

[Natalie Vock]

When checking whether to skip certain buffers because they're protected
by dmem.low, we're checking the effective protection of the evictee's
cgroup, but depending on how the evictor's cgroup relates to the
evictee's, the semantics of effective protection values change.

When testing against cgroups from different subtrees, page_counter's
recursive protection propagates memory protection afforded to a parent
down to the child cgroups, even if the children were not explicitly
protected. This prevents cgroups whose parents were afforded no
protection from stealing memory from cgroups whose parents were afforded
more protection, without users having to explicitly propagate this
protection.

However, if we always calculate protection from the root cgroup, this
breaks prioritization of sibling cgroups: If one cgroup was explicitly
protected and its siblings were not, the protected cgroup should get
higher priority, i.e. the protected cgroup should be able to steal from
unprotected siblings. This only works if we restrict the protection
calculation to the subtree shared by evictor and evictee.

Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 6bbea148838e4..79c96aba4bf49 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -628,11 +628,48 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
 {
 	struct ttm_bo_evict_walk *evict_walk =
 		container_of(walk, typeof(*evict_walk), walk);
+	struct dmem_cgroup_pool_state *limit_pool, *ancestor = NULL;
+	bool evict_valuable;
 	s64 lret;
 
-	if (!dmem_cgroup_state_evict_valuable(evict_walk->alloc_state->limit_pool,
-					      bo->resource->css, evict_walk->try_low,
-					      &evict_walk->hit_low))
+	/*
+	 * If may_try_low is not set, then we're trying to evict unprotected
+	 * buffers in favor of a protected allocation for charge_pool. Explicitly skip
+	 * buffers belonging to the same cgroup here - that cgroup is definitely protected,
+	 * even though dmem_cgroup_state_evict_valuable would allow the eviction because a
+	 * cgroup is always allowed to evict from itself even if it is protected.
+	 */
+	if (!evict_walk->alloc_state->may_try_low &&
+			bo->resource->css == evict_walk->alloc_state->charge_pool)
+		return 0;
+
+	limit_pool = evict_walk->alloc_state->limit_pool;
+	/*
+	 * If there is no explicit limit pool, find the root of the shared subtree between
+	 * evictor and evictee. This is important so that recursive protection rules can
+	 * apply properly: Recursive protection distributes cgroup protection afforded
+	 * to a parent cgroup but not used explicitly by a child cgroup between all child
+	 * cgroups (see docs of effective_protection in mm/page_counter.c). However, when
+	 * direct siblings compete for memory, siblings that were explicitly protected
+	 * should get prioritized over siblings that weren't. This only happens correctly
+	 * when the root of the shared subtree is passed to
+	 * dmem_cgroup_state_evict_valuable. Otherwise, the effective-protection
+	 * calculation cannot distinguish direct siblings from unrelated subtrees and the
+	 * calculated protection ends up wrong.
+	 */
+	if (!limit_pool) {
+		ancestor = dmem_cgroup_get_common_ancestor(bo->resource->css,
+							   evict_walk->alloc_state->charge_pool);
+		limit_pool = ancestor;
+	}
+
+	evict_valuable = dmem_cgroup_state_evict_valuable(limit_pool, bo->resource->css,
+							  evict_walk->try_low,
+							  &evict_walk->hit_low);
+	if (ancestor)
+		dmem_cgroup_pool_state_put(ancestor);
+
+	if (!evict_valuable)
 		return 0;
 
 	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))

-- 
2.55.0

Re: [PATCH v8 5/6] drm/ttm: Be more aggressive when allocating below protection limit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Allocations protected by the dmem cgroup `min` limit will incorrectly evict their own cgroup's buffers during the early eviction pass, preventing the cgroup from reaching its `min` guarantee.
--

--- Patch [5]: [PATCH v8 5/6] drm/ttm: Be more aggressive when allocating below protection limit ---
commit 30d3ed5047eb42c7b72fcb6f1d1973210be68163
Author: Natalie Vock <nat@pixelcluster.dev>

drm/ttm: Be more aggressive when allocating below protection limit
    
When the cgroup's memory usage is below the low/min limit and allocation
fails, try evicting some unprotected buffers to make space. Otherwise,
application buffers may be forced to go into GTT even though usage is
below the corresponding low/min limit, if other applications filled VRAM
with their allocations first.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 3becddd2c43b8..6bbea148838e4 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -537,12 +541,50 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
[ ... ]
> +	/*
> +	 * Buffers protected by 'min' will never be considered for eviction
> +	 * anyway, so the regular eviction path should be triggered here.
> +	 * Buffers protected by 'low' but not 'min' will take a special
> +	 * eviction path that only evicts buffers covered by neither 'low' or
> +	 * 'min' protections.
> +	 */
> +	if (!alloc_state->in_evict) {
> +		may_evict |= dmem_cgroup_below_min(NULL, alloc_state->charge_pool);
> +		alloc_state->may_try_low = may_evict;
> +
> +		may_evict |= dmem_cgroup_below_low(NULL, alloc_state->charge_pool);
> +	}

[Severity: High]
Does this code inadvertently allow a cgroup to evict its own buffers when it is
below the min limit?

When dmem_cgroup_below_min() returns true, alloc_state->may_try_low is set
to true here.

Later in ttm_bo_evict_cb(), this causes the self-eviction skip check to be
bypassed:

	if (!evict_walk->alloc_state->may_try_low &&
			bo->resource->css == evict_walk->alloc_state->charge_pool)
		return 0;

Because dmem_cgroup_state_evict_valuable() allows a pool to evict from itself,
this means the cgroup will evict its own buffers instead of unprotected buffers
from other cgroups. Will this prevent the cgroup from reaching its min limit
guarantee?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-dmemcg-aggressive-protect-v8-0-07af96681bf8@gmx.de?part=5

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

[sashiko-bot]

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 <nat@pixelcluster.dev>

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/20260804-dmemcg-aggressive-protect-v8-0-07af96681bf8@gmx.de?part=1

Re: [PATCH v8 3/6] drm/ttm: Extract code for attempting allocation in a place

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The synthetic `-EBUSY` signal collides with the native `-EBUSY` error code, causing unintended evictions and silent API contract changes.
--

--- Patch [3]: [PATCH v8 3/6] drm/ttm: Extract code for attempting allocation in a place ---
commit 8416fd662f16adcc69fd20180c1e3da4cb94ac03
Author: Natalie Vock <nat@pixelcluster.dev>

drm/ttm: Extract code for attempting allocation in a place

This commit moves the logic for attempting buffer allocation for a specific
place into a separate ttm_bo_alloc_at_place() function. It uses a synthetic
-EBUSY return code to signal that eviction should be attempted.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 3980f376e3ba4..1cdb2172d5924 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c

[ ... ]

> @@ -488,6 +488,51 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man
> +static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
> +				 const struct ttm_place *place,
> +				 bool force_space,
> +				 struct ttm_resource **res,
> +				 struct ttm_bo_alloc_state *alloc_state)
> +{
> +	bool may_evict;
> +	int ret;
> +
> +	may_evict = force_space && place->mem_type != TTM_PL_SYSTEM;
> +
> +	ret = ttm_resource_alloc(bo, place, res,
> +				 force_space ? &alloc_state->limit_pool : NULL);
> +
> +	if (ret) {
> +		if (ret == -ENOSPC && may_evict)
> +			return -EBUSY;
> +
> +		return ret;

[Severity: Medium]
If ttm_resource_alloc() organically returns -EBUSY, will this native error be
passed up unmodified and collide with the synthetic -EBUSY eviction signal?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -736,25 +782,25 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
> -		may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM);
> -		ret = ttm_resource_alloc(bo, place, res, force_space ? &limit_pool : NULL);
> -		if (ret) {
> -			if (ret != -ENOSPC) {
> -				dmem_cgroup_pool_state_put(limit_pool);
> -				return ret;
> -			}
> -			if (!may_evict) {
> -				dmem_cgroup_pool_state_put(limit_pool);
> -				continue;
> -			}
> +		ret = ttm_bo_alloc_at_place(bo, place, force_space, res,
> +					    &alloc_state);
>  
> +		if (ret == -ENOSPC) {
> +			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
> +			continue;
> +		} else if (ret == -EBUSY) {
>  			ret = ttm_bo_evict_alloc(bdev, man, place, bo, ctx,
> -						 ticket, res, limit_pool);
> -			dmem_cgroup_pool_state_put(limit_pool);
> +						 ticket, res, &alloc_state);

[Severity: Medium]
Before this patch, if ttm_resource_alloc() returned a native -EBUSY, it was
directly returned to the caller since it was not -ENOSPC.

By treating any -EBUSY returned from ttm_bo_alloc_at_place() as a signal to evict,
does this inadvertently cause an organic -EBUSY to trigger an unnecessary eviction
walk rather than propagating the error?

> +
> +			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
> +
>  			if (ret == -EBUSY)
>  				continue;
> -			if (ret)
> +			else if (ret)
>  				return ret;
> +		} else if (ret) {
> +			dmem_cgroup_pool_state_put(alloc_state.limit_pool);
> +			return ret;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-dmemcg-aggressive-protect-v8-0-07af96681bf8@gmx.de?part=3

Re: [PATCH v8 4/6] drm/ttm: Split cgroup charge and resource allocation

[sashiko-bot]

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

New issues:
- [High] Eviction walk continues to restrict evictions to a specific cgroup even after its memory limit is satisfied, causing spurious allocation failures under global memory pressure.

Pre-existing issues:
- [Critical] dmem_cgroup_get_common_ancestor returns a pool without a CSS reference, but the caller puts it using dmem_cgroup_pool_state_put which drops a CSS reference, leading to a CSS refcount underflow.
--

--- Patch [4]: [PATCH v8 4/6] drm/ttm: Split cgroup charge and resource allocation ---
commit aa5cab6f4c763e172491a34bffbf1a67e37f5262
Author: Natalie Vock <nat@pixelcluster.dev>

drm/ttm: Split cgroup charge and resource allocation

This patch decouples resource allocation from cgroup charging to address 
raciness when checking min/low cgroup limits during eviction. It charges 
the allocation to the cgroup once and keeps the charge for the duration of
the resource allocation attempt.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 1cdb2172d5924..3becddd2c43b8 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -518,18 +522,39 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
>  	bool may_evict;
>  	int ret;
>  
> -	may_evict = force_space && place->mem_type != TTM_PL_SYSTEM;
> -
> -	ret = ttm_resource_alloc(bo, place, res,
> -				 force_space ? &alloc_state->limit_pool : NULL);
> +	may_evict = !alloc_state->in_evict && force_space &&
> +		    place->mem_type != TTM_PL_SYSTEM;
> +	if (!alloc_state->charge_pool) {
> +		ret = ttm_resource_try_charge(bo, place, &alloc_state->charge_pool,
> +					      force_space ? &alloc_state->limit_pool
> +							  : NULL);

[Severity: High]
If ttm_resource_try_charge() succeeds during an eviction callback (where
force_space is false), alloc_state->limit_pool is not cleared.

If the subsequent physical resource allocation still fails due to global
memory pressure, the eviction walk will continue.

Because alloc_state->limit_pool remains set, will the LRU walk continue to
only evict buffers from that specific cgroup, eventually causing a spurious
-ENOSPC failure if that cgroup has no more evictable buffers?

> +		if (ret) {
> +			/*
> +			 * -EAGAIN means the charge failed, which we treat
> +			 * like an allocation failure. Therefore, return an
> +			 * error code indicating the allocation failed -
> +			 * either -EBUSY if the allocation should be
> +			 * retried with eviction, or -ENOSPC if there should
> +			 * be no second attempt.
> +			 */
> +			if (ret == -EAGAIN)
> +				ret = may_evict ? -EBUSY : -ENOSPC;
> +			return ret;
> +		}
> +	}
[ ... ]
> @@ -584,8 +609,10 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
>  
>  	evict_walk->evicted++;
>  	if (evict_walk->res)
> -		lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
> -					  evict_walk->res, NULL);
> +		lret = ttm_bo_alloc_at_place(evict_walk->evictor,
> +					     evict_walk->place, false,
> +					     evict_walk->res,
> +					     evict_walk->alloc_state);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential refcount underflow
involving the ancestor pool earlier in ttm_bo_evict_cb().

dmem_cgroup_get_common_ancestor() explicitly drops its CSS reference before
returning the pool:

kernel/cgroup/dmem.c:dmem_cgroup_get_common_ancestor() {
    ...
    if (IS_ERR(pool))
        pool = NULL;
    css_put(ancestor_css);
    return pool;
}

However, in ttm_bo_evict_cb(), the ancestor pool is freed using
dmem_cgroup_pool_state_put():

    if (ancestor)
        dmem_cgroup_pool_state_put(ancestor);

Since dmem_cgroup_pool_state_put() calls css_put(&pool->cs->css), doesn't
this drop a CSS reference that the caller does not own, potentially leading
to a use-after-free of the cgroup subsystem state?

>  	if (lret == 0)
>  		return 1;
>  out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-dmemcg-aggressive-protect-v8-0-07af96681bf8@gmx.de?part=4

Re: [PATCH v8 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Refcount underflow and use-after-free of cgroup subsystem state (css).
- [High] GFP_KERNEL allocation while holding a dma_resv lock causes lockdep warnings and potential deadlocks.
--

--- Patch [6]: [PATCH v8 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool ---
commit 80190cfff36182bea4c8b3f724df7df584bfc3b3
Author: Natalie Vock <nat@pixelcluster.dev>

drm/ttm: Use common ancestor of evictor and evictee as limit pool
    
When checking if buffers should be skipped for eviction due to dmem.low
protection, this patch calculates the protection from the shared ancestor
of the evictor and evictee instead of the root cgroup. This prevents
breaking prioritization of sibling cgroups.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 6bbea148838e4..79c96aba4bf49 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -628,11 +628,48 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
>  {
>  	struct ttm_bo_evict_walk *evict_walk =
>  		container_of(walk, typeof(*evict_walk), walk);
> +	struct dmem_cgroup_pool_state *limit_pool, *ancestor = NULL;
> +	bool evict_valuable;
>  	s64 lret;
>  
[ ... ]
> +	if (!limit_pool) {
> +		ancestor = dmem_cgroup_get_common_ancestor(bo->resource->css,
> +							   evict_walk->alloc_state->charge_pool);

[Severity: High]
Does this allocation path trigger a deadlock?

ttm_bo_evict_cb() is executed via ttm_lru_walk_for_evict() which holds the
dma_resv lock of the buffer object (acquired via
ttm_bo_lru_for_each_reserved_guarded).

When calling dmem_cgroup_get_common_ancestor(), it can end up in
get_cg_pool_unlocked() which does a GFP_KERNEL allocation if the ancestor
pool does not exist:

kernel/cgroup/dmem.c:get_cg_pool_unlocked() {
	...
	allocpool = kzalloc_obj(*allocpool);
	...
}

Using GFP_KERNEL while holding a dma_resv lock can trigger direct memory
reclaim, which in turn can invoke GPU shrinkers.

Those shrinkers will attempt to acquire dma_resv locks, leading to a
potential deadlock.

> +		limit_pool = ancestor;
> +	}
> +
> +	evict_valuable = dmem_cgroup_state_evict_valuable(limit_pool, bo->resource->css,
> +							  evict_walk->try_low,
> +							  &evict_walk->hit_low);
> +	if (ancestor)
> +		dmem_cgroup_pool_state_put(ancestor);

[Severity: Critical]
Does this cause a refcount underflow and use-after-free of the cgroup
subsystem state (css)?

dmem_cgroup_get_common_ancestor() acquires and returns a pool reference but
explicitly drops its temporary css reference before returning:

kernel/cgroup/dmem.c:dmem_cgroup_get_common_ancestor() {
	...
	if (IS_ERR(pool))
		pool = NULL;
	css_put(ancestor_css);
	return pool;
}

However, dmem_cgroup_pool_state_put() is designed to balance
dmem_cgroup_try_charge() and drops both a pool reference and a css reference.

Since dmem_cgroup_get_common_ancestor() did not acquire an extra css reference
for the caller, the css_put() in dmem_cgroup_pool_state_put() causes an
underflow.

This can lead to premature freeing of the css, resulting in a use-after-free
and memory corruption.

> +
> +	if (!evict_valuable)
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-dmemcg-aggressive-protect-v8-0-07af96681bf8@gmx.de?part=6

Re: [PATCH v8 0/6] cgroup/dmem, drm/ttm: Improve protection in contended cases

[Timur Kristóf]

On Tuesday, August 4, 2026 10:25:15 AM Central European Summer Time Natalie 
Vock wrote:
> Hi all,
> 
> I've been looking into some cases where dmem protection fails to prevent
> allocations from ending up in GTT when VRAM gets scarce and apps start
> competing hard.
> 
> In short, this is because other (unprotected) applications end up
> filling VRAM before protected applications do. This causes TTM to back
> off and try allocating in GTT before anything else, and that is where
> the allocation is placed in the end. The existing eviction protection
> cannot prevent this, because no attempt at evicting is ever made
> (although you could consider the backing-off as an immediate eviction to
> GTT).
> 
> This series tries to alleviate this by adding a special case when the
> allocation is protected by cgroups: Instead of backing off immediately,
> TTM will try evicting unprotected buffers from the domain to make space
> for the protected one. This ensures that applications can actually use
> all the memory protection awarded to them by the system, without being
> prone to ping-ponging (only protected allocations can evict unprotected
> ones, never the other way around).
> 
> The first two patches just add a few small utilities needed to implement
> this to the dmem controller. The other patches are the TTM implementation:
> 
> "drm/ttm: Be more aggressive..." decouples cgroup charging from resource
> allocation to allow us to hold on to the charge even if allocation fails
> on first try, and adds a path to call ttm_bo_evict_alloc when the
> charged allocation falls within min/low protection limits.
> 
> "drm/ttm: Use common ancestor..." is a more general improvement in
> correctly implementing cgroup protection semantics. With recursive
> protection rules, unused memory protection afforded to a parent node is
> transferred to children recursively, which helps protect entire
> subtrees from stealing each others' memory without needing to protect
> each cgroup individually. This doesn't apply when considering direct
> siblings inside the same subtree, so in order to not break
> prioritization between these siblings, we need to consider the
> relationship of evictor and evictee when calculating protection.
> In practice, this fixes cases where a protected cgroup cannot steal
> memory from unprotected siblings (which, in turn, leads to eviction
> failures and new allocations being placed in GTT).
> 
> Thanks,
> Natalie
> 
> Signed-off-by: Natalie Vock <natalie.vock@gmx.de>

Very nice work!

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>

for the whole series.

> ---
> Changes in v8:
> - Fix a few Sashiko complaints
> - Link to v7:
> https://patch.msgid.link/20260721-dmemcg-aggressive-protect-v7-0-97c4b10143
> 15@gmx.de
> 
> Changes in v7:
> - Fix incorrect refcounting/locking around dmem_cgroup_common_ancestor
>   (Cascardo)
> - Rebase on latest drm-misc-next
> - Link to v6:
> https://lore.kernel.org/r/20260313-dmemcg-aggressive-protect-v6-0-7c71cc149
> 2db@gmx.de
> 
> Changes in v6:
> - Rename dmem_cgroup_common_ancestor to dmem_cgroup_get_common_ancestor
>   (Maarten)
> - Use ttm_bo_alloc_at_place in eviction cb as well (Tvrtko)
> - Clean up the more aggressive eviction logic (Tvrtko)
> - Link to v5:
> https://lore.kernel.org/r/20260302-dmemcg-aggressive-protect-v5-0-ffd3a2602
> 309@gmx.de
> 
> Changes in v5:
> - Added cgroup_common_ancestor helper to use with
>   dmem_cgroup_common_ancestor (Tejun)
> - Note: "drm/ttm: Use common ancestor..." needed minor changes since
>   dmem_cgroup_common_ancestor now grabs a reference to the ancestor
>   pool which needs to be dropped after use
> - Removed extraneous whitespaces in "drm/ttm: Split cgroup charge..."
>   and unnecessary changes done in "drm/ttm: Extract code..." (Tvrtko)
> - Applied a comment from v3 about below_low not needing to be
>   initialized in "drm/ttm: Be more aggressive..." (Tvrtko)
> - Fixed uncharging the cgroup on allocation failure (Tvrtko)
> - Fixed a typo in the message of "drm/ttm: Split cgroup charge..."
>   (Tvrtko)
> - Added case in ttm_bo_evict_cb for when charging fails, since we need
>   to retry the charge (found myself)
> - Link to v4:
> https://lore.kernel.org/r/20260225-dmemcg-aggressive-protect-v4-0-de847ab35
> 184@gmx.de
> 
> Changes in v4:
> - Split cgroup charge decoupling and eviction logic changes into
>   separate commits (Tvrtko)
> - Fix two cases of errno handling in ttm_bo_alloc_place and its caller
>   (Tvrtko)
> - Improve commit message/description of "drm/ttm: Make a helper..." (now
>   "drm/ttm: Extract code...") (Tvrtko)
> - Documentation improvements for new TTM eviction logic (Tvrtko)
> - Formatting fixes (Tvrtko)
> - Link to v3:
> https://lore.kernel.org/r/20251110-dmemcg-aggressive-protect-v3-0-219ffcfc5
> 4e9@gmx.de
> 
> Changes in v3:
> - Improved documentation around cgroup queries and TTM eviction helpers
>   (Maarten)
> - Fixed up ttm_alloc_at_place charge failure logic to return either
>   -EBUSY or -ENOSPC, not -EAGAIN (found this myself)
> - Link to v2:
> https://lore.kernel.org/r/20251015-dmemcg-aggressive-protect-v2-0-36644fb4e
> 37f@gmx.de
> 
> Changes in v2:
> - Factored out the ttm logic for charging/allocating/evicting into a
>   separate helper to keep things simpler
> - Link to v1:
> https://lore.kernel.org/r/20250915-dmemcg-aggressive-protect-v1-0-2f3353bfc
> dac@gmx.de
> 
> ---
> Natalie Vock (6):
>       cgroup/dmem: Add queries for protection values
>       cgroup,cgroup/dmem: Add (dmem_)cgroup_common_ancestor helper
>       drm/ttm: Extract code for attempting allocation in a place
>       drm/ttm: Split cgroup charge and resource allocation
>       drm/ttm: Be more aggressive when allocating below protection limit
>       drm/ttm: Use common ancestor of evictor and evictee as limit pool
> 
>  drivers/gpu/drm/ttm/ttm_bo.c       | 223
> ++++++++++++++++++++++++++++++++----- drivers/gpu/drm/ttm/ttm_resource.c | 
> 51 ++++++---
>  include/drm/ttm/ttm_resource.h     |   6 +-
>  include/linux/cgroup.h             |  21 ++++
>  include/linux/cgroup_dmem.h        |  25 +++++
>  kernel/cgroup/dmem.c               | 103 +++++++++++++++++
>  6 files changed, 383 insertions(+), 46 deletions(-)
> ---
> base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
> change-id: 20250915-dmemcg-aggressive-protect-5cf37f717cdb
> 
> Best regards,
> --
> Natalie Vock <natalie.vock@gmx.de>

Re: [PATCH v8 0/6] cgroup/dmem,drm/ttm: Improve protection in contended cases

[Timur Kristóf]

On Tuesday, August 4, 2026 10:25:15 AM Central European Summer Time Natalie 
Vock wrote:
> Hi all,
> 
> I've been looking into some cases where dmem protection fails to prevent
> allocations from ending up in GTT when VRAM gets scarce and apps start
> competing hard.
> 
> In short, this is because other (unprotected) applications end up
> filling VRAM before protected applications do. This causes TTM to back
> off and try allocating in GTT before anything else, and that is where
> the allocation is placed in the end. The existing eviction protection
> cannot prevent this, because no attempt at evicting is ever made
> (although you could consider the backing-off as an immediate eviction to
> GTT).
> 
> This series tries to alleviate this by adding a special case when the
> allocation is protected by cgroups: Instead of backing off immediately,
> TTM will try evicting unprotected buffers from the domain to make space
> for the protected one. This ensures that applications can actually use
> all the memory protection awarded to them by the system, without being
> prone to ping-ponging (only protected allocations can evict unprotected
> ones, never the other way around).
> 
> The first two patches just add a few small utilities needed to implement
> this to the dmem controller. The other patches are the TTM implementation:
> 
> "drm/ttm: Be more aggressive..." decouples cgroup charging from resource
> allocation to allow us to hold on to the charge even if allocation fails
> on first try, and adds a path to call ttm_bo_evict_alloc when the
> charged allocation falls within min/low protection limits.
> 
> "drm/ttm: Use common ancestor..." is a more general improvement in
> correctly implementing cgroup protection semantics. With recursive
> protection rules, unused memory protection afforded to a parent node is
> transferred to children recursively, which helps protect entire
> subtrees from stealing each others' memory without needing to protect
> each cgroup individually. This doesn't apply when considering direct
> siblings inside the same subtree, so in order to not break
> prioritization between these siblings, we need to consider the
> relationship of evictor and evictee when calculating protection.
> In practice, this fixes cases where a protected cgroup cannot steal
> memory from unprotected siblings (which, in turn, leads to eviction
> failures and new allocations being placed in GTT).
> 
> Thanks,
> Natalie
> 
> Signed-off-by: Natalie Vock <natalie.vock@gmx.de>

Very nice work!

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>

for the whole series.

> ---
> Changes in v8:
> - Fix a few Sashiko complaints
> - Link to v7:
> https://patch.msgid.link/20260721-dmemcg-aggressive-protect-v7-0-97c4b10143
> 15@gmx.de
> 
> Changes in v7:
> - Fix incorrect refcounting/locking around dmem_cgroup_common_ancestor
>   (Cascardo)
> - Rebase on latest drm-misc-next
> - Link to v6:
> https://lore.kernel.org/r/20260313-dmemcg-aggressive-protect-v6-0-7c71cc149
> 2db@gmx.de
> 
> Changes in v6:
> - Rename dmem_cgroup_common_ancestor to dmem_cgroup_get_common_ancestor
>   (Maarten)
> - Use ttm_bo_alloc_at_place in eviction cb as well (Tvrtko)
> - Clean up the more aggressive eviction logic (Tvrtko)
> - Link to v5:
> https://lore.kernel.org/r/20260302-dmemcg-aggressive-protect-v5-0-ffd3a2602
> 309@gmx.de
> 
> Changes in v5:
> - Added cgroup_common_ancestor helper to use with
>   dmem_cgroup_common_ancestor (Tejun)
> - Note: "drm/ttm: Use common ancestor..." needed minor changes since
>   dmem_cgroup_common_ancestor now grabs a reference to the ancestor
>   pool which needs to be dropped after use
> - Removed extraneous whitespaces in "drm/ttm: Split cgroup charge..."
>   and unnecessary changes done in "drm/ttm: Extract code..." (Tvrtko)
> - Applied a comment from v3 about below_low not needing to be
>   initialized in "drm/ttm: Be more aggressive..." (Tvrtko)
> - Fixed uncharging the cgroup on allocation failure (Tvrtko)
> - Fixed a typo in the message of "drm/ttm: Split cgroup charge..."
>   (Tvrtko)
> - Added case in ttm_bo_evict_cb for when charging fails, since we need
>   to retry the charge (found myself)
> - Link to v4:
> https://lore.kernel.org/r/20260225-dmemcg-aggressive-protect-v4-0-de847ab35
> 184@gmx.de
> 
> Changes in v4:
> - Split cgroup charge decoupling and eviction logic changes into
>   separate commits (Tvrtko)
> - Fix two cases of errno handling in ttm_bo_alloc_place and its caller
>   (Tvrtko)
> - Improve commit message/description of "drm/ttm: Make a helper..." (now
>   "drm/ttm: Extract code...") (Tvrtko)
> - Documentation improvements for new TTM eviction logic (Tvrtko)
> - Formatting fixes (Tvrtko)
> - Link to v3:
> https://lore.kernel.org/r/20251110-dmemcg-aggressive-protect-v3-0-219ffcfc5
> 4e9@gmx.de
> 
> Changes in v3:
> - Improved documentation around cgroup queries and TTM eviction helpers
>   (Maarten)
> - Fixed up ttm_alloc_at_place charge failure logic to return either
>   -EBUSY or -ENOSPC, not -EAGAIN (found this myself)
> - Link to v2:
> https://lore.kernel.org/r/20251015-dmemcg-aggressive-protect-v2-0-36644fb4e
> 37f@gmx.de
> 
> Changes in v2:
> - Factored out the ttm logic for charging/allocating/evicting into a
>   separate helper to keep things simpler
> - Link to v1:
> https://lore.kernel.org/r/20250915-dmemcg-aggressive-protect-v1-0-2f3353bfc
> dac@gmx.de
> 
> ---
> Natalie Vock (6):
>       cgroup/dmem: Add queries for protection values
>       cgroup,cgroup/dmem: Add (dmem_)cgroup_common_ancestor helper
>       drm/ttm: Extract code for attempting allocation in a place
>       drm/ttm: Split cgroup charge and resource allocation
>       drm/ttm: Be more aggressive when allocating below protection limit
>       drm/ttm: Use common ancestor of evictor and evictee as limit pool
> 
>  drivers/gpu/drm/ttm/ttm_bo.c       | 223
> ++++++++++++++++++++++++++++++++----- drivers/gpu/drm/ttm/ttm_resource.c | 
> 51 ++++++---
>  include/drm/ttm/ttm_resource.h     |   6 +-
>  include/linux/cgroup.h             |  21 ++++
>  include/linux/cgroup_dmem.h        |  25 +++++
>  kernel/cgroup/dmem.c               | 103 +++++++++++++++++
>  6 files changed, 383 insertions(+), 46 deletions(-)
> ---
> base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
> change-id: 20250915-dmemcg-aggressive-protect-5cf37f717cdb
> 
> Best regards,
> --
> Natalie Vock <natalie.vock@gmx.de>

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

[Maarten Lankhorst]

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 <natalie.vock@gmx.de>
> ---
>  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.

Reviewed-by: Maarten Lankhorst <dev@lankhorst.se>

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

[Natalie Vock]

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 <natalie.vock@gmx.de>
>> ---
>>   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 <dev@lankhorst.se>
> 

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

[Maarten Lankhorst]

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 <natalie.vock@gmx.de>
>>> ---
>>>   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

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

[Thadeu Lima de Souza Cascardo]

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 <natalie.vock@gmx.de>
> >>> ---
> >>>   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.

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.

Regards.
Cascardo.

[1] https://lore.kernel.org/cgroups/ahW8MY4XBox_nsmB@quatroqueijos.cascardo.eti.br/

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

[Natalie Vock]

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 <natalie.vock@gmx.de>
>>>>> ---
>>>>>    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.)

> 
> 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?

Regards,
Natalie

> 
> Regards.
> Cascardo.
> 
> [1] https://lore.kernel.org/cgroups/ahW8MY4XBox_nsmB@quatroqueijos.cascardo.eti.br/

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

[Thadeu Lima de Souza Cascardo]

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 <natalie.vock@gmx.de>
> > > > > > ---
> > > > > >    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.


> Regards,
> Natalie
> 
> > 
> > Regards.
> > Cascardo.
> > 
> > [1] https://lore.kernel.org/cgroups/ahW8MY4XBox_nsmB@quatroqueijos.cascardo.eti.br/
> 

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

[Thadeu Lima de Souza Cascardo]

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 <natalie.vock@gmx.de>
> > > > > > > ---
> > > > > > >    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 <cascardo@igalia.com>


> 
> > Regards,
> > Natalie
> > 
> > > 
> > > Regards.
> > > Cascardo.
> > > 
> > > [1] https://lore.kernel.org/cgroups/ahW8MY4XBox_nsmB@quatroqueijos.cascardo.eti.br/
> > 

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

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

[Maarten Lankhorst]

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 <natalie.vock@gmx.de>
>>>>> ---
>>>>>   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.
> 
> 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.

region->unregistered would not be possible here. This means that the
driver code is calling dmem_cgroup_get_common_ancestor while unregistering
at the same time.

There's a bigger problem in that case, the driver should first deallocate all
its allocations before unregistering. The unregistered is intended for
guarding against userspace access while unregistering, not against kernel
tearing down a cgroup while it's still live.

Kind regards,
~Maarten Lankhorst

Re: [PATCH v8 0/6] cgroup/dmem,drm/ttm: Improve protection in contended cases

[Natalie Vock]

Applied the series with a small fixup for patch 2/6. Thanks everyone!

Best,
Natalie