Re: [RFC PATCH] mm/hmm, mm/migrate_device: Allow p2p access and p2p migration

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 01:13:22PM +0200, Thomas Hellström wrote:
> Introduce a way for hmm_range_fault() and migrate_vma_setup() to identify
> foreign devices with fast interconnect and thereby allow
> both direct access over the interconnect and p2p migration.
> 
> The need for a callback arises because without it, the p2p ability would
> need to be static and determined at dev_pagemap creation time. With
> a callback it can be determined dynamically, and in the migrate case
> the callback could separate out local device pages.


> +static bool hmm_allow_devmem(struct hmm_range *range, struct page *page)
> +{
> +	if (likely(page->pgmap->owner == range->dev_private_owner))
> +		return true;
> +	if (likely(!range->p2p))
> +		return false;
> +	return range->p2p->ops->p2p_allow(range->p2p, page);
> +}
> +
>  static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  			      unsigned long end, pmd_t *pmdp, pte_t *ptep,
>  			      unsigned long *hmm_pfn)
> @@ -248,8 +258,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  		 * just report the PFN.
>  		 */
>  		if (is_device_private_entry(entry) &&
> -		    pfn_swap_entry_to_page(entry)->pgmap->owner ==
> -		    range->dev_private_owner) {
> +		    hmm_allow_devmem(range, pfn_swap_entry_to_page(entry))) {
>  			cpu_flags = HMM_PFN_VALID;
>  			if (is_writable_device_private_entry(entry))
>  				cpu_flags |= HMM_PFN_WRITE;

This is really misnamed and took me a while to get it.

It has nothing to do with kernel P2P, you are just allowing more
selective filtering of dev_private_owner. You should focus on that in
the naming, not p2p. ie allow_dev_private()

P2P is stuff that is dealing with MEMORY_DEVICE_PCI_P2PDMA.

This is just allowing more instances of the same driver to co-ordinate
their device private memory handle, for whatever purpose.

Otherwise I don't see a particular problem, though we have talked
about widening the matching for device_private more broadly using some
kind of grouping tag or something like that instead of a callback. You
may consider that as an alternative

I would also probably try to have less indirection, you can embedd the
hmm_range struct inside a caller private data struct and use that
instead if inventing a whole new struct and pointer.

Jason