From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panthor: Implement evicted status for GEM objects Date: Thu, 23 Apr 2026 09:29:12 +1000 Message-ID: In-Reply-To: <20260420-panthor-bo-reclaim-observability-v1-2-a4d1a36ee84f@collabora.com> References: <20260420-panthor-bo-reclaim-observability-v1-0-a4d1a36ee84f@collabora.com> <20260420-panthor-bo-reclaim-observability-v1-2-a4d1a36ee84f@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Summary:** Adds an `atomic_t reclaimed_count` field to `panthor_gem_objec= t`, increments it in the evict path, and uses it to report `DRM_GEM_OBJECT_= EVICTED` via the `.status` callback and the debugfs gems interface. **Review:** **Issue 1 (design): The wrap-around avoidance is over-engineered for a bool= ean need.** ```c /* Don't ever wrap around as far as 0, jump from INT_MIN to 1 */ if (!atomic_inc_unless_negative(&bo->reclaimed_count)) atomic_set(&bo->reclaimed_count, 1); ``` The only consumer of `reclaimed_count` checks `!=3D 0`: ```c else if (atomic_read(&bo->reclaimed_count)) res |=3D DRM_GEM_OBJECT_EVICTED; ``` If the sole purpose is "has this BO ever been evicted", a simple `bool` or = even a flag bit would suffice. The commit message justifies the counter by = saying "It's possible to distinguish evicted non-resident pages from newly = allocated non-resident pages by checking whether reclaimed_count is !=3D 0"= =E2=80=94 but that's the same as a boolean. If there's a genuine use case for knowing *how many times* a BO was evicted= (e.g., for future debugfs stats), the counter makes sense. But in that cas= e, the wrap-around logic is wrong =E2=80=94 after wrap-around, the count is= no longer meaningful. And `atomic_inc_unless_negative` is not a cheap oper= ation (it's a cmpxchg loop), unlike a simple `atomic_inc` or `atomic_set`. Suggestion: Either use a simpler `bool evicted` (or a flag bit) if you only= need the boolean, or use a plain `atomic_inc` if you want a counter (wrap-= around after 2^31 evictions is not a real-world concern, and the count bein= g "wrong" after wrap is no worse than clamping at 1). **Issue 2 (atomics concern): `atomic_set` after failed `atomic_inc_unless_n= egative` is not atomic with respect to concurrent reads.** ```c if (!atomic_inc_unless_negative(&bo->reclaimed_count)) atomic_set(&bo->reclaimed_count, 1); ``` If two threads hit this simultaneously when `reclaimed_count` is `INT_MIN`,= both could see `atomic_inc_unless_negative` fail (since `INT_MIN` is negat= ive), and both call `atomic_set(&bo->reclaimed_count, 1)`. That's fine =E2= =80=94 the result is still 1. But there's a window where a concurrent `atom= ic_read` from `panthor_gem_status` could observe 0 if the set hasn't comple= ted. In practice, this is harmless since: - Eviction holds a lock (`panthor_gem_evict_locked`) - The status read is racy by design (documented in `drm_gem_object_funcs.st= atus`) But it would be cleaner to just use `atomic_inc` and accept that the counte= r can wrap, or use a simple flag. **Issue 3 (logic): The `else if` chain in `panthor_gem_status` means import= ed objects can never be reported as evicted.** ```c if (drm_gem_is_imported(&bo->base) || bo->backing.pages) res |=3D DRM_GEM_OBJECT_RESIDENT; else if (atomic_read(&bo->reclaimed_count)) res |=3D DRM_GEM_OBJECT_EVICTED; ``` Since imported objects always get `RESIDENT`, they can never get `EVICTED`.= This is probably correct (imported objects shouldn't be evicted by the pan= thor shrinker), but worth confirming =E2=80=94 does the panthor shrinker sk= ip imported BOs? If an imported BO were somehow evicted, the status would i= ncorrectly show it as `RESIDENT`. **Issue 4 (debugfs): Same `else if` pattern with different condition for de= bugfs.** ```c if (drm_gem_is_imported(&bo->base)) gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED; else if (!resident_size && atomic_read(&bo->reclaimed_count)) gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED; if (bo->base.dma_buf) gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED; ``` Note the `else if` means an imported BO will never show as evicted in debug= fs either. An exported BO *can* show as evicted (since the exported check u= ses a separate `if`), which is inconsistent with the imported BO treatment.= If an exported BO is evicted, it will show both "exported" and "evicted" f= lags in debugfs, which seems correct. But an imported+evicted BO (if possib= le) would only show "imported". This is a minor consistency concern. **Issue 5 (missing initialization): `reclaimed_count` is not explicitly ini= tialized.** The new `atomic_t reclaimed_count` field in `panthor_gem_object` is not exp= licitly initialized to 0 in any allocation path. This relies on the GEM obj= ect being allocated with `kzalloc` (via shmem helpers), which zero-initiali= zes the entire struct. This works correctly for `atomic_t` since `ATOMIC_IN= IT(0)` is just `{ 0 }`, but it's worth noting that this is an implicit depe= ndency on zero-initialization. **Issue 6 (nit): Comment style.** ```c /* Don't ever wrap around as far as 0, jump from INT_MIN to 1 */ ``` This is a reasonable comment given the non-obvious wrap-around handling, th= ough it could be simplified if the approach itself is simplified. **Positive aspects:** - The debugfs flag naming table is correctly extended - The `else if` logic in `panthor_gem_status` correctly makes `EVICTED` mut= ually exclusive with `RESIDENT` - The commit message is well-written and clearly explains the rationale --- **Summary:** The series is architecturally sound and fills a real observabi= lity gap. The main feedback is: (1) consider whether `obj->size` is more ap= propriate than `add_size` for the evicted counter in patch 1, (2) simplify = the `reclaimed_count` to either a plain boolean flag or a simple `atomic_in= c` counter in patch 2, and (3) add documentation about `EVICTED`/`RESIDENT`= mutual exclusivity to the enum comments. --- Generated by Claude Code Patch Reviewer