Skip to contentSkip to conversation

Add reclaim to the dmem cgroup controller

PATCH In review Thomas Hellström 2 participants 10 messages Jul 25 – Jul 28 2 messages not in the archive

Subsystems (8)
lore
Share & tools
lore

Thomas Hellström

author maintainer
#0 lore raw

v9 cover Add reclaim to the dmem cgroup controller

When writing a "max" limit lower than the current usage, the existing code silently failed. This series aims to improve on that by returning -EBUSY on failure and also attempt to synchronously reclaim device memory to push the usage under the new max limit to avoid the error.

Patch 1 fixes a pre-existing amdgpu_vram_mgr_init() error path Patch 2 introduces struct dmem_cgroup_init for extensible region
registration.
Patch 3 implements and documents a reclaim callback interface
for the dmem controller.
Patch 4 implements a TTM reclaim callback.
Patches 5-6 hook up the reclaim callback to the dmem cgroup-aware
drivers xe and amdgpu.

v2:
- Remove the error propagation that was in a previous series (Maarten)
- A number of updates in patch 1. See its commit message for details (Maarten)

v3:
- Add patch 1 fixing a pre-existing amdgpu_vram_mgr_init() error path bug where drmm_cgroup_register_region() was called before INIT_LIST_HEAD() and gpu_buddy_init(), causing a kernel panic on failure. (Sashiko-bot)
- Use an rwsem to protect reclaim callback registration and region unregister against concurrent reclaim invocations. (Sashiko-bot)
- Fix ttm_resource_manager_set_dmem_region() storing an error pointer in man->cg unconditionally. (Sashiko-bot)
- Fix kernel-doc function name format for ttm_bo_evict_cgroup() and ttm_resource_manager_set_dmem_region().

v4:
- Rebased on drm-tip; dropped the XE_PL_STOLEN guard in the xe patch as stolen memory uses a separate TTM manager.

v5:
- Add patch 2 introducing struct dmem_cgroup_init to make the dmem_cgroup_register_region() API extensible without adding positional arguments in the future.
- Use nonblock=true in reset_all_resource_limits() to avoid sleeping inside rcu_read_lock() in dmemcs_offline(). (Sashiko-bot)
- Compare usage against the truncated limit stored in cnt.max, not the original u64. (Sashiko-bot)
- Use DMEM_MAX_RECLAIM_RETRIES (16) retry budget instead of 5, matching the memcg controller; only -ENOSPC (no progress) counts against the budget, other errors abort immediately.
- Handle NULL region in ttm_resource_manager_set_dmem_region() to clear the reclaim callback, preventing use-after-free when the manager is torn down while the dmem region outlives it. (Sashiko-bot)
- Return 0 on any eviction progress; reserve -ENOSPC for zero progress.
- Clear the reclaim callback in xe and amdgpu fini paths to prevent use-after-free after driver unbind with open DRM file descriptors. (Sashiko-bot)
- Register xe fini devres action before drmm_cgroup_register_region() so LIFO teardown runs unregister first, draining callbacks before the manager is destroyed. (Sashiko-bot)
- Switch amdgpu to explicit dmem_cgroup_unregister_region() at the top of amdgpu_vram_mgr_fini() before any manager teardown, since amdgpu's fini is called explicitly during driver unbind before drmm cleanup. (Sashiko-bot)
- Wrap the xe reclaim callback with drm_dev_enter()/drm_dev_exit() to prevent TTM reclaim from running after driver unbind.

v6:
- Move the ops check inside down_read() in set_resource_max(), guarded by region->unregistered, to close a UAF race against dmem_cgroup_unregister_region(). (Sashiko-bot)
- Fix dmem_cgroup_ops->reclaim docstring: -ENOSPC is retried up to DMEM_MAX_RECLAIM_RETRIES times, not an immediate stop. (Sashiko-bot)
- Fix mgr->cg_region never being assigned in amdgpu_vram_mgr_init(), causing dmem_cgroup_unregister_region() in fini to silently no-op. (Sashiko-bot)
- Reorder amdgpu_vram_mgr_fini() to call set_used(false) and evict_all() before dmem_cgroup_unregister_region(), so ttm_resource_free() can uncharge via man->cg during eviction; clear man->cg after unregister. (Sashiko-bot)

v7:
- Replace the per-region rw_semaphore with a static SRCU domain (dmemcg_srcu). SRCU is a better fit: it avoids per-region lock overhead on every reclaim call, and synchronize_srcu() at unregister time is a rare, shutdown-time operation. (Maarten)
- Trim in-function comments to focus on what rather than how.
- Switch back to drmm_cgroup_register_region() with a drm_dev_enter/ exit guard in the reclaim callback (matching xe), rather than manual register/unregister. drm_dev_unplug() fires before vram_mgr_fini(), so drm_dev_enter() returning false prevents any reclaim from touching the manager during teardown. This also fixes the "vram" name collision on multi-GPU systems, since drmm_cgroup_register_region() automatically prefixes with "drm/<pci-addr>/". (Sashiko-bot)

v8:
- Fix ttm_resource_manager_set_dmem_region() using IS_ERR_OR_NULL(), which skipped the assignment for a NULL region and thus never cleared man->cg. Use IS_ERR() so that a NULL region detaches the region as the kernel-doc and the v5 changelog intended. (Sashiko-bot)
- Move the amdgpu dmem cgroup region teardown back into amdgpu_vram_mgr_fini() with an explicit dmem_cgroup_unregister_region() after evict_all(), keeping the drm_dev_enter() guard. The v7 drmm-only approach left a use-after-free on the probe-error teardown path, where drm_dev_unplug() is never called so the drm_dev_enter() guard stays open while drm_driver.release destroys the manager before the drmm action unregisters the region. (Sashiko-bot)

v9:
- Init / fini race fixes. (Sashiko-bot, Thomas)
- Drop a misleading comment. (Maarten)
- Acquire runtime PM before reclaim. (Sashiko-bot)

User-space tests are at
https://patchwork.freedesktop.org/series/163935/

Test-with: [email protected]

