From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/panthor: Add a GEM shrinker
Date: Tue, 31 Mar 2026 17:24:47 +1000 [thread overview]
Message-ID: <review-patch9-20260330094848.2169422-10-boris.brezillon@collabora.com> (raw)
In-Reply-To: <20260330094848.2169422-10-boris.brezillon@collabora.com>
Patch Review
This is the main event. Several points:
1. **`panthor_gem_free_object` without resv lock**: The comment explains that the last ref can be dropped from the shrinker path where resv can't be taken. The `bo_assert_locked_or_gone` helper checks `kref_read() == 0` to allow lockless access during destruction. This is correct because at `refcount==0` no other code path can access the object.
2. **`panthor_gem_try_evict_no_resv_wait` lock ordering**: The function comments explain the lock order inversion between `vm_resv -> bo_resv -> bo_gpuva` (normal path) vs `bo_resv -> bo_gpuva -> vm_resv` (reclaim path). Using `trylock` throughout is the right approach. The comment about unmapping CPU pages before taking `bo_gpuva` lock to avoid `i_mmap_rwsem` ordering issues is correct.
3. **`panthor_vm_evict_bo_mappings_locked` partial eviction**: When `panthor_vm_lock_region` fails mid-eviction, `drm_gpuvm_bo_evict(vm_bo, true)` has already been called. The comment explains this is safe because validation will repopulate all evicted VMAs. However:
```c
ret = panthor_vm_lock_region(vm, va->va.addr, va->va.range);
if (ret)
break;
panthor_vm_unmap_pages(vm, va->va.addr, va->va.range);
panthor_vm_unlock_region(vm);
```
If `panthor_vm_lock_region` fails, we break without calling `panthor_vm_unlock_region`. Looking at the kernel tree, `panthor_vm_lock_region` only sets `locked_region` on success, so not calling unlock is correct (no region was locked).
4. **`select_evicted_vma` / `remap_evicted_vma` race**: The lock is released between selection and remapping to allow PT allocation (which can't happen under `op_lock` since it's in a DMA-signalling path). The re-verification via both pointer equality AND the `evicted` bit is sound - if the VMA was freed and reallocated at the same address, the evicted bit would be false (new VMAs start non-evicted).
5. **`panthor_mmu_reclaim_priv_bos` VM list management**: The splice/re-insert logic looks correct. VMs with remaining reclaimable BOs are preserved in LRU order. The `kref_get_unless_zero` check handles VMs being freed concurrently.
6. **`panthor_gem_shrinker_count` raciness**: The comment acknowledges the race on `gpu_mapped_count` and compares to MSM. This is standard practice for shrinker count functions - the value is a hint. Fine.
7. **`panthor_gem_shrinker_scan` retry logic**:
```c
if (!freed && remaining > 0 &&
atomic_inc_return(&ptdev->reclaim.retry_count) < 2)
return 0;
```
Returning 0 (not `SHRINK_STOP`) when there are remaining objects but we couldn't free any tells the shrinker to try again. After 2 failed attempts, it falls through to `SHRINK_STOP`. This seems reasonable though the threshold of 2 is somewhat arbitrary.
8. **`can_block` check**:
```c
return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) ||
((sc->gfp_mask & __GFP_KSWAPD_RECLAIM) && current_is_kswapd());
```
This is correct - `__GFP_KSWAPD_RECLAIM` alone doesn't mean we can block (it just means kswapd has been woken), but if we're actually running *in* kswapd, we can.
9. **`panthor_vm_bo_free` change**: Previously called `panthor_gem_unpin(bo)`, now calls `panthor_gem_update_reclaim_state_locked`. The pin is now managed via `op_ctx->map.bo` (added ref + pin in prepare, dropped in cleanup). The state update in `panthor_vm_bo_free` ensures the BO moves to the correct LRU when the last vm_bo is freed.
10. **`panthor_kernel_bo_create` pinning for FW VM**: FW buffers are now explicitly pinned to prevent reclaim. This is correct - firmware memory must not be evicted.
11. **Patch 9 changes `panthor_gem_any_fault`** to handle `FAULT_FLAG_VMA_LOCK`:
```c
if (vmf->flags & FAULT_FLAG_VMA_LOCK)
vma_end_read(vmf->vma);
else
mmap_read_unlock(vmf->vma->vm_mm);
```
This is needed because with per-VMA locks (a relatively recent kernel feature), the fault handler may be called with only the VMA lock held rather than the mmap read lock. Good attention to detail.
12. **`blocking_page_setup` in patch 9** now calls `panthor_gem_update_reclaim_state_locked` after page setup succeeds. This ensures newly faulted-in pages are properly tracked. The `gpuva.lock` is taken inside the `resv` lock, consistent with the declared lock ordering.
13. **`debugfs shrink` interface**: The `shrink_set` function wraps the scan in `fs_reclaim_acquire/release` to simulate reclaim context for lockdep. The format string `"0x%08llx\n"` for the get is using hex for a page count - a decimal format might be more user-friendly for debugging, but this is minor.
Overall, patch 9 is complex but well-commented and the locking considerations are thoroughly documented. The series is in good shape for v6.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-03-31 7:24 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-30 9:48 [PATCH v6 0/9] drm/panthor: Add a GEM shrinker Boris Brezillon
2026-03-30 9:48 ` [PATCH v6 1/9] drm/gem: Consider GEM object reclaimable if shrinking fails Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 2/9] drm/panthor: Move panthor_gems_debugfs_init() to panthor_gem.c Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 3/9] drm/panthor: Group panthor_kernel_bo_xxx() helpers Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 4/9] drm/panthor: Don't call drm_gpuvm_bo_extobj_add() if the object is private Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 5/9] drm/panthor: Part ways with drm_gem_shmem_object Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 6/9] drm/panthor: Lazily allocate pages on mmap() Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 7/9] drm/panthor: Split panthor_vm_prepare_map_op_ctx() to prepare for reclaim Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 8/9] drm/panthor: Track the number of mmap on a BO Boris Brezillon
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
2026-03-30 9:48 ` [PATCH v6 9/9] drm/panthor: Add a GEM shrinker Boris Brezillon
2026-03-31 7:24 ` Claude Code Review Bot [this message]
2026-03-30 10:39 ` [PATCH v6 0/9] " Steven Price
2026-03-31 7:24 ` Claude review: " Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-09 15:11 [PATCH v5 0/9] " Boris Brezillon
2026-03-09 15:11 ` [PATCH v5 9/9] " Boris Brezillon
2026-03-10 2:15 ` Claude review: " Claude Code Review Bot
2026-03-10 2:15 ` Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch9-20260330094848.2169422-10-boris.brezillon@collabora.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox