Re: [PATCH v10 2/5] mm: Add helper to convert HMM pfn to migrate pfn

["Mika Penttilä" <mpenttil@redhat.com>]

On 5/12/26 14:43, David Hildenbrand (Arm) wrote:

>> +
>> +/**
>> + * migrate_hmm_range_setup() - prepare to migrate a range of memory
>> + * @range: contains pointer to struct migrate_vma to be set up.
>> + *
>> + * When collecting has been done with hmm_range_fault(), this
>> + * should be called next, and completes range->migrate by
>> + * populating migrate->src[] and migrate->dst[]
>> + * using range->hmm_pfns[].
>> + * Also, migrate->cpages and migrate->npages get initialized.
>> + * After migrate_hmm_range_setup(), range->migrate is good
>> + * for the rest of the migrate_vma_* flow.
>> + */
>> +void migrate_hmm_range_setup(struct hmm_range *range)
>> +{
>> +	struct migrate_vma *migrate = range->migrate;
>> +
>> +	if (!migrate)
>> +		return;
>> +
>> +	migrate->npages = (migrate->end - migrate->start) >> PAGE_SHIFT;
>> +	migrate->cpages = 0;
>> +
>> +	for (unsigned long i = 0; i < migrate->npages; i++) {
>> +		unsigned long pfn = range->hmm_pfns[i];
>> +
>> +		/*
>> +		 * We are only interested in entries to be
>> +		 * migrated.
>> +		 */
>> +		if (!(pfn & HMM_PFN_MIGRATE)) {
>> +			migrate->src[i] = 0;
>> +			migrate->dst[i] = 0;
>> +			continue;
>> +		}
>> +
>> +		migrate->cpages++;
>> +
>> +		/* HMM_PFN_MIGRATE without HMM_PFN_VALID denotes the special zero page */
>> +		if (pfn & (HMM_PFN_VALID))
> No need for ()

ack

>
>> +			migrate->src[i] = migrate_pfn(page_to_pfn(hmm_pfn_to_page(pfn)))
> Is there no easier way to avoid going through a page?
>
> I guess you really just want "migrate_pfn(pfn & ~HMM_PFN_FLAGS)".
>
> Maybe add a hmm_pfn_to_pfn() helper that does the "pfn & ~HMM_PFN_FLAGS" ?

Basically yes. I noticed this was helpful for instance with the "separate address space for device_private"
patchset not to depend on the coding.

>
>> +				| MIGRATE_PFN_MIGRATE;
> We (MM folks) usually put the | onto the previous line to then properly indent
> the second line

Okay I will change.

>
> migrate->src[i] = migrate_pfn(page_to_pfn(hmm_pfn_to_page(pfn))) |
> 		  MIGRATE_PFN_MIGRATE;
>
>> +		else
>> +			migrate->src[i] = MIGRATE_PFN_MIGRATE;
> It might be cleaner to just set migrate->src[i] = 0; here and to unconditionally
>
> 	migrate->src[i] |= MIGRATE_PFN_MIGRATE;
>
> So you don't have the MIGRATE_PFN_MIGRATE on two paths.

Good point.

>
>> +
>> +		migrate->src[i] |= (pfn & HMM_PFN_WRITE) ? MIGRATE_PFN_WRITE : 0;
>> +		migrate->src[i] |= (pfn & HMM_PFN_COMPOUND) ? MIGRATE_PFN_COMPOUND : 0;
>> +		migrate->dst[i] = 0;
>> +	}
>> +
>> +	if (migrate->cpages)
>> +		migrate_vma_unmap(migrate);
> Can you remind me why we do this here, in the setup() phase? The function doc
> does not really describe that.

It is to be in par what migrate_vma_setup() does today after the "collect" phase.

Thanks,
Mika

>