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/fdinfo: Add "evicted" memory accounting Date: Thu, 23 Apr 2026 09:29:11 +1000 Message-ID: In-Reply-To: <20260420-panthor-bo-reclaim-observability-v1-1-a4d1a36ee84f@collabora.com> References: <20260420-panthor-bo-reclaim-observability-v1-0-a4d1a36ee84f@collabora.com> <20260420-panthor-bo-reclaim-observability-v1-1-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 `DRM_GEM_OBJECT_EVICTED` (BIT(3)) to `enum drm_gem_object= _status`, adds `u64 evicted` to `struct drm_memory_stats`, and wires both i= nto the fdinfo printing and accumulation paths. **Review:** The changes correctly follow the established pattern used by `RESIDENT`, `P= URGEABLE`, and `ACTIVE`. The new enum value is properly documented with a k= doc comment. **Issue 1 (minor): The "evicted" accumulation in `drm_show_memory_stats` us= es `add_size` unconditionally.** ```c if (s & DRM_GEM_OBJECT_EVICTED) status.evicted +=3D add_size; ``` This uses `add_size`, which is the RSS size (from `obj->funcs->rss()` if av= ailable, otherwise `obj->size`). For an evicted object, `rss()` would typic= ally return 0 since the pages are gone. In the panthor driver (patch 2), ev= icted objects won't have `DRM_GEM_OBJECT_RESIDENT` set, so they won't have = backing pages =E2=80=94 meaning `rss()` would return 0 (or not be meaningfu= l). For evicted objects, the interesting number is the *total* size of the obje= ct (how much was evicted to swap), not the RSS. Consider using `obj->size` = instead of `add_size`, or document why `add_size` is correct here. **Issue 2 (minor/doc): The `@DRM_GEM_OBJECT_EVICTED` doc says "object is ev= icted to swap".** ```c * @DRM_GEM_OBJECT_EVICTED: object is evicted to swap ``` The commit message mentions this is about pages being swapped out, but "evi= cted" in the GPU driver world often means "evicted from VRAM to system memo= ry" (e.g., in TTM-based drivers). The doc comment and the enum value name c= ould potentially be confused with VRAM eviction. Consider whether the descr= iption should be more specific, e.g. "object's backing pages have been recl= aimed by the shrinker." This is a naming/documentation concern rather than = a correctness issue. **Issue 3 (question): Interaction with existing `RESIDENT`/`PURGEABLE` filt= ering logic.** In `drm_show_memory_stats`, when an object is not `RESIDENT`, the code clea= rs `PURGEABLE`: ```c if (s & DRM_GEM_OBJECT_RESIDENT) { status.resident +=3D add_size; } else { s &=3D ~DRM_GEM_OBJECT_PURGEABLE; } ``` The new `EVICTED` status is accumulated *after* this block (based on the pa= tch context), which is correct since evicted objects should be non-resident= by definition. But there's no explicit mutual exclusivity check =E2=80=94 = a driver could theoretically return both `RESIDENT` and `EVICTED`, which wo= uld be nonsensical. This is not a bug (drivers shouldn't do this), but a co= mment noting that `EVICTED` is expected to be mutually exclusive with `RESI= DENT` would be helpful in the enum documentation. **Issue 4 (nit): Missing documentation update.** The `drm_gem_object_status` enum doc block has a long comment about the int= eraction between `PURGEABLE`, `RESIDENT`, and `ACTIVE`. It would be good to= add a similar note about `EVICTED` =E2=80=94 specifically that it should o= nly be set for non-resident objects whose pages were previously allocated a= nd then reclaimed, and that it is mutually exclusive with `RESIDENT`. **Positive:** The `drm_memory_stats_is_zero` check is correctly updated, an= d the printing order (after purgeable) is reasonable. --- --- Generated by Claude Code Patch Reviewer