Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incorrect base-page count calculation for large folios desynchronizes migration loops and skips valid entries. - [High] Permanent device memory leak on migration error path. -- commit f9a15fc79a089e7970a0abb86569d6cf4a696964 Author: Junhua Shen <[email protected]> drm/pagemap: centralize migrate src base-page counting This commit factors out the base-page count calculation for migrate.src entries into a new drm_pagemap_src_pfn_nr_pages function. It applies this new helper to the accounting loops to consistently handle compound pages. > diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c > index 892b325fa99b8..a31cc4f0af68c 100644 > --- a/drivers/gpu/drm/drm_pagemap.c > +++ b/drivers/gpu/drm/drm_pagemap.c > @@ -554,6 +554,37 @@ static int drm_pagemap_migrate_range(struct > drm_pagemap_devmem *devmem, [ ... ] > +static unsigned long drm_pagemap_src_pfn_nr_pages(unsigned long src_pfn, > + struct page **src_page) > +{ > + struct page *page = NULL; > + unsigned long nr_pages = 1; > + > + if (src_pfn & MIGRATE_PFN_VALID) { > + page = migrate_pfn_to_page(src_pfn); > + if (page) > + nr_pages = NR_PAGES(folio_order(page_folio(page))); [Severity: High] This is a pre-existing issue, but does computing the stride using the backing folio's size skip independent PTE entries? If a VMA maps a transparent huge page (THP) via PTEs, migrate_vma_setup produces individual base-page entries. However, using folio_order(page_folio(page)) returns the full compound order (e.g., 512). The loops incrementing by this value would then skip the next 511 valid entries in the migrate.src array. [Severity: High] This is also a pre-existing issue related to the above, but does this create a desync between the migration steps? In the central loop in drivers/gpu/drm/drm_pagemap.c:drm_pagemap_migrate_to_devmem(): for (i = 0; i < npages;) { unsigned int order = 0; ... if (migrate.src[i] & MIGRATE_PFN_COMPOUND) { order = HPAGE_PMD_ORDER; ... } ... i += NR_PAGES(order); } Because order is 0 for PTE-mapped THPs here (MIGRATE_PFN_COMPOUND is false), this central loop correctly strides by 1. However, the accounting loops that use the new drm_pagemap_src_pfn_nr_pages() helper will stride by the full folio size. Will this result in device pages being allocated and processed here, but skipped in the accounting phases? [Severity: High] There is another pre-existing issue on the migration error path. Does truncating npages cause device pages to be permanently leaked? drivers/gpu/drm/drm_pagemap.c:drm_pagemap_migrate_to_devmem() { ... err = ops->populate_devmem_pfn(devmem_allocation, npages, migrate.dst); ... for (i = 0; i < npages;) { ... err = drm_pagemap_migrate_range(...); if (err) { npages = i + 1; goto err_finalize; } ... } ... err_finalize: if (err) { drm_pagemap_migration_unlock_put_pages(npages, migrate.dst); for (i = npages; i < npages_in_range(start, end); ++i) migrate.dst[i] = 0; } ... } When drm_pagemap_migrate_range() fails, npages is truncated. The cleanup code in err_finalize then only handles pages up to this truncated index. The loop clearing migrate.dst[i] = 0 wipes the only references to the remaining device pages originally allocated by ops->populate_devmem_pfn(), ensuring they can never be freed. Could we avoid this by unlocking only the processed pages while still freeing the remaining unprocessed device pages? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