Thomas Hellström (6):
drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init() cgroup/dmem: Introduce struct dmem_cgroup_init for region
initialization
cgroup/dmem: Add reclaim callback for lowering max below current usage drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem
controller
drm/xe: Wire up dmem cgroup reclaim for VRAM manager drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 65 +++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h |  3 +
 drivers/gpu/drm/drm_drv.c                    |  8 +-
 drivers/gpu/drm/ttm/ttm_bo.c                 | 94 +++++++++++++++++++-
 drivers/gpu/drm/ttm/ttm_bo_util.c            |  3 +-
 drivers/gpu/drm/ttm/ttm_resource.c           | 54 ++++++++++-
 drivers/gpu/drm/xe/xe_ttm_vram_mgr.c         | 71 +++++++++++++--
 include/drm/drm_drv.h                        |  4 +-
 include/drm/ttm/ttm_bo.h                     | 10 +++
 include/drm/ttm/ttm_resource.h               |  7 ++
 include/linux/cgroup_dmem.h                  | 38 +++++++-
 kernel/cgroup/dmem.c                         | 90 +++++++++++++++----
 13 files changed, 408 insertions(+), 41 deletions(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH v9 1/6 drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init()

drmm_cgroup_register_region() is called before INIT_LIST_HEAD() and gpu_buddy_init() in amdgpu_vram_mgr_init(). If it fails, the function returns early and bypasses those initializations.

Since adev->mman.initialized is set to true before amdgpu_vram_mgr_init() is called, a failure triggers amdgpu_ttm_fini(), which calls amdgpu_vram_mgr_fini(), which then:

- Calls list_for_each_entry_safe() on reservations_pending and reserved_pages, whose list_head::next pointers are zero-initialized (NULL). The loop does not recognize them as empty and dereferences NULL.

- Calls gpu_buddy_fini(), which iterates free_trees[] unconditionally via for_each_free_tree(). Since mm->free_trees is NULL (never allocated), this dereferences NULL.

Both result in a kernel panic on the module load error path.

Fix by moving drmm_cgroup_register_region() to after the list and buddy allocator are fully initialized, so the teardown path is safe to run.

Fixes 2b624a2c1865 ("drm/ttm: Handle cgroup based eviction in TTM")
Cc Friedrich Vock <[email protected]>
Cc Maarten Lankhorst <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Maxime Ripard <[email protected]>
Cc Christian König <[email protected]>
Cc Alex Deucher <[email protected]>
Cc <[email protected]> # v6.14+
Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +4 -3
index 2a241a5b12c4..ac3f71d77140 100644
@@ -918,9 +918,6 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev)
 	struct ttm_resource_manager *man = &mgr->manager;
 	int err;
 
-	man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram", adev->gmc.real_vram_size);
-	if (IS_ERR(man->cg))
-		return PTR_ERR(man->cg);
 	ttm_resource_manager_init(man, &adev->mman.bdev,
 				  adev->gmc.real_vram_size);
 
@@ -935,6 +932,10 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev)
 	if (err)
 		return err;
 
+	man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram", adev->gmc.real_vram_size);
+	if (IS_ERR(man->cg))
+		return PTR_ERR(man->cg);
+
 	ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_VRAM, &mgr->manager);
 	ttm_resource_manager_set_used(man, true);
 	return 0;
Signature · 1 line
2.55.0
author maintainer
#2 lore raw

PATCH v9 2/6 cgroup/dmem: Introduce struct dmem_cgroup_init for region initialization

In reply to Thomas Hellström

Replace the bare u64 size argument to dmem_cgroup_register_region() and drmm_cgroup_register_region() with a const struct dmem_cgroup_init * pointer. The struct currently carries only the size field, but using a struct makes the API extensible: future callers can supply additional initialization parameters without adding more positional arguments.

Update all in-tree callers (amdgpu, xe) to use a compound-literal initializer.

v5:
- Commit introduced.

Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c |  6 +++++-
 drivers/gpu/drm/drm_drv.c                    |  8 +++++---
 drivers/gpu/drm/xe/xe_ttm_vram_mgr.c         |  7 ++++++-
 include/drm/drm_drv.h                        |  4 +++-
 include/linux/cgroup_dmem.h                  | 16 +++++++++++++---
 kernel/cgroup/dmem.c                         | 10 ++++++----
 6 files changed, 38 insertions(+), 13 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +5 -1
index ac3f71d77140..08f05c3aed1d 100644
@@ -23,6 +23,7 @@
  */
 
 #include <linux/dma-mapping.h>
+#include <linux/cgroup_dmem.h>
 #include <drm/ttm/ttm_range_manager.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_buddy.h>
@@ -932,7 +933,10 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev)
 	if (err)
 		return err;
 
-	man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram", adev->gmc.real_vram_size);
+	man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram",
+					      &(struct dmem_cgroup_init){
+						.size = adev->gmc.real_vram_size,
+					      });
 	if (IS_ERR(man->cg))
 		return PTR_ERR(man->cg);
 
drivers/gpu/drm/drm_drv.c +5 -3
index e51ed959da89..c808958a2188 100644
@@ -968,17 +968,19 @@ static void drmm_cg_unregister_region(struct drm_device *dev, void *arg)
  * drmm_cgroup_register_region - Register a region of a DRM device to cgroups
  * @dev: device for region
  * @region_name: Region name for registering
- * @size: Size of region in bytes
+ * @init: Initialization parameters for the region.
  *
  * This decreases the ref-count of @dev by one. The device is destroyed if the
  * ref-count drops to zero.
  */
