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

["David Hildenbrand (Arm)" <david@kernel.org>]

> +
> +/**
> + * 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 ()

> +			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" ?

> +				| MIGRATE_PFN_MIGRATE;

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

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.

> +
> +		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.

-- 
Cheers,

David