[PATCH] cgroup/dmem: return error when failing to set dmem.max

[PATCH] cgroup/dmem: return error when failing to set dmem.max

[Thadeu Lima de Souza Cascardo]

page_counter_set_max may return -EBUSY in case the current usage is above
the new max. When writing to dmem.max, this error is ignored and the new
max is not set.

Return as soon as setting one of the regions max limit fails. This keeps
with the current behavior of returning when one of the region names is not
valid.

After this fix, setting a max value below the current value returns -EBUSY.

 # cat dmem.current
 drm/0000:04:00.0/vram 1060864
 # echo drm/0000:04:00.0/vram 0 > dmem.max
 -bash: echo: write error: Device or resource busy
 #

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 kernel/cgroup/dmem.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
index 9d95824dc6fa09422274422313b63c25986596de..3e6d4c0b26a1f972b6c6e875f274a091fc9e2b75 100644
--- a/kernel/cgroup/dmem.c
+++ b/kernel/cgroup/dmem.c
@@ -144,22 +144,24 @@ static void free_cg_pool(struct dmem_cgroup_pool_state *pool)
 	dmemcg_pool_put(pool);
 }
 
-static void
+static int
 set_resource_min(struct dmem_cgroup_pool_state *pool, u64 val)
 {
 	page_counter_set_min(&pool->cnt, val);
+	return 0;
 }
 
-static void
+static int
 set_resource_low(struct dmem_cgroup_pool_state *pool, u64 val)
 {
 	page_counter_set_low(&pool->cnt, val);
+	return 0;
 }
 
-static void
+static int
 set_resource_max(struct dmem_cgroup_pool_state *pool, u64 val)
 {
-	page_counter_set_max(&pool->cnt, val);
+	return page_counter_set_max(&pool->cnt, val);
 }
 
 static u64 get_resource_low(struct dmem_cgroup_pool_state *pool)
@@ -726,7 +728,7 @@ static int dmemcg_parse_limit(char *options, struct dmem_cgroup_region *region,
 
 static ssize_t dmemcg_limit_write(struct kernfs_open_file *of,
 				 char *buf, size_t nbytes, loff_t off,
-				 void (*apply)(struct dmem_cgroup_pool_state *, u64))
+				 int (*apply)(struct dmem_cgroup_pool_state *, u64))
 {
 	struct dmemcg_state *dmemcs = css_to_dmemcs(of_css(of));
 	int err = 0;
@@ -773,7 +775,7 @@ static ssize_t dmemcg_limit_write(struct kernfs_open_file *of,
 		}
 
 		/* And commit */
-		apply(pool, new_limit);
+		err = apply(pool, new_limit);
 		dmemcg_pool_put(pool);
 
 out_put:

---
base-commit: f338e77383789c0cae23ca3d48adcc5e9e137e3c
change-id: 20260318-dmem_max_ebusy-bfd33564f2c3

Best regards,
-- 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Re: [PATCH] cgroup/dmem: return error when failing to set dmem.max

[Tejun Heo]

On Wed, Mar 18, 2026 at 04:34:17PM -0300, Thadeu Lima de Souza Cascardo wrote:
> page_counter_set_max may return -EBUSY in case the current usage is above
> the new max. When writing to dmem.max, this error is ignored and the new
> max is not set.
> 
> Return as soon as setting one of the regions max limit fails. This keeps
> with the current behavior of returning when one of the region names is not
> valid.

Ugh, I don't know why dmemcg_limit_write() is trying to handle multi-line
inputs. After this, there's no atomicity w.r.t. failures either, so this
seems entirely pointless. I'd much prefer to strip out the multiline
handling.

Thanks.

-- 
tejun

Re: [PATCH] cgroup/dmem: return error when failing to set dmem.max

[Maarten Lankhorst]

Hey,

Den 2026-03-18 kl. 20:34, skrev Thadeu Lima de Souza Cascardo:
> page_counter_set_max may return -EBUSY in case the current usage is above
> the new max. When writing to dmem.max, this error is ignored and the new
> max is not set.
> 
> Return as soon as setting one of the regions max limit fails. This keeps
> with the current behavior of returning when one of the region names is not
> valid.
> 
> After this fix, setting a max value below the current value returns -EBUSY.
> 
>  # cat dmem.current
>  drm/0000:04:00.0/vram 1060864
>  # echo drm/0000:04:00.0/vram 0 > dmem.max
>  -bash: echo: write error: Device or resource busy
>  #
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
The semantics of dmemcg should not substantially differ from the memory cgroup
controller. I believe the memory cgroup controller does allow setting a lower
max, and will evict until below the new max.

See mm/memcontrol.c:memory_max_write

We should probably do the same in dmemcg instead, although we currently have no
mechanism to evict, setting a new lower max at least prevents future allocations
from failing.

I believe we should have a similar loop in dmemcg, and allow ttm to evict until
the new max is reached.

Kind regards,
~Maarten Lankhorst

Re: [PATCH] cgroup/dmem: return error when failing to set dmem.max

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Thu, Mar 19, 2026 at 08:33:12AM +0100, Maarten Lankhorst <dev@lankhorst.se> wrote:
> The semantics of dmemcg should not substantially differ from the memory cgroup
> controller. I believe the memory cgroup controller does allow setting a lower
> max, and will evict until below the new max.
> 
> See mm/memcontrol.c:memory_max_write
> 
> We should probably do the same in dmemcg instead, although we currently have no
> mechanism to evict, setting a new lower max at least prevents future allocations
> from failing.

+1

Yes, if the dmem resource is preemptible, the limit decrement should take an
action to fullfill the limit (like with memory.max).
Even as non-preemptible resource, the behavior could be more consistent
with misc controller that allows "storing" any value (with the effect of
preventing further growth).

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Claude review: cgroup/dmem: return error when failing to set dmem.max

[Claude Code Review Bot]

Overall Series Review

Subject: cgroup/dmem: return error when failing to set dmem.max
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
Patches: 4
Reviewed: 2026-03-22T05:01:51.798365

---

This is a single-patch fix for the `cgroup/dmem` subsystem. The bug is clear and well-described: `page_counter_set_max()` can return `-EBUSY` when the current usage exceeds the requested new max, but the return value was being silently discarded by `set_resource_max()` (which returned `void`). The fix is straightforward and correct in approach — propagate the error from `page_counter_set_max()` back to userspace.

The patch is small, well-scoped, and the commit message includes a nice before/after demonstration. Overall this is a good bugfix.

---
Generated by Claude Code Patch Reviewer

Claude review: cgroup/dmem: return error when failing to set dmem.max

[Claude Code Review Bot]

Patch Review

**Correctness of the core fix:** The key change at line 121 of the diff is correct:

```c
-	page_counter_set_max(&pool->cnt, val);
+	return page_counter_set_max(&pool->cnt, val);
```

`page_counter_set_max()` returns `int` (confirmed at `include/linux/page_counter.h:86`), and can return `-EBUSY`. Propagating this is the right thing to do.

**Signature change is appropriate:** Changing the `apply` callback from `void (*apply)(...)` to `int (*apply)(...)` and updating `set_resource_min`/`set_resource_low` to return `0` is a clean way to handle this uniformly.

**Potential issue — partial application on multi-region writes:** Looking at the loop in `dmemcg_limit_write()` (line 734 of the existing code: `while (buf && !err)`), when a user writes multiple region limits in a single write (multiple lines), an error from `apply()` on one region will stop processing subsequent regions. The commit message acknowledges this ("Return as soon as setting one of the regions max limit fails"). However, consider that **regions processed before the failing one have already been committed** — the function does not roll back previously applied limits. This is a pre-existing pattern (same happens for parse errors), but worth noting that the semantics are "apply until first failure" with no rollback.

**Minor note — `err` after `apply` vs `goto out_put`:** Looking at the patched code:

```c
		err = apply(pool, new_limit);
		dmemcg_pool_put(pool);

out_put:
		kref_put(&region->ref, dmemcg_free_region);
```

When `apply()` returns an error, the code still correctly calls `dmemcg_pool_put(pool)` and `kref_put()` before the `while` loop condition `!err` terminates the loop. This is correct — no resource leak.

**Nit — `set_resource_min` and `set_resource_low`:** `page_counter_set_min()` and `page_counter_set_low()` are `void` functions, so wrapping them with `return 0` is fine. If these ever gain error returns in the future, this patch has already set up the plumbing to handle it. No issue here.

**Verdict:** The patch looks correct and ready to apply. The fix is minimal and appropriate. 

Reviewed-by quality: **Good to go**, no blocking issues.

---
Generated by Claude Code Patch Reviewer