-struct dmem_cgroup_region *drmm_cgroup_register_region(struct drm_device *dev, const char *region_name, u64 size)
+struct dmem_cgroup_region *
+drmm_cgroup_register_region(struct drm_device *dev, const char *region_name,
+			    const struct dmem_cgroup_init *init)
 {
 	struct dmem_cgroup_region *region;
 	int ret;
 
-	region = dmem_cgroup_register_region(size, "drm/%s/%s", dev->unique, region_name);
+	region = dmem_cgroup_register_region(init, "drm/%s/%s", dev->unique, region_name);
 	if (IS_ERR_OR_NULL(region))
 		return region;
 
drivers/gpu/drm/xe/xe_ttm_vram_mgr.c +6 -1
index b518f7dec680..308fda4248eb 100644
@@ -4,6 +4,8 @@
  * Copyright (C) 2021-2022 Red Hat
  */
 
+#include <linux/cgroup_dmem.h>
+
 #include <drm/drm_managed.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_buddy.h>
@@ -303,7 +305,10 @@ int __xe_ttm_vram_mgr_init(struct xe_device *xe, struct xe_ttm_vram_mgr *mgr,
 	int err;
 
 	name = mem_type == XE_PL_VRAM0 ? "vram0" : "vram1";
-	man->cg = drmm_cgroup_register_region(&xe->drm, name, size);
+	man->cg = drmm_cgroup_register_region(&xe->drm, name,
+					      &(struct dmem_cgroup_init){
+						.size = size,
+					      });
 	if (IS_ERR(man->cg))
 		return PTR_ERR(man->cg);
 
include/drm/drm_drv.h +3 -1
index e09559495c5b..b23830494ed4 100644
@@ -34,6 +34,7 @@
 
 #include <drm/drm_device.h>
 
+struct dmem_cgroup_init;
 struct dmem_cgroup_region;
 struct drm_fb_helper;
 struct drm_fb_helper_surface_size;
@@ -433,7 +434,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
 
 struct dmem_cgroup_region *
 drmm_cgroup_register_region(struct drm_device *dev,
-			    const char *region_name, u64 size);
+			    const char *region_name,
+			    const struct dmem_cgroup_init *init);
 
 /**
  * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
include/linux/cgroup_dmem.h +13 -3
index dd4869f1d736..d9eab8a2c1ee 100644
@@ -14,8 +14,18 @@ struct dmem_cgroup_pool_state;
 /* Opaque definition of a cgroup region, used internally */
 struct dmem_cgroup_region;
 
+/**
+ * struct dmem_cgroup_init - Initialization parameters for a dmem cgroup region.
+ * @size: Size of the region in bytes.
+ */
+struct dmem_cgroup_init {
+	u64 size;
+};
+
 #if IS_ENABLED(CONFIG_CGROUP_DMEM)
-struct dmem_cgroup_region *dmem_cgroup_register_region(u64 size, const char *name_fmt, ...) __printf(2,3);
+struct dmem_cgroup_region *
+dmem_cgroup_register_region(const struct dmem_cgroup_init *init,
+			    const char *name_fmt, ...) __printf(2, 3);
 void dmem_cgroup_unregister_region(struct dmem_cgroup_region *region);
 int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
 			   struct dmem_cgroup_pool_state **ret_pool,
@@ -27,8 +37,8 @@ bool dmem_cgroup_state_evict_valuable(struct dmem_cgroup_pool_state *limit_pool,
 
 void dmem_cgroup_pool_state_put(struct dmem_cgroup_pool_state *pool);
 #else
-static inline __printf(2,3) struct dmem_cgroup_region *
-dmem_cgroup_register_region(u64 size, const char *name_fmt, ...)
+static inline __printf(2, 3) struct dmem_cgroup_region *
+dmem_cgroup_register_region(const struct dmem_cgroup_init *init, const char *name_fmt, ...)
 {
 	return NULL;
 }
kernel/cgroup/dmem.c +6 -4
index 39930c59cb76..eb46e99b7366 100644
@@ -502,7 +502,7 @@ EXPORT_SYMBOL_GPL(dmem_cgroup_unregister_region);
 
 /**
  * dmem_cgroup_register_region() - Register a regions for dev cgroup.
- * @size: Size of region to register, in bytes.
+ * @init: Initialization parameters for the region.
  * @fmt: Region parameters to register
  *
  * This function registers a node in the dmem cgroup with the
@@ -511,13 +511,15 @@ EXPORT_SYMBOL_GPL(dmem_cgroup_unregister_region);
  *
  * Return: NULL or a struct on success, PTR_ERR on failure.
  */
-struct dmem_cgroup_region *dmem_cgroup_register_region(u64 size, const char *fmt, ...)
+struct dmem_cgroup_region *
+dmem_cgroup_register_region(const struct dmem_cgroup_init *init,
+			    const char *fmt, ...)
 {
 	struct dmem_cgroup_region *ret;
 	char *region_name;
 	va_list ap;
 
-	if (!size)
+	if (!init || !init->size)
 		return NULL;
 
 	va_start(ap, fmt);
@@ -534,7 +536,7 @@ struct dmem_cgroup_region *dmem_cgroup_register_region(u64 size, const char *fmt
 
 	INIT_LIST_HEAD(&ret->pools);
 	ret->name = region_name;
-	ret->size = size;
+	ret->size = init->size;
 	kref_init(&ret->ref);
 
 	spin_lock(&dmemcg_lock);
Signature · 1 line
2.55.0
author maintainer
#3 lore raw

PATCH v9 3/6 cgroup/dmem: Add reclaim callback for lowering max below current usage

In reply to Thomas Hellström

Add an optional reclaim callback to struct dmem_cgroup_region. When dmem.max is set below the current usage of a cgroup pool, the new limit is applied immediately (so that concurrent allocations are throttled while reclaim is in progress) and then the driver is asked to evict memory to bring usage back below the limit.

Reclaim is attempted up to a bounded number of times. No error is returned to userspace if usage remains above the limit after reclaim, and a pending signal will abort the reclaim loop early. This matches the behavior of memory.max in the memory cgroup controller.

Also honor O_NONBLOCK so that if that flag is set during the max value write, no reclaim is initiated. The idea is to avoid charging the reclaim cost to the writer of the max value.

v2:
- Write max before reclaim is attempted (Maarten)
- Let signals abort the reclaim without error (Maarten)
- If a new max value is written with the O_NONBLOCK flag, reclaim is not attempted (Maarten)
- Extract region from the pool parameter rather than passing it explicitly to set_resource_xxx().

v3:
- Use an rw_semaphore (unregister_sem) to protect reclaim callbacks against concurrent region unregistration: readers (reclaim) hold the read side; dmem_cgroup_unregister_region() takes the write side to drain in-flight callbacks before returning. (Sashiko-bot)

v5:
- Rebased on the introduction of struct dmem_cgroup_init.
- Use nonblock=true in reset_all_resource_limits() to avoid sleeping inside rcu_read_lock() in dmemcs_offline(). (Sashiko-bot)
- Compare usage against the truncated limit value stored in cnt.max, not the original u64. (Sashiko-bot)
- Use a DMEM_MAX_RECLAIM_RETRIES (16) retry budget instead of 5, matching the memcg controller's MAX_RECLAIM_RETRIES. Only -ENOSPC (no progress) counts against the retry budget; other errors terminate the loop immediately.

v6:
- Fix dmem_cgroup_ops->reclaim docstring: -ENOSPC does not stop reclaim immediately but is retried up to DMEM_MAX_RECLAIM_RETRIES times; only other negative errors terminate the loop. (Sashiko-bot)

v7:
- Replace the per-region rw_semaphore with a static SRCU domain (dmemcg_srcu). SRCU is a better fit than rwsem for this use: it avoids the per-region lock overhead on every reclaim call, and synchronize_srcu() at unregister time is a rare operation. (Maarten)
- Trim in-function comments to focus on what rather than how.

Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 include/linux/cgroup_dmem.h | 22 ++++++++++
 kernel/cgroup/dmem.c        | 80 +++++++++++++++++++++++++++++++------
 2 files changed, 90 insertions(+), 12 deletions(-)
include/linux/cgroup_dmem.h +22 -0
index d9eab8a2c1ee..8664321fa9f7 100644
@@ -14,12 +14,34 @@ struct dmem_cgroup_pool_state;
 /* Opaque definition of a cgroup region, used internally */
 struct dmem_cgroup_region;
 
+/**
+ * struct dmem_cgroup_ops - Operations for a dmem cgroup region.
+ * @reclaim: Optional callback invoked when dmem.max is set below the current
+ *           usage of a pool. The driver should attempt to free at least
+ *           @target_bytes from @pool. May be called multiple times if usage
+ *           remains above the limit after returning.
+ *
+ *           Return: 0 if some progress was made (even if less than
+ *           @target_bytes was freed), -ENOSPC if no progress could be made
+ *           (the caller will retry up to a bounded number of times), or
+ *           another negative error code if a fatal error occurred (stops
+ *           further reclaim attempts immediately).
+ */
+struct dmem_cgroup_ops {
+	int (*reclaim)(struct dmem_cgroup_pool_state *pool,
+		       u64 target_bytes, void *priv);
+};
+
 /**
  * struct dmem_cgroup_init - Initialization parameters for a dmem cgroup region.
  * @size: Size of the region in bytes.
+ * @ops: Optional operations for this region. May be NULL.
+ * @reclaim_priv: Opaque pointer passed to @ops->reclaim. May be NULL.
  */
 struct dmem_cgroup_init {
 	u64 size;
+	const struct dmem_cgroup_ops *ops;
+	void *reclaim_priv;
 };
 
 #if IS_ENABLED(CONFIG_CGROUP_DMEM)
kernel/cgroup/dmem.c +68 -12

153 lines not on this page · raw message

Signature · 1 line
2.55.0
author maintainer
#4 lore raw

PATCH v9 4/6 drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller

In reply to Thomas Hellström

Add ttm_bo_evict_cgroup() to evict buffer objects charged to a specific dmem cgroup pool from a resource manager's LRU until a byte target is met. Add ttm_resource_manager_set_dmem_region() to associate a dmem cgroup region with a resource manager; drivers supply their own dmem_cgroup_ops with ttm_resource_manager_dmem_reclaim as the reclaim function and the manager pointer as reclaim_priv in the dmem_cgroup_init to wire up TTM eviction as the reclaim callback.

The eviction context is interruptible; signals abort the operation and propagate back through the write() syscall.

Introduce a new mode for the bo LRU walker so that sleeping locks can be taken. This can be used when the caller doesn't hold any previous dma_resv locks, and where it intends to hold at most one lock at a time.

Like the rest of the TTM eviction this should sooner than later be converted to full WW transactions.

v3:
- Fix ttm_resource_manager_set_dmem_region() storing an error pointer in man->cg unconditionally. (Sashiko-bot)
- Fix kernel-doc function name format for ttm_bo_evict_cgroup() and ttm_resource_manager_set_dmem_region().

v5:
- Rebased on the introduction of struct dmem_cgroup_init.
- Handle NULL region in ttm_resource_manager_set_dmem_region() to clear the reclaim callback, preventing use-after-free when the manager is torn down while the dmem region outlives it. (Sashiko-bot)
- Return 0 on any progress (even partial eviction), -ENOSPC only when nothing was freed; fixes callers that expected 0 on partial success.
- Document that the reclaim callback should return 0 if some progress was made, -ENOSPC if no progress at all, or another error for fatal failures.

v8:
- Fix ttm_resource_manager_set_dmem_region() using IS_ERR_OR_NULL(), which skipped the assignment for a NULL region and thus never cleared man->cg. Use IS_ERR() so that a NULL region detaches the region as the kernel-doc and the v5 changelog intended. (Sashiko-bot)

v9:
- Don't leak cgroup charges for bos that may have survived dmemcg region fini.
- Drop the misleading "Capture size before eviction in case res is cleared" comment in ttm_bo_evict_cb(). (Maarten Lankhorst)

Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 drivers/gpu/drm/ttm/ttm_bo.c       | 94 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/ttm/ttm_bo_util.c  |  3 +-
 drivers/gpu/drm/ttm/ttm_resource.c | 54 ++++++++++++++++-
 include/drm/ttm/ttm_bo.h           | 10 ++++
 include/drm/ttm/ttm_resource.h     |  7 +++
 5 files changed, 163 insertions(+), 5 deletions(-)
drivers/gpu/drm/ttm/ttm_bo.c +91 -3
index 3980f376e3ba..8d7403c714cf 100644
@@ -515,12 +515,19 @@ 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);
+	s64 bo_size = bo->base.size;
 	s64 lret;
 
 	if (!dmem_cgroup_state_evict_valuable(evict_walk->limit_pool, bo->resource->css,
 					      evict_walk->try_low, &evict_walk->hit_low))
 		return 0;
 
+	/*
+	 * evict_walk->place is NULL in cgroup drain mode.  Drivers'
+	 * eviction_valuable() callbacks must handle a NULL place, treating it
+	 * as "any placement": the TTM base implementation already does so via
+	 * ttm_resource_intersects().
+	 */
 	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
 		return 0;
 
@@ -536,11 +543,15 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
 		goto out;
 
 	evict_walk->evicted++;
-	if (evict_walk->res)
+	if (evict_walk->res) {
 		lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
 					  evict_walk->res, NULL);
-	if (lret == 0)
-		return 1;
+		if (lret == 0)
+			return 1;
+	} else {
+		/* Cgroup drain: return bytes freed for byte-denominated progress. */
+		return bo_size;
+	}
 out:
 	/* Errors that should terminate the walk. */
 	if (lret == -ENOSPC)
@@ -614,6 +625,83 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
 	return 0;
 }
 
+/**
+ * ttm_bo_evict_cgroup() - Evict buffer objects charged to a specific cgroup.
+ * @bdev: The TTM device.
+ * @man: The resource manager whose LRU to walk.
+ * @limit_pool: The cgroup pool state whose members should be evicted.
+ * @target_bytes: Number of bytes to free.
+ * @ctx: The TTM operation context.
+ *
+ * Walk the LRU of @man and evict buffer objects that are charged to the
+ * cgroup identified by @limit_pool, until at least @target_bytes have been
+ * freed.  Mirrors the two-pass (trylock -> sleeping-lock, low-watermark)
+ * strategy used by ttm_bo_evict_alloc().
+ *
+ * Return: >= @target_bytes on full success, 0..target_bytes-1 if partial,
+ *         negative error code on fatal error.
+ */
+s64 ttm_bo_evict_cgroup(struct ttm_device *bdev,
+			struct ttm_resource_manager *man,
+			struct dmem_cgroup_pool_state *limit_pool,
+			s64 target_bytes,
+			struct ttm_operation_ctx *ctx)
+{
+	struct ttm_bo_evict_walk evict_walk = {
+		.walk = {
+			.ops = &ttm_evict_walk_ops,
+			.arg = { .ctx = ctx },
+		},
+		.limit_pool = limit_pool,
+		/* place, evictor, res left NULL: selects cgroup drain mode */
+	};
+	s64 lret, pass;
+
+	evict_walk.walk.arg.trylock_only = true;
+	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, target_bytes);
+	if (lret < 0 || lret >= target_bytes)
+		return lret;
+
+	/* Second pass: also evict BOs at the low watermark. */
+	if (evict_walk.hit_low) {
+		evict_walk.try_low = true;
+		pass = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man,
+					      target_bytes - lret);
+		if (pass < 0)
+			return pass;
+		lret += pass;
+		if (lret >= target_bytes)
+			return lret;
+	}
+
+	/* Full sleeping-lock pass for remaining target. */
+	evict_walk.try_low = evict_walk.hit_low = false;
+	evict_walk.walk.arg.trylock_only = false;
+
+retry:
+	evict_walk.walk.arg.sleeping_lock = true;
+	do {
+		evict_walk.evicted = 0;
+		pass = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man,
+					      target_bytes - lret);
+		if (pass < 0) {
+			lret = pass;
+			goto out;
+		}
+		lret += pass;
+	} while (lret < target_bytes && evict_walk.evicted);
+
+	/* One more attempt if we hit the low limit during sleeping-lock pass. */
+	if (lret < target_bytes && evict_walk.hit_low && !evict_walk.try_low) {
+		evict_walk.try_low = true;
+		goto retry;
+	}
+
+out:
+	return lret;
+}
+EXPORT_SYMBOL(ttm_bo_evict_cgroup);
+
 /**
  * ttm_bo_pin - Pin the buffer object.
  * @bo: The buffer object to pin
drivers/gpu/drm/ttm/ttm_bo_util.c +2 -1
index 3e3c201a0222..bd0b23ac2cc4 100644
@@ -999,7 +999,8 @@ __ttm_bo_lru_cursor_next(struct ttm_bo_lru_cursor *curs)
 		bo = res->bo;
 		if (ttm_lru_walk_trylock(curs, bo))
 			bo_locked = true;
-		else if (!arg->ticket || arg->ctx->no_wait_gpu || arg->trylock_only)
+		else if ((!arg->ticket && !arg->sleeping_lock) || arg->ctx->no_wait_gpu ||
+			 arg->trylock_only)
 			continue;
 
 		if (!ttm_bo_get_unless_zero(bo)) {
drivers/gpu/drm/ttm/ttm_resource.c +53 -1

63 lines not on this page · raw message

include/drm/ttm/ttm_bo.h +10 -0
index 8310bc3d55f9..32791c4db2a9 100644
@@ -226,6 +226,11 @@ struct ttm_lru_walk_arg {
 	struct ww_acquire_ctx *ticket;
 	/** @trylock_only: Only use trylock for locking. */
 	bool trylock_only;
+	/**
+	 * @sleeping_lock: Use sleeping locks even with %NULL @ticket.
+	 * @trylock_only has precedence over this field.
+	 */
+	bool sleeping_lock;
 };
 
 /**
@@ -431,6 +436,11 @@ void ttm_bo_unpin(struct ttm_buffer_object *bo);
 int ttm_bo_evict_first(struct ttm_device *bdev,
 		       struct ttm_resource_manager *man,
 		       struct ttm_operation_ctx *ctx);
+s64 ttm_bo_evict_cgroup(struct ttm_device *bdev,
+			struct ttm_resource_manager *man,
+			struct dmem_cgroup_pool_state *limit_pool,
+			s64 target_bytes,
+			struct ttm_operation_ctx *ctx);
 int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long offset,
 		  void *buf, int len, int write);
 vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
include/drm/ttm/ttm_resource.h +7 -0
index a5d386583fb6..32e485fdce9a 100644
@@ -39,6 +39,7 @@
 
 struct dentry;
 struct dmem_cgroup_device;
+struct dmem_cgroup_region;
 struct drm_printer;
 struct ttm_device;
 struct ttm_resource_manager;
@@ -477,6 +478,12 @@ void ttm_resource_manager_init(struct ttm_resource_manager *man,
 			       struct ttm_device *bdev,
 			       uint64_t size);
 
+void ttm_resource_manager_set_dmem_region(struct ttm_resource_manager *man,
+					  struct dmem_cgroup_region *region);
+
+int ttm_resource_manager_dmem_reclaim(struct dmem_cgroup_pool_state *pool,
+				      u64 target_bytes, void *priv);
+
 int ttm_resource_manager_evict_all(struct ttm_device *bdev,
 				   struct ttm_resource_manager *man);
 
Signature · 1 line
2.55.0

A message that is not in the archive (sent off-list or not yet received): [email protected]

author maintainer
#6 lore raw

Re: [PATCH v9 4/6] drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller

Quoted 23 lines · On 7/25/26 12:22, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch introduces a cgroup eviction path that passes place = NULL to drivers' eviction_valuable callbacks, but fails to update AMDGPU's implementation, leading to a NULL pointer dereference.
- [Medium] The patch falsely claims to prevent a use-after-free by clearing the reclaim callback, but it only nullifies a local pointer (man->cg), leaving the callback registered. --

--- Patch [4]: [PATCH v9 4/6] drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller --- commit 239e3794b12b4c52556c630073a7d4ef2e00e721
Author: Thomas Hellström <[email protected]>
drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller

This commit adds functions to evict buffer objects charged to a specific dmem cgroup pool until a target size is met. It also introduces a new mode for the buffer object LRU walker to support sleeping locks.

Quoted 5 lines

v5:
- Rebased on the introduction of struct dmem_cgroup_init.
- Handle NULL region in ttm_resource_manager_set_dmem_region() to clear the reclaim callback, preventing use-after-free when the manager is torn down while the dmem region outlives it. (Sashiko-bot)

[Severity: Medium]
The commit message states that a NULL region clears the reclaim callback to prevent a use-after-free. Is this description accurate?

Quoted 7 lines

Looking at ttm_resource_manager_set_dmem_region(), it only sets man->cg to the new region (or NULL). It does not appear to unregister the callback from the dmem_cgroup subsystem.

If the callback remains registered, could the subsystem still invoke ttm_resource_manager_dmem_reclaim() with a freed manager pointer, causing a use-after-free when man->bdev is accessed?

The dmem region needs to be unregistered before the manager is taken down, So while the v5 changelog is not up-to-date, this is a false positive.

Quoted 39 lines
Quoted 5 lines
drivers/gpu/drm/ttm/ttm_bo.c +0 -0
index 3980f376e3ba4..8d7403c714cf0 100644

[ ... ]

Quoted 9 lines
drivers/gpu/drm/ttm/ttm_bo.c +1 -0
@@ -515,12 +515,19 @@ 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);
+	s64 bo_size = bo->base.size;
  	s64 lret;
 
  	if (!dmem_cgroup_state_evict_valuable(evict_walk->limit_pool, bo->resource->css,
  					      evict_walk->try_low, &evict_walk->hit_low))

drivers/gpu/drm/ttm/ttm_bo.c static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object * · line 532

Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo.c +6 -0
  		return 0;
 
+	/*
+	 * evict_walk->place is NULL in cgroup drain mode.  Drivers'
+	 * eviction_valuable() callbacks must handle a NULL place, treating it
+	 * as "any placement": the TTM base implementation already does so via
+	 * ttm_resource_intersects().
+	 */
  	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
  		return 0;

