public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] ttm/pool: Use sentinels in debugfs
@ 2026-04-27 16:22 Mario Limonciello
  2026-04-28  4:24 ` Claude review: " Claude Code Review Bot
  2026-04-28  4:24 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Mario Limonciello @ 2026-04-27 16:22 UTC (permalink / raw)
  To: Christian König; +Cc: dri-devel, Mario Limonciello

The values in the `page_pool_shrink` debugfs file should recognize
the sentinels rather than causing an underflow.

Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5218
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/ttm/ttm_pool.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 26a3689e5fd90..4e83f4978b136 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -1335,15 +1335,22 @@ static int ttm_pool_debugfs_shrink_show(struct seq_file *m, void *data)
 		.gfp_mask = GFP_NOFS,
 		.nr_to_scan = TTM_SHRINKER_BATCH,
 	};
-	unsigned long count;
+	unsigned long count, scanned;
 	int nid;
 
 	fs_reclaim_acquire(GFP_KERNEL);
 	for_each_node(nid) {
 		sc.nid = nid;
 		count = ttm_pool_shrinker_count(mm_shrinker, &sc);
-		seq_printf(m, "%d: %lu/%lu\n", nid, count,
-			   ttm_pool_shrinker_scan(mm_shrinker, &sc));
+		scanned = ttm_pool_shrinker_scan(mm_shrinker, &sc);
+
+		/* Convert shrinker API sentinel values to 0 for debugfs output */
+		if (count == SHRINK_EMPTY)
+			count = 0;
+		if (scanned == SHRINK_STOP)
+			scanned = 0;
+
+		seq_printf(m, "%d: %lu/%lu\n", nid, count, scanned);
 	}
 	fs_reclaim_release(GFP_KERNEL);
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: ttm/pool: Use sentinels in debugfs
  2026-04-27 16:22 [PATCH] ttm/pool: Use sentinels in debugfs Mario Limonciello
@ 2026-04-28  4:24 ` Claude Code Review Bot
  2026-04-28  4:24 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:24 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: ttm/pool: Use sentinels in debugfs
Author: Mario Limonciello <mario.limonciello@amd.com>
Patches: 1
Reviewed: 2026-04-28T14:24:08.932872

---

This is a single-patch fix for the TTM pool debugfs output. The problem is straightforward: `ttm_pool_shrinker_count()` returns `SHRINK_EMPTY` (~0UL - 1) when there are no pages, and `ttm_pool_shrinker_scan()` returns `SHRINK_STOP` (~0UL) when nothing was freed. These are enormous unsigned long sentinel values that would be printed verbatim in the `page_pool_shrink` debugfs file, appearing as nonsensical huge numbers (e.g., 18446744073709551614/18446744073709551615) rather than meaningful zeros.

The fix is correct and minimal.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: ttm/pool: Use sentinels in debugfs
  2026-04-27 16:22 [PATCH] ttm/pool: Use sentinels in debugfs Mario Limonciello
  2026-04-28  4:24 ` Claude review: " Claude Code Review Bot
@ 2026-04-28  4:24 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:24 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The sentinel checks match exactly what the underlying functions can return:

- `ttm_pool_shrinker_count()` at line 1222 returns `SHRINK_EMPTY` when `num_pages` is 0 — the patch correctly checks `count == SHRINK_EMPTY`.
- `ttm_pool_shrinker_scan()` at line 1213 returns `SHRINK_STOP` when `num_freed` is 0 — the patch correctly checks `scanned == SHRINK_STOP`.

**Minor observations:**

1. **Scan is unconditional even when count shows empty.** The code calls `ttm_pool_shrinker_scan()` even when `count == SHRINK_EMPTY` (i.e., there are zero pages on this node). The scan will iterate and find nothing to free, returning `SHRINK_STOP`. This is harmless since this is a debugfs path (not performance-critical), and the pre-existing code had the same behavior. Not a blocking concern, but a `if (count != SHRINK_EMPTY)` guard could avoid the pointless scan call.

2. **The comment is reasonable.** The inline comment `/* Convert shrinker API sentinel values to 0 for debugfs output */` explains the *why* clearly enough.

3. **The variable split is good practice.** Separating `scanned = ttm_pool_shrinker_scan(...)` out of the `seq_printf` call improves readability and enables the sentinel checks without evaluating side effects in a printf argument list.

**No correctness issues found.** The patch is a clean, minimal fix for a real debugfs output bug.

Reviewed-by: looks appropriate.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-04-28  4:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-27 16:22 [PATCH] ttm/pool: Use sentinels in debugfs Mario Limonciello
2026-04-28  4:24 ` Claude review: " Claude Code Review Bot
2026-04-28  4:24 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox