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/ttm: Use ttm_backup_insert_folio() for zero-copy swapout Date: Sat, 16 May 2026 13:43:40 +1000 Message-ID: In-Reply-To: <20260512110339.6244-3-thomas.hellstrom@linux.intel.com> References: <20260512110339.6244-1-thomas.hellstrom@linux.intel.com> <20260512110339.6244-3-thomas.hellstrom@linux.intel.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 **drivers/gpu/drm/ttm/ttm_backup.c =E2=80=94 `ttm_backup_insert_folio()`:** The new function is a clean wrapper: ```c s64 ttm_backup_insert_folio(struct file *backup, struct folio *folio, unsigned int order, bool writeback, pgoff_t idx, gfp_t folio_gfp) { int ret; WARN_ON_ONCE(folio_get_private(folio)); ret =3D shmem_insert_folio(backup, folio, order, idx, writeback, folio_gfp= ); if (ret) return ret; return ttm_backup_shmem_idx_to_handle(idx); } ``` The `WARN_ON_ONCE(folio_get_private(folio))` is good =E2=80=94 it catches t= he case where TTM forgot to clear `page->private` (which stores the pool or= der) before insertion. Note that the old `ttm_backup_backup_page()` is completely removed and repl= aced. The old function is no longer called anywhere. Good =E2=80=94 no dead= code left behind. **drivers/gpu/drm/ttm/ttm_pool.c =E2=80=94 `ttm_pool_backup()` changes:** 1. **Loop structure change:** The loop changes from `for (i =3D 0; i < num_pages; ++i)` to `for (i =3D= 0; i < num_pages; i +=3D npages)` with `npages` computed from the page ord= er. This is the core of the optimization =E2=80=94 instead of splitting eve= ry compound page before backup, it attempts to insert the entire compound f= olio. 2. **New handle check in the initial shrink pass:** ```c if (unlikely(!page || ttm_backup_page_ptr_is_handle(page))) { num_pages =3D 1; continue; } ``` The old code only checked `!page`. The addition of `ttm_backup_page_ptr_= is_handle(page)` correctly handles re-entry after a partial backup =E2=80= =94 pages that were already backed up have their `tt->pages[i]` slot replac= ed with a handle pointer. 3. **Fallback-to-split on insertion failure:** ```c if (unlikely(handle < 0)) { if (order) { page->private =3D order; ttm_pool_split_for_swap(pool, page); npages =3D 0; continue; } ret =3D (int)handle; break; } ``` When high-order insertion fails, the code restores `page->private =3D or= der` (which was cleared to 0 before the insertion attempt) and calls `ttm_p= ool_split_for_swap()`. Setting `npages =3D 0` causes `i` not to advance, so= the loop retries the same index =E2=80=94 but now the page is order-0 afte= r splitting. This is a clever retry mechanism. **However**, there's a subtlety: `shmem_insert_folio()` may have called = `prep_compound_page()` to promote the non-compound high-order allocation to= compound, and then `undo_compound_page()` on failure. After undo, the page= s are back to non-compound state. Then `ttm_pool_split_for_swap()` calls `s= plit_page(p, order)`. `split_page()` handles non-compound high-order pages = (adjusting refcounts), so this is technically correct, but it means the pag= es have been through a promote=E2=86=92undo=E2=86=92split cycle where `page= [1]` and other tail pages' internal fields (lru, etc.) were modified by the= compound promotion and not fully restored by undo. Since the pages are iso= lated and split_page only cares about refcounts, this works in practice. It= would be worth a comment explaining this lifecycle. 4. **NR_GPU_ACTIVE accounting:** ```c mod_node_page_state(page_pgdat(page), NR_GPU_ACTIVE, -(1 << order)); folio_put(page_folio(page)); ``` The old code used `__free_pages_gpu_account(page, 0, false)` (which does= n't exist in the current drm-next tree, confirming these patches are agains= t a different base). The new code correctly decrements `NR_GPU_ACTIVE` by t= he full folio size and drops the caller's reference. Since `shmem_insert_fo= lio()` transferred ownership to shmem (which took its own reference via `sh= mem_add_to_page_cache()`), the `folio_put()` drops the last non-shmem refer= ence. 5. **Handle array population:** ```c for (j =3D 0; j < npages; j++) tt->pages[i + j] =3D ttm_backup_handle_to_page_ptr(handle + j); ``` This fills in handles for all sub-pages of the compound folio. The comme= nt in `ttm_backup_insert_folio()`'s docstring says "Handles for sub-pages o= f a compound folio follow sequentially: handle + j addresses sub-page j." T= his relies on the shmem page cache index being the handle base, with sequen= tial offsets for each page within the folio. This is consistent with `ttm_b= ackup_shmem_idx_to_handle()`. 6. **Fault injection guard:** ```c if (unlikely(i + npages > num_pages)) break; ``` Good defensive check =E2=80=94 if `CONFIG_FAULT_INJECTION` truncated `nu= m_pages` mid-compound-page, the code breaks out rather than trying to parti= ally insert a folio. **include/drm/ttm/ttm_backup.h:** Clean interface update, just renaming `ttm_backup_backup_page` to `ttm_back= up_insert_folio` with the new folio-based signature. The `alloc_gfp` parame= ter is correctly dropped since shmem pages are no longer allocated. **Summary of issues requiring attention:** - The missing `folio_clear_dirty_for_io()` / `folio_set_reclaim()` before `= shmem_writeout()` in patch 1 is the most significant concern and should be = addressed or explicitly justified. - `undo_compound_page()` scope and documentation could be tightened. - The silent EEXIST truncation in `shmem_insert_folio()` should at least em= it a debug/warning message. --- Generated by Claude Code Patch Reviewer