Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] ttm_pool_split_for_swap() ignores the return value of folio_split_driver_managed(), leading to severe buddy allocator corruption if the split fails. - [High] Kconfig hazard: Forcing TRANSPARENT_HUGEPAGE via select bypasses critical architectural dependencies. --
--- Patch [3]: [PATCH 3/3] drm/ttm: allocate pool pages as compound (__GFP_COMP) --- commit dae665417eeff981f7bae0e75da08ad47a4dd0a0 Author: Matthew Brost <[email protected]> drm/ttm: allocate pool pages as compound (__GFP_COMP) This commit changes the non-DMA path in ttm_pool_alloc_page() to allocate compound pages with __GFP_COMP instead of stashing the order in page->private. It updates the backup, restore, and free paths to read the order via folio_order() and split compound folios using folio_split_driver_managed(). > folio_split_driver_managed() lives in the THP split machinery > (mm/huge_memory.c), which only builds when CONFIG_TRANSPARENT_HUGEPAGE > is enabled. Drivers that drive the TTM shrinker and therefore reach the > split path must select TRANSPARENT_HUGEPAGE. [Severity: High] Is it safe to force select TRANSPARENT_HUGEPAGE in drivers? The Kconfig select directive forcefully enables the target symbol without evaluating its dependencies. TRANSPARENT_HUGEPAGE strictly depends on HAVE_ARCH_TRANSPARENT_HUGEPAGE and !PREEMPT_RT. Using select could force THP on architectures that lack support, breaking the build, or bypass the !PREEMPT_RT dependency and violate strict latency requirements. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 1bf37023fed66..364cc7ec7469c 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -493,15 +493,31 @@ static unsigned int ttm_pool_page_order(struct ttm_pool > *pool, struct page *p) > static void ttm_pool_split_for_swap(struct ttm_pool *pool, struct page *p) > { > unsigned int order = ttm_pool_page_order(pool, p); > > if (!order) > return; > > - split_page(p, order); > - nr = 1UL << order; > - while (nr--) > - (p++)->private = 0; > + if (ttm_pool_uses_dma_alloc(pool)) { > + pgoff_t nr; > + > + /* > + * DMA-alloc pages are not compound; split the plain > + * higher-order allocation and clear the per-page private > + * (which held the order for the non-compound case). > + */ > + split_page(p, order); > + nr = 1UL << order; > + while (nr--) > + (p++)->private = 0; > + return; > + } > + > + /* > + * The non-DMA path allocates compound folios (__GFP_COMP). Split the > + * driver-owned, off-LRU, unmapped folio into order-0 folios so each > + * page can be freed as soon as it has been backed up. > + */ > + folio_split_driver_managed(page_folio(p), 0); > } [Severity: Critical] Does this code handle split failures correctly? If CONFIG_TRANSPARENT_HUGEPAGE is disabled, the fallback stub for folio_split_driver_managed() unconditionally returns -EINVAL and the folio remains an intact compound page. Because the return value is ignored, callers like ttm_pool_backup() will incorrectly assume the split succeeded. Will this corrupt the buddy allocator in ttm_pool_backup()? It follows the split with this loop: ttm_pool_split_for_swap(pool, page); for (j = 0; j < nr_backed; j++) __free_pages_gpu_account(page + j, 0, false); Calling __free_pages(page + j, 0) on a tail page of an active compound folio redirects the refcount decrement to the head page, rapidly underflowing it. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
