Re: [v1 resend 04/12] mm/migrate_device: THP migration of zone device pages

[Matthew Brost]

On Fri, Jul 18, 2025 at 05:04:39PM +1000, Balbir Singh wrote:
> On 7/18/25 16:59, Matthew Brost wrote:
> > On Fri, Jul 04, 2025 at 09:35:03AM +1000, Balbir Singh wrote:
> >> +	if (thp_migration_supported() &&
> >> +		(migrate->flags & MIGRATE_VMA_SELECT_COMPOUND) &&
> >> +		(IS_ALIGNED(start, HPAGE_PMD_SIZE) &&
> >> +		 IS_ALIGNED(end, HPAGE_PMD_SIZE))) {
> >> +		migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE |
> >> +						MIGRATE_PFN_COMPOUND;
> >> +		migrate->dst[migrate->npages] = 0;
> >> +		migrate->npages++;
> >> +		migrate->cpages++;
> > 
> > It's a bit unclear what cpages and npages actually represent when
> > collecting a THP. In my opinion, they should reflect the total number of
> > minimum sized pages collected—i.e., we should increment by the shifted
> > order (512) here. I'm fairly certain the logic in migrate_device.c would
> > break if a 4MB range was requested and a THP was found first, followed by a
> > non-THP.
> > 
> 
> cpages and npages represent entries in the array and when or'ed with MIGRATE_PFN_COMPOUND
> represent the right number of entries populated. If you have a test that shows
> the breakage, I'd be keen to see it. We do populate other entries in 4k size(s) when
> collecting to allow for a split of the folio.
> 

I don’t have a test case, but let me quickly point out a logic bug.

Look at migrate_device_unmap. The variable i is incremented by
folio_nr_pages, which seems correct. However, in the earlier code, we
populate migrate->src using migrate->npages as the index, then increment
it by 1. So, if two THPs are found back to back, they’ll occupy entries
0 and 1, while migrate_device_unmap will access entries 0 and 512.

Given that we have no idea what mix of THP vs non-THP we’ll encounter,
the only sane approach is to populate the input array at minimum
page-entry alignment. Similarly, npages and cpages should reflect the
number of minimum-sized pages found, with the caller (and
migrate_device) understanding that src and dst will be sparsely
populated based on each entry’s folio order.

Matt

> Thanks for the review,
> Balbir Singh