[Severity: Medium]
Will passing a NULL place here cause a crash in the AMDGPU driver?

Looking at amdgpu_ttm_bo_eviction_valuable(), it appears to unconditionally dereference the place pointer:

amdgpu_ttm_bo_eviction_valuable() {
     ...
     if (amdkfd_fence_check_mm(f, current->mm) &&
         !(place->flags & TTM_PL_FLAG_CONTIGUOUS))
         return false;
     ...
}
Quoted 2 lines

Does the AMDGPU implementation need to be updated to handle a NULL place before this cgroup eviction path can be safely used?

This is done in patch 6, where the place=NULL is introudced for amdgpu. False positive.

Quoted 22 lines
Quoted 5 lines
drivers/gpu/drm/ttm/ttm_resource.c +0 -0
index 154d6739256f8..1417a09b767c4 100644

[ ... ]

drivers/gpu/drm/ttm/ttm_resource.c void ttm_resource_manager_create_debugfs(struct ttm_resource_manager *man, · line 1007

Quoted 7 lines
drivers/gpu/drm/ttm/ttm_resource.c +7 -0
+void ttm_resource_manager_set_dmem_region(struct ttm_resource_manager *man,
+					  struct dmem_cgroup_region *region)
+{
+	if (!IS_ERR(region))
+		man->cg = region;
+}
+EXPORT_SYMBOL(ttm_resource_manager_set_dmem_region);

[Severity: Medium]
As mentioned regarding the commit message, does this function fully detach the region?

Since this only nullifies the local man->cg pointer, the dmem_cgroup subsystem might still hold the ttm_resource_manager_dmem_reclaim callback. Would an explicit unregistration step be required here to safely tear down the manager?

author maintainer
#7 lore raw

PATCH v9 5/6 drm/xe: Wire up dmem cgroup reclaim for VRAM manager

In reply to Thomas Hellström

Register the VRAM manager with the dmem cgroup reclaim infrastructure so that lowering dmem.max below current VRAM usage triggers TTM eviction rather than failing with -EBUSY.

v4:
- Rebased on drm-tip; dropped the XE_PL_STOLEN guard as stolen memory uses a separate TTM manager and never calls __xe_ttm_vram_mgr_init().

v5:
- Rebased on the introduction of struct dmem_cgroup_init.
- Register the fini drmm action before drmm_cgroup_register_region() so that devres LIFO teardown runs unregister_region() first (draining any in-flight reclaim callbacks via the rwsem) and xe_ttm_vram_mgr_fini() second, ensuring the manager is never accessed by a reclaim callback after teardown. (Sashiko-bot)
- Wrap the reclaim callback in xe_ttm_vram_mgr_dmem_reclaim() using drm_dev_enter()/drm_dev_exit() to prevent TTM reclaim from running after driver unbind.

v9:
- Close ttm_resource_manager_set_used() races around vram manager init and fini.
- Formatting fix.
- Acquire a runtime PM reference around the TTM reclaim call in xe_ttm_vram_mgr_dmem_reclaim(). (Sashiko-bot)

Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 drivers/gpu/drm/xe/xe_ttm_vram_mgr.c | 72 +++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 12 deletions(-)
drivers/gpu/drm/xe/xe_ttm_vram_mgr.c +60 -12
index 308fda4248eb..05911904c1f9 100644
@@ -15,6 +15,7 @@
 
 #include "xe_bo.h"
 #include "xe_device.h"
+#include "xe_pm.h"
 #include "xe_res_cursor.h"
 #include "xe_ttm_vram_mgr.h"
 #include "xe_vram_types.h"
@@ -276,14 +277,47 @@ static const struct ttm_resource_manager_func xe_ttm_vram_mgr_func = {
 	.debug	= xe_ttm_vram_mgr_debug
 };
 
+static const struct dmem_cgroup_ops xe_ttm_vram_mgr_dmem_ops;
+
+static int xe_ttm_vram_mgr_dmem_reclaim(struct dmem_cgroup_pool_state *pool,
+					u64 target_bytes, void *priv)
+{
+	struct ttm_resource_manager *man = priv;
+	struct xe_device *xe = ttm_to_xe_device(man->bdev);
+	int ret, idx;
+
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	{
+		ACQUIRE(xe_pm_runtime_ioctl, pm)(xe);
+
+		ret = ACQUIRE_ERR(xe_pm_runtime_ioctl, &pm);
+		if (ret >= 0)
+			ret = ttm_resource_manager_dmem_reclaim(pool, target_bytes, priv);
+	}
+
+	drm_dev_exit(idx);
+	return ret;
+}
+
+static const struct dmem_cgroup_ops xe_ttm_vram_mgr_dmem_ops = {
+	.reclaim = xe_ttm_vram_mgr_dmem_reclaim,
+};
+
+static void xe_ttm_vram_mgr_set_unused(struct drm_device *dev, void *arg)
+{
+	struct ttm_resource_manager *man = arg;
+
+	ttm_resource_manager_set_used(man, false);
+}
+
 static void xe_ttm_vram_mgr_fini(struct drm_device *dev, void *arg)
 {
 	struct xe_device *xe = to_xe_device(dev);
 	struct xe_ttm_vram_mgr *mgr = arg;
 	struct ttm_resource_manager *man = &mgr->manager;
 
-	ttm_resource_manager_set_used(man, false);
-
 	if (ttm_resource_manager_evict_all(&xe->ttm, man))
 		return;
 
@@ -301,17 +335,10 @@ int __xe_ttm_vram_mgr_init(struct xe_device *xe, struct xe_ttm_vram_mgr *mgr,
 			   u64 default_page_size)
 {
 	struct ttm_resource_manager *man = &mgr->manager;
+	struct dmem_cgroup_region *cg;
 	const char *name;
 	int err;
 
-	name = mem_type == XE_PL_VRAM0 ? "vram0" : "vram1";
-	man->cg = drmm_cgroup_register_region(&xe->drm, name,
-					      &(struct dmem_cgroup_init){
-						.size = size,
-					      });
-	if (IS_ERR(man->cg))
-		return PTR_ERR(man->cg);
-
 	man->func = &xe_ttm_vram_mgr_func;
 	mgr->mem_type = mem_type;
 	err = drmm_mutex_init(&xe->drm, &mgr->lock);
@@ -328,9 +355,30 @@ int __xe_ttm_vram_mgr_init(struct xe_device *xe, struct xe_ttm_vram_mgr *mgr,
 
 	gpu_buddy_driver_set_lock(&mgr->mm, &mgr->lock);
 	ttm_set_driver_manager(&xe->ttm, mem_type, &mgr->manager);
-	ttm_resource_manager_set_used(&mgr->manager, true);
 
-	return drmm_add_action_or_reset(&xe->drm, xe_ttm_vram_mgr_fini, mgr);
+	/*
+	 * Register the fini action before the cgroup region so that devres
+	 * LIFO teardown runs unregister_region before manager teardown
+	 * (draining any in-flight reclaim callbacks) and the manager fini second.
+	 */
+	err = drmm_add_action_or_reset(&xe->drm, xe_ttm_vram_mgr_fini, mgr);
+	if (err)
+		return err;
+
+	name = mem_type == XE_PL_VRAM0 ? "vram0" : "vram1";
+	cg = drmm_cgroup_register_region(&xe->drm, name,
+					 &(struct dmem_cgroup_init){
+						.size = size,
+						.ops = &xe_ttm_vram_mgr_dmem_ops,
+						.reclaim_priv = man,
+					 });
+	if (IS_ERR(cg))
+		return PTR_ERR(cg);
+
+	ttm_resource_manager_set_dmem_region(man, cg);
+	ttm_resource_manager_set_used(man, true);
+
+	return drmm_add_action_or_reset(&xe->drm, xe_ttm_vram_mgr_set_unused, man);
 }
 
 /**
Signature · 1 line
2.55.0
author maintainer
#8 lore raw

PATCH v9 6/6 drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

In reply to Thomas Hellström

Register the VRAM manager with the dmem cgroup reclaim infrastructure so that lowering dmem.max below current VRAM usage triggers TTM eviction rather than failing with -EBUSY.

Guard place->flags in amdgpu_ttm_bo_eviction_valuable() against NULL, as the TTM reclaim path passes a NULL place in cgroup drain mode.

Use drmm_cgroup_register_region() so that the region is automatically unregistered at DRM device release, after drm_dev_unplug() has already made drm_dev_enter() return false. The drm_dev_enter/exit guard in the reclaim callback ensures no reclaim work touches the TTM manager after driver unbind, closing the window between vram_mgr_fini() (called from drm_driver.release) and the drmm cleanup that unregisters the region.

v3:
- Rebased on fix for uninitialized list and buddy allocator on the drmm_cgroup_register_region() error path.

v5:
- Rebased on the introduction of struct dmem_cgroup_init.
- Clear the reclaim callback in amdgpu_vram_mgr_fini() to prevent use-after-free if cgroup reclaim is triggered after driver unbind while userspace holds an open DRM file descriptor. (Sashiko-bot)
- Switch from drmm_cgroup_register_region() to the raw dmem_cgroup_register_region() and store the region in amdgpu_vram_mgr.cg_region. Call dmem_cgroup_unregister_region() in amdgpu_vram_mgr_fini() after ttm_resource_manager_evict_all() to drain in-flight reclaim callbacks, and clear man->cg afterwards. This is required because amdgpu's vram manager fini is called explicitly during driver unbind, which may precede the DRM device release and thus precede any drmm-based cleanup. (Sashiko-bot)

v6:
- Fix mgr->cg_region never being assigned, so dmem_cgroup_unregister_region() in fini silently no-ops on NULL and leaks the region. (Sashiko-bot)
- Reorder fini to call set_used(false) and evict_all() before dmem_cgroup_unregister_region(), so ttm_resource_free() can uncharge via man->cg during eviction; clear man->cg after unregister. (Sashiko-bot)

v7:
- Move dmem_cgroup_unregister_region() before the early return on evict_all() failure; not doing so leaves a dangling reclaim callback pointing to the partially-torn-down VRAM manager, causing a use-after-free when the cgroup later triggers reclaim. (Sashiko-bot)
- Switch back to drmm_cgroup_register_region() with a drm_dev_enter/ exit guard in the reclaim callback (matching xe), rather than manual register/unregister. drm_dev_unplug() fires before vram_mgr_fini(), so drm_dev_enter() returning false prevents any reclaim from touching the manager during teardown. This also fixes the "vram" name collision on multi-GPU systems, since drmm_cgroup_register_region() automatically prefixes with "drm/<pci-addr>/". (Sashiko-bot)

v8:
- Move the dmem cgroup region teardown back into amdgpu_vram_mgr_fini(): register the region with dmem_cgroup_register_region() (keeping the "drm/<unique>/vram" prefix), store it in amdgpu_vram_mgr.cg_region, and unregister it explicitly after ttm_resource_manager_evict_all(). The v7 drmm-only approach left a use-after-free on the probe-error teardown path: drm_dev_unplug() is not called there, so the drm_dev_enter() guard stays open while drm_driver.release destroys the manager before the drmm action unregisters the region. The explicit unregister drains in-flight reclaim on every teardown path; the drm_dev_enter() guard is kept as defense against reclaim after unplug. (Sashiko-bot)

v9:
- Don't leak a gpu buddy manager on cgroup init failure. (Sashiko-bot)
- Acquire a runtime PM reference (pm_runtime_get_sync/put_autosuspend) around the TTM reclaim call in amdgpu_vram_mgr_dmem_reclaim(). (Sashiko-bot)

Assisted-by GitHub_Copilot:claude-sonnet-4.6
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 64 ++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h |  3 +
 3 files changed, 63 insertions(+), 6 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +1 -1
index 7920675af1d1..b46b1d19349e 100644
@@ -1507,7 +1507,7 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 	dma_resv_for_each_fence(&resv_cursor, bo->base.resv,
 				DMA_RESV_USAGE_BOOKKEEP, f) {
 		if (amdkfd_fence_check_mm(f, current->mm) &&
-		    !(place->flags & TTM_PL_FLAG_CONTIGUOUS))
+		    !(place && (place->flags & TTM_PL_FLAG_CONTIGUOUS)))
 			return false;
 	}
 
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +59 -5

95 lines not on this page · raw message

drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h +3 -0
index 429a21a2e9b2..e581cbf52fcb 100644
@@ -26,6 +26,8 @@
 
 #include <linux/gpu_buddy.h>
 
+struct dmem_cgroup_region;
+
 struct amdgpu_vram_mgr {
 	struct ttm_resource_manager manager;
 	struct gpu_buddy mm;
@@ -36,6 +38,7 @@ struct amdgpu_vram_mgr {
 	atomic64_t vis_usage;
 	u64 default_page_size;
 	struct list_head allocated_vres_list;
+	struct dmem_cgroup_region *cg_region;
 };
 
 struct amdgpu_vres_task {
Signature · 1 line
2.55.0

A message that is not in the archive (sent off-list or not yet received): [email protected]

author maintainer
#10 lore raw

Re: [PATCH v9 6/6] drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

Quoted 60 lines · On 7/25/26 12:34, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The explicit unregistration of the dmem cgroup region in amdgpu_vram_mgr_fini() misses the probe-error teardown path, leading to a memory leak and a critical use-after-free. --

--- Patch [6]: [PATCH v9 6/6] drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager --- commit 5968e79644a4b5720ca50af075e6cce858a1f21a
Author: Thomas Hellström <[email protected]>

drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

Register the VRAM manager with the dmem cgroup reclaim infrastructure so that lowering dmem.max below current VRAM usage triggers TTM eviction rather than failing with -EBUSY.

Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +0 -0
index 08f05c3aed1d5..249f07f309515 100644

[ ... ]

Quoted 9 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +5 -0
@@ -963,6 +1004,19 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev)
  	ttm_resource_manager_set_used(man, false);
 
  	ret = ttm_resource_manager_evict_all(&adev->mman.bdev, man);
+
+	/*
+	 * Unregister the dmem cgroup region regardless of the evict_all()
+	 * result and before any further teardown.  This drains in-flight
+	 * reclaim callbacks and blocks new ones, so no reclaim can reference

drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) · line 1021

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +8 -0
+	 * the manager once we start freeing it.  It must run after evict_all()
+	 * so that ttm_resource_free() can still uncharge via man->cg during
+	 * eviction.  Clear man->cg afterwards.
+	 */
+	dmem_cgroup_unregister_region(mgr->cg_region);
+	mgr->cg_region = NULL;
+	ttm_resource_manager_set_dmem_region(man, NULL);
+
  	if (ret)
  		return;

[Severity: High]
Does explicit unregistration in amdgpu_vram_mgr_fini() miss the probe error teardown path?

If an initialization step fails after amdgpu_vram_mgr_init() succeeds, such as amdgpu_gtt_mgr_init() failing inside amdgpu_ttm_init():

amdgpu_ttm.c:amdgpu_ttm_init() {
     ...
     r = amdgpu_vram_mgr_init(adev);
     if (r)
         return r;

     r = amdgpu_gtt_mgr_init(adev, gtt_size);
     if (r) {
         dev_err(adev->dev, "Failed initializing GTT heap.\n");
         return r;
     }
     ...
}
Quoted 1 line

The function returns an error without cleaning up the VRAM manager.

This is a pre-existing error. While this patch introduces yet another vulnerability related to this, its fix is not in the scope of this series, and if this is considered a no-go, better hold off this patch until the underlying issue is fixed.

/Thomas

Quoted 21 lines

Because the AMDGPU IP block init sequence sees the sw_init failure, it leaves status.sw = false for the GMC block.

During the subsequent teardown in amdgpu_device_fini_sw(), the sw_fini callback for the GMC block is skipped because status.sw is false. Consequently, amdgpu_bo_fini() and amdgpu_vram_mgr_fini() are never executed.

Since amdgpu_vram_mgr_fini() is bypassed, the dmem cgroup region is never unregistered. When the driver probe fails, the adev structure is freed by devm_drm_dev_alloc teardown.

The global dmem_cgroup_regions list then retains the region with a reclaim_priv pointer pointing to the freed ttm_resource_manager. If memory pressure later triggers cgroup reclaim, it would call amdgpu_vram_mgr_dmem_reclaim() with the freed pointer, potentially leading to a use-after-free regression.

Is there a way to ensure the region is unregistered during probe failures?

Tested-by on the v9 series
#11 lore raw
In reply to Thomas Hellström
Quoted 105 lines · Thomas Hellström

When writing a "max" limit lower than the current usage, the existing code silently failed. This series aims to improve on that by returning -EBUSY on failure and also attempt to synchronously reclaim device memory to push the usage under the new max limit to avoid the error.

Patch 1 fixes a pre-existing amdgpu_vram_mgr_init() error path Patch 2 introduces struct dmem_cgroup_init for extensible region
registration.
Patch 3 implements and documents a reclaim callback interface
for the dmem controller.
Patch 4 implements a TTM reclaim callback.
Patches 5-6 hook up the reclaim callback to the dmem cgroup-aware
drivers xe and amdgpu.

v2:
- Remove the error propagation that was in a previous series (Maarten)
- A number of updates in patch 1. See its commit message for details (Maarten)

v3:
- Add patch 1 fixing a pre-existing amdgpu_vram_mgr_init() error path bug where drmm_cgroup_register_region() was called before INIT_LIST_HEAD() and gpu_buddy_init(), causing a kernel panic on failure. (Sashiko-bot)
- Use an rwsem to protect reclaim callback registration and region unregister against concurrent reclaim invocations. (Sashiko-bot)
- Fix ttm_resource_manager_set_dmem_region() storing an error pointer in man->cg unconditionally. (Sashiko-bot)
- Fix kernel-doc function name format for ttm_bo_evict_cgroup() and ttm_resource_manager_set_dmem_region().

v4:
- Rebased on drm-tip; dropped the XE_PL_STOLEN guard in the xe patch as stolen memory uses a separate TTM manager.

v5:
- Add patch 2 introducing struct dmem_cgroup_init to make the dmem_cgroup_register_region() API extensible without adding positional arguments in the future.
- Use nonblock=true in reset_all_resource_limits() to avoid sleeping inside rcu_read_lock() in dmemcs_offline(). (Sashiko-bot)
- Compare usage against the truncated limit stored in cnt.max, not the original u64. (Sashiko-bot)
- Use DMEM_MAX_RECLAIM_RETRIES (16) retry budget instead of 5, matching the memcg controller; only -ENOSPC (no progress) counts against the budget, other errors abort immediately.
- Handle NULL region in ttm_resource_manager_set_dmem_region() to clear the reclaim callback, preventing use-after-free when the manager is torn down while the dmem region outlives it. (Sashiko-bot)
- Return 0 on any eviction progress; reserve -ENOSPC for zero progress.
- Clear the reclaim callback in xe and amdgpu fini paths to prevent use-after-free after driver unbind with open DRM file descriptors. (Sashiko-bot)
- Register xe fini devres action before drmm_cgroup_register_region() so LIFO teardown runs unregister first, draining callbacks before the manager is destroyed. (Sashiko-bot)
- Switch amdgpu to explicit dmem_cgroup_unregister_region() at the top of amdgpu_vram_mgr_fini() before any manager teardown, since amdgpu's fini is called explicitly during driver unbind before drmm cleanup. (Sashiko-bot)
- Wrap the xe reclaim callback with drm_dev_enter()/drm_dev_exit() to prevent TTM reclaim from running after driver unbind.

v6:
- Move the ops check inside down_read() in set_resource_max(), guarded by region->unregistered, to close a UAF race against dmem_cgroup_unregister_region(). (Sashiko-bot)
- Fix dmem_cgroup_ops->reclaim docstring: -ENOSPC is retried up to DMEM_MAX_RECLAIM_RETRIES times, not an immediate stop. (Sashiko-bot)
- Fix mgr->cg_region never being assigned in amdgpu_vram_mgr_init(), causing dmem_cgroup_unregister_region() in fini to silently no-op. (Sashiko-bot)
- Reorder amdgpu_vram_mgr_fini() to call set_used(false) and evict_all() before dmem_cgroup_unregister_region(), so ttm_resource_free() can uncharge via man->cg during eviction; clear man->cg after unregister. (Sashiko-bot)

v7:
- Replace the per-region rw_semaphore with a static SRCU domain (dmemcg_srcu). SRCU is a better fit: it avoids per-region lock overhead on every reclaim call, and synchronize_srcu() at unregister time is a rare, shutdown-time operation. (Maarten)
- Trim in-function comments to focus on what rather than how.
- Switch back to drmm_cgroup_register_region() with a drm_dev_enter/ exit guard in the reclaim callback (matching xe), rather than manual register/unregister. drm_dev_unplug() fires before vram_mgr_fini(), so drm_dev_enter() returning false prevents any reclaim from touching the manager during teardown. This also fixes the "vram" name collision on multi-GPU systems, since drmm_cgroup_register_region() automatically prefixes with "drm/<pci-addr>/". (Sashiko-bot)

v8:
- Fix ttm_resource_manager_set_dmem_region() using IS_ERR_OR_NULL(), which skipped the assignment for a NULL region and thus never cleared man->cg. Use IS_ERR() so that a NULL region detaches the region as the kernel-doc and the v5 changelog intended. (Sashiko-bot)
- Move the amdgpu dmem cgroup region teardown back into amdgpu_vram_mgr_fini() with an explicit dmem_cgroup_unregister_region() after evict_all(), keeping the drm_dev_enter() guard. The v7 drmm-only approach left a use-after-free on the probe-error teardown path, where drm_dev_unplug() is never called so the drm_dev_enter() guard stays open while drm_driver.release destroys the manager before the drmm action unregisters the region. (Sashiko-bot)

Quoted 8 lines

v9:
- Init / fini race fixes. (Sashiko-bot, Thomas)
- Drop a misleading comment. (Maarten)
- Acquire runtime PM before reclaim. (Sashiko-bot)

User-space tests are at
https://patchwork.freedesktop.org/series/163935/

I used https://gitlab.freedesktop.org/cascardo/igt-gpu-tools/-/commits/dmem_max?ref_type=heads,
on top of my v5 to test this version.

Quoted 30 lines

Test-with: [email protected]

Thomas Hellström (6):
drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init() cgroup/dmem: Introduce struct dmem_cgroup_init for region
initialization
cgroup/dmem: Add reclaim callback for lowering max below current usage drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem
controller
drm/xe: Wire up dmem cgroup reclaim for VRAM manager drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 65 +++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h |  3 +
 drivers/gpu/drm/drm_drv.c                    |  8 +-
 drivers/gpu/drm/ttm/ttm_bo.c                 | 94 +++++++++++++++++++-
 drivers/gpu/drm/ttm/ttm_bo_util.c            |  3 +-
 drivers/gpu/drm/ttm/ttm_resource.c           | 54 ++++++++++-
 drivers/gpu/drm/xe/xe_ttm_vram_mgr.c         | 71 +++++++++++++--
 include/drm/drm_drv.h                        |  4 +-
 include/drm/ttm/ttm_bo.h                     | 10 +++
 include/drm/ttm/ttm_resource.h               |  7 ++
 include/linux/cgroup_dmem.h                  | 38 +++++++-
 kernel/cgroup/dmem.c                         | 90 +++++++++++++++----
 13 files changed, 408 insertions(+), 41 deletions(-)
Signature · 1 line
2.55.0