From: Yonatan Maman <Ymaman@Nvidia.com>

hmm_range_fault() natively triggers a page fault on device private
pages, migrating them to RAM. In some cases, such as with RDMA devices,
the migration overhead between the device (e.g., GPU) and the CPU, and
vice-versa, significantly damages performance. Thus, enabling Peer-to-
Peer (P2P) DMA access for device private page might be crucial for
minimizing data transfer overhead.

This change introduces an API to support P2P connections for device
private pages by implementing the following:

 - Leveraging the struct pagemap_ops for P2P Page Callbacks. This
   callback involves mapping the page to MMIO and returning the
   corresponding PCI_P2P page.

 - Utilizing hmm_range_fault for Initializing P2P Connections. The API
   also adds the HMM_PFN_REQ_TRY_P2P flag option for the
   hmm_range_fault caller to initialize P2P. If set, hmm_range_fault
   attempts initializing the P2P connection first, if the owner device
   supports P2P, using p2p_page. In case of failure or lack of support,
   hmm_range_fault will continue with the regular flow of migrating the
   page to RAM.

This change does not affect previous use-cases of hmm_range_fault,
because both the caller and the page owner must explicitly request and
support it to initialize P2P connection.

Signed-off-by: Yonatan Maman <Ymaman@Nvidia.com>
Reviewed-by: Gal Shalom <GalShalom@Nvidia.com>
---
 include/linux/hmm.h      |  2 ++
 include/linux/memremap.h |  7 +++++++
 mm/hmm.c                 | 28 ++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 126a36571667..7154f5ed73a1 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -41,6 +41,8 @@ enum hmm_pfn_flags {
 	/* Input flags */
 	HMM_PFN_REQ_FAULT = HMM_PFN_VALID,
 	HMM_PFN_REQ_WRITE = HMM_PFN_WRITE,
+	/* allow returning PCI P2PDMA pages */
+	HMM_PFN_REQ_ALLOW_P2P = 1,
 
 	HMM_PFN_FLAGS = 0xFFUL << HMM_PFN_ORDER_SHIFT,
 };
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 3f7143ade32c..0ecfd3d191fa 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -89,6 +89,13 @@ struct dev_pagemap_ops {
 	 */
 	vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
 
+	/*
+	 * Used for private (un-addressable) device memory only. Return a
+	 * corresponding struct page, that can be mapped to device
+	 * (e.g using dma_map_page)
+	 */
+	struct page *(*get_dma_page_for_device)(struct page *private_page);
+
 	/*
 	 * Handle the memory failure happens on a range of pfns.  Notify the
 	 * processes who are using these pfns, and try to recover the data on
diff --git a/mm/hmm.c b/mm/hmm.c
index 7e0229ae4a5a..987dd143d697 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -230,6 +230,8 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
 	unsigned long cpu_flags;
 	pte_t pte = ptep_get(ptep);
 	uint64_t pfn_req_flags = *hmm_pfn;
+	struct page *(*get_dma_page_handler)(struct page *private_page);
+	struct page *dma_page;
 
 	if (pte_none_mostly(pte)) {
 		required_fault =
@@ -257,6 +259,32 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
 			return 0;
 		}
 
+		/*
+		 * P2P for supported pages, and according to caller request
+		 * translate the private page to the match P2P page if it fails
+		 * continue with the regular flow
+		 */
+		if (is_device_private_entry(entry)) {
+			get_dma_page_handler =
+				pfn_swap_entry_to_page(entry)
+					->pgmap->ops->get_dma_page_for_device;
+			if ((hmm_vma_walk->range->default_flags &
+			    HMM_PFN_REQ_ALLOW_P2P) &&
+			    get_dma_page_handler) {
+				dma_page = get_dma_page_handler(
+					pfn_swap_entry_to_page(entry));
+				if (!IS_ERR(dma_page)) {
+					cpu_flags = HMM_PFN_VALID;
+					if (is_writable_device_private_entry(
+						    entry))
+						cpu_flags |= HMM_PFN_WRITE;
+					*hmm_pfn = page_to_pfn(dma_page) |
+						   cpu_flags;
+					return 0;
+				}
+			}
+		}
+
 		required_fault =
 			hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0);
 		if (!required_fault) {
-- 
2.34.1

Yonatan Maman <ymaman@nvidia.com> writes:

> From: Yonatan Maman <Ymaman@Nvidia.com>
>
> hmm_range_fault() natively triggers a page fault on device private
> pages, migrating them to RAM. In some cases, such as with RDMA devices,
> the migration overhead between the device (e.g., GPU) and the CPU, and
> vice-versa, significantly damages performance. Thus, enabling Peer-to-
> Peer (P2P) DMA access for device private page might be crucial for
> minimizing data transfer overhead.
>
> This change introduces an API to support P2P connections for device
> private pages by implementing the following:
>
>  - Leveraging the struct pagemap_ops for P2P Page Callbacks. This
>    callback involves mapping the page to MMIO and returning the
>    corresponding PCI_P2P page.
>
>  - Utilizing hmm_range_fault for Initializing P2P Connections. The API
>    also adds the HMM_PFN_REQ_TRY_P2P flag option for the
>    hmm_range_fault caller to initialize P2P. If set, hmm_range_fault
>    attempts initializing the P2P connection first, if the owner device
>    supports P2P, using p2p_page. In case of failure or lack of support,
>    hmm_range_fault will continue with the regular flow of migrating the
>    page to RAM.
>
> This change does not affect previous use-cases of hmm_range_fault,
> because both the caller and the page owner must explicitly request and
> support it to initialize P2P connection.
>
> Signed-off-by: Yonatan Maman <Ymaman@Nvidia.com>
> Reviewed-by: Gal Shalom <GalShalom@Nvidia.com>
> ---
>  include/linux/hmm.h      |  2 ++
>  include/linux/memremap.h |  7 +++++++
>  mm/hmm.c                 | 28 ++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 126a36571667..7154f5ed73a1 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -41,6 +41,8 @@ enum hmm_pfn_flags {
>  	/* Input flags */
>  	HMM_PFN_REQ_FAULT = HMM_PFN_VALID,
>  	HMM_PFN_REQ_WRITE = HMM_PFN_WRITE,
> +	/* allow returning PCI P2PDMA pages */
> +	HMM_PFN_REQ_ALLOW_P2P = 1,
>  
>  	HMM_PFN_FLAGS = 0xFFUL << HMM_PFN_ORDER_SHIFT,
>  };
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 3f7143ade32c..0ecfd3d191fa 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -89,6 +89,13 @@ struct dev_pagemap_ops {
>  	 */
>  	vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
>  
> +	/*
> +	 * Used for private (un-addressable) device memory only. Return a
> +	 * corresponding struct page, that can be mapped to device
> +	 * (e.g using dma_map_page)
> +	 */
> +	struct page *(*get_dma_page_for_device)(struct page *private_page);

It would be nice to add some documentation about this feature to
Documentation/mm/hmm.rst. In particular some notes on the page
lifetime/refcounting rules.

On that note how is the refcounting of the returned p2pdma page expected
to work? We don't want the driver calling hmm_range_fault() to be able
to pin the page with eg. get_page(), so the returned p2pdma page should
have a zero refcount to enforce that.

> +
>  	/*
>  	 * Handle the memory failure happens on a range of pfns.  Notify the
>  	 * processes who are using these pfns, and try to recover the data on
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 7e0229ae4a5a..987dd143d697 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -230,6 +230,8 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  	unsigned long cpu_flags;
>  	pte_t pte = ptep_get(ptep);
>  	uint64_t pfn_req_flags = *hmm_pfn;
> +	struct page *(*get_dma_page_handler)(struct page *private_page);
> +	struct page *dma_page;
>  
>  	if (pte_none_mostly(pte)) {
>  		required_fault =
> @@ -257,6 +259,32 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  			return 0;
>  		}
>  
> +		/*
> +		 * P2P for supported pages, and according to caller request
> +		 * translate the private page to the match P2P page if it fails
> +		 * continue with the regular flow
> +		 */
> +		if (is_device_private_entry(entry)) {
> +			get_dma_page_handler =
> +				pfn_swap_entry_to_page(entry)
> +					->pgmap->ops->get_dma_page_for_device;
> +			if ((hmm_vma_walk->range->default_flags &
> +			    HMM_PFN_REQ_ALLOW_P2P) &&
> +			    get_dma_page_handler) {
> +				dma_page = get_dma_page_handler(
> +					pfn_swap_entry_to_page(entry));
> +				if (!IS_ERR(dma_page)) {
> +					cpu_flags = HMM_PFN_VALID;
> +					if (is_writable_device_private_entry(
> +						    entry))
> +						cpu_flags |= HMM_PFN_WRITE;
> +					*hmm_pfn = page_to_pfn(dma_page) |
> +						   cpu_flags;
> +					return 0;
> +				}
> +			}
> +		}
> +
>  		required_fault =
>  			hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0);
>  		if (!required_fault) {

On Wed, Oct 16, 2024 at 04:10:53PM +1100, Alistair Popple wrote:
> On that note how is the refcounting of the returned p2pdma page expected
> to work? We don't want the driver calling hmm_range_fault() to be able
> to pin the page with eg. get_page(), so the returned p2pdma page should
> have a zero refcount to enforce that.

I think that is just the rule for hmm stuff in general, don't touch
the refcount.

We don't need to enforce, it we don't know what else the driver will
want to use that P2P page for after all. It might stick it in a VMA
for some unrelated reason.

Jason

Jason Gunthorpe <jgg@ziepe.ca> writes:

> On Wed, Oct 16, 2024 at 04:10:53PM +1100, Alistair Popple wrote:
>> On that note how is the refcounting of the returned p2pdma page expected
>> to work? We don't want the driver calling hmm_range_fault() to be able
>> to pin the page with eg. get_page(), so the returned p2pdma page should
>> have a zero refcount to enforce that.
>
> I think that is just the rule for hmm stuff in general, don't touch
> the refcount.

Actually I think the rule should be don't look at the page at
all. hmm_range_fault() is about mirroring PTEs, no assumption should
even be made about the existence or otherwise of a struct page.

> We don't need to enforce, it we don't know what else the driver will
> want to use that P2P page for after all. It might stick it in a VMA
> for some unrelated reason.

And wouldn't that touch the refcount and therefore be wrong? How is the
driver which handed out the P2PDMA page meant to ensure it can
free/evict the underlying device private memory in this case? Normally
this would happen by migration, which would trigger an MMU notifier on
the virtual address. But if the P2PDMA page has been mapped into another
VMA how does it drop the reference on the P2PDMA page?

 - Alistair

> Jason

On Thu, Oct 17, 2024 at 12:58:48PM +1100, Alistair Popple wrote:

> Actually I think the rule should be don't look at the page at
> all. hmm_range_fault() is about mirroring PTEs, no assumption should
> even be made about the existence or otherwise of a struct page.

We are not there yet..
 
> > We don't need to enforce, it we don't know what else the driver will
> > want to use that P2P page for after all. It might stick it in a VMA
> > for some unrelated reason.
> 
> And wouldn't that touch the refcount and therefore be wrong?

I mean the originating driver would do that

Jason

The subject does not make sense.  All P2P is on ZONE_DEVICE pages.
It seems like this is about device private memory?

On Tue, Oct 15, 2024 at 06:23:45PM +0300, Yonatan Maman wrote:
> From: Yonatan Maman <Ymaman@Nvidia.com>
> 
> hmm_range_fault() natively triggers a page fault on device private
> pages, migrating them to RAM.

That "natively" above doesn't make sense to me.

> In some cases, such as with RDMA devices,
> the migration overhead between the device (e.g., GPU) and the CPU, and
> vice-versa, significantly damages performance. Thus, enabling Peer-to-

s/damages/degrades/

> Peer (P2P) DMA access for device private page might be crucial for
> minimizing data transfer overhead.
> 
> This change introduces an API to support P2P connections for device
> private pages by implementing the following:

"This change.. " or "This patch.." is pointless, just explain what you
are doing.

> 
>  - Leveraging the struct pagemap_ops for P2P Page Callbacks. This
>    callback involves mapping the page to MMIO and returning the
>    corresponding PCI_P2P page.

While P2P uses the same underlying PCIe TLPs as MMIO, it is not
MMIO by definition, as memory mapped I/O is by definition about
the CPU memory mappping so that load and store instructions cause
the I/O.  It also uses very different concepts in Linux.

>  - Utilizing hmm_range_fault for Initializing P2P Connections. The API

There is no concept of a "connection" in PCIe dta transfers.

>    also adds the HMM_PFN_REQ_TRY_P2P flag option for the
>    hmm_range_fault caller to initialize P2P. If set, hmm_range_fault
>    attempts initializing the P2P connection first, if the owner device
>    supports P2P, using p2p_page. In case of failure or lack of support,
>    hmm_range_fault will continue with the regular flow of migrating the
>    page to RAM.

What is the need for the flag?  As far as I can tell from reading
the series, the P2P mapping is entirely transparent to the callers
of hmm_range_fault.

> +	/*
> +	 * Used for private (un-addressable) device memory only. Return a
> +	 * corresponding struct page, that can be mapped to device
> +	 * (e.g using dma_map_page)
> +	 */
> +	struct page *(*get_dma_page_for_device)(struct page *private_page);

We are talking about P2P memory here.  How do you manage to get a page
that dma_map_page can be used on?  All P2P memory needs to use the P2P
aware dma_map_sg as the pages for P2P memory are just fake zone device
pages.


> +		 * P2P for supported pages, and according to caller request
> +		 * translate the private page to the match P2P page if it fails
> +		 * continue with the regular flow
> +		 */
> +		if (is_device_private_entry(entry)) {
> +			get_dma_page_handler =
> +				pfn_swap_entry_to_page(entry)
> +					->pgmap->ops->get_dma_page_for_device;
> +			if ((hmm_vma_walk->range->default_flags &
> +			    HMM_PFN_REQ_ALLOW_P2P) &&
> +			    get_dma_page_handler) {
> +				dma_page = get_dma_page_handler(
> +					pfn_swap_entry_to_page(entry));

This is really messy.  You probably really want to share a branch
with the private page handling for the owner so that you only need
a single is_device_private_entry and can use a local variable for
to shortcut finding the page.  Probably best done with a little helper:

Then  this becomes:

static bool hmm_handle_device_private(struct hmm_range *range,
		swp_entry_t entry, unsigned long *hmm_pfn)
{
	struct page *page = pfn_swap_entry_to_page(entry);
	struct dev_pagemap *pgmap = page->pgmap;

	if (pgmap->owner == range->dev_private_owner) {
		*hmm_pfn = swp_offset_pfn(entry);
		goto found;
	}

	if (pgmap->ops->get_dma_page_for_device) {
		*hmm_pfn =
			page_to_pfn(pgmap->ops->get_dma_page_for_device(page));
		goto found;
	}

	return false;

found:
	*hmm_pfn |= HMM_PFN_VALID
	if (is_writable_device_private_entry(entry))
		*hmm_pfn |= HMM_PFN_WRITE;
	return true;
}

which also makes it clear that returning a page from the method is
not that great, a PFN might work a lot better, e.g.

	unsigned long (*device_private_dma_pfn)(struct page *page);

On Tue, Oct 15, 2024 at 09:49:30PM -0700, Christoph Hellwig wrote:

> > +	/*
> > +	 * Used for private (un-addressable) device memory only. Return a
> > +	 * corresponding struct page, that can be mapped to device
> > +	 * (e.g using dma_map_page)
> > +	 */
> > +	struct page *(*get_dma_page_for_device)(struct page *private_page);
> 
> We are talking about P2P memory here.  How do you manage to get a page
> that dma_map_page can be used on?  All P2P memory needs to use the P2P
> aware dma_map_sg as the pages for P2P memory are just fake zone device
> pages.

FWIW, I've been expecting this series to be rebased on top of Leon's
new DMA API series so it doesn't have this issue.. I think this series
is at RFC quality, but shows more of the next steps.

I'm guessing they got their testing done so far on a system without an
iommu translation?

> which also makes it clear that returning a page from the method is
> not that great, a PFN might work a lot better, e.g.
> 
> 	unsigned long (*device_private_dma_pfn)(struct page *page);

Ideally I think we should not have the struct page * at all through
these APIs if we can avoid it..

Jason

On Wed, Oct 16, 2024 at 12:44:28PM -0300, Jason Gunthorpe wrote:
> > We are talking about P2P memory here.  How do you manage to get a page
> > that dma_map_page can be used on?  All P2P memory needs to use the P2P
> > aware dma_map_sg as the pages for P2P memory are just fake zone device
> > pages.
> 
> FWIW, I've been expecting this series to be rebased on top of Leon's
> new DMA API series so it doesn't have this issue..

That's not going to make a difference at this level.

> I'm guessing they got their testing done so far on a system without an
> iommu translation?

IOMMU or not doens't matter much for P2P.  The important difference is
through the host bridge or through a switch.  dma_map_page will work
for P2P through the host brige (assuming the host bridge even support
it as it also lacks the error handling for when not), but it lacks the
handling for P2P through a switch.

> 
> > which also makes it clear that returning a page from the method is
> > not that great, a PFN might work a lot better, e.g.
> > 
> > 	unsigned long (*device_private_dma_pfn)(struct page *page);
> 
> Ideally I think we should not have the struct page * at all through
> these APIs if we can avoid it..

The input page is the device private page that we have at hand
anyway.  Until that scheme is complete redone it is the right kind
of parameter.

On Wed, Oct 16, 2024 at 09:41:03AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2024 at 12:44:28PM -0300, Jason Gunthorpe wrote:
> > > We are talking about P2P memory here.  How do you manage to get a page
> > > that dma_map_page can be used on?  All P2P memory needs to use the P2P
> > > aware dma_map_sg as the pages for P2P memory are just fake zone device
> > > pages.
> > 
> > FWIW, I've been expecting this series to be rebased on top of Leon's
> > new DMA API series so it doesn't have this issue..
> 
> That's not going to make a difference at this level.

I'm not sure what you are asking then.

Patch 2 does pci_p2pdma_add_resource() and so a valid struct page with
a P2P ZONE_DEVICE type exists, and that gets returned back to the
hmm/odp code.

Today odp calls dma_map_page() which only works by chance in limited
cases. With Leon's revision it will call hmm_dma_map_pfn() ->
dma_iova_link() which does call pci_p2pdma_map_type() and should do
the right thing.

> > I'm guessing they got their testing done so far on a system without an
> > iommu translation?
> 
> IOMMU or not doens't matter much for P2P.  The important difference is
> through the host bridge or through a switch.  dma_map_page will work
> for P2P through the host brige (assuming the host bridge even support
> it as it also lacks the error handling for when not), but it lacks the
> handling for P2P through a switch.

On most x86 systems the BAR/bus address of the P2P memory is the same
as the CPU address, so without an IOMMU translation dma_map_page()
will return the CPU/host physical address which is the same as the
BAR/bus address and that will take the P2P switch path for testing.

Jason

On Wed, Oct 16, 2024 at 02:44:45PM -0300, Jason Gunthorpe wrote:
> > > FWIW, I've been expecting this series to be rebased on top of Leon's
> > > new DMA API series so it doesn't have this issue..
> > 
> > That's not going to make a difference at this level.
> 
> I'm not sure what you are asking then.
> 
> Patch 2 does pci_p2pdma_add_resource() and so a valid struct page with
> a P2P ZONE_DEVICE type exists, and that gets returned back to the
> hmm/odp code.
> 
> Today odp calls dma_map_page() which only works by chance in limited
> cases. With Leon's revision it will call hmm_dma_map_pfn() ->
> dma_iova_link() which does call pci_p2pdma_map_type() and should do
> the right thing.

Again none of this affects the code posted here.  It reshuffles the
callers but has no direct affect on the patches posted here.

(and the current DMA series lacks P2P support, I'm trying to figure
out how to properly handle it at the moment).

> > IOMMU or not doens't matter much for P2P.  The important difference is
> > through the host bridge or through a switch.  dma_map_page will work
> > for P2P through the host brige (assuming the host bridge even support
> > it as it also lacks the error handling for when not), but it lacks the
> > handling for P2P through a switch.
> 
> On most x86 systems the BAR/bus address of the P2P memory is the same
> as the CPU address, so without an IOMMU translation dma_map_page()
> will return the CPU/host physical address which is the same as the
> BAR/bus address and that will take the P2P switch path for testing.

Maybe.  Either way the use of dma_map_page is incorrect.


> 
> Jason
---end quoted text---

On Thu, Oct 17, 2024 at 04:58:12AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2024 at 02:44:45PM -0300, Jason Gunthorpe wrote:
> > > > FWIW, I've been expecting this series to be rebased on top of Leon's
> > > > new DMA API series so it doesn't have this issue..
> > > 
> > > That's not going to make a difference at this level.
> > 
> > I'm not sure what you are asking then.
> > 
> > Patch 2 does pci_p2pdma_add_resource() and so a valid struct page with
> > a P2P ZONE_DEVICE type exists, and that gets returned back to the
> > hmm/odp code.
> > 
> > Today odp calls dma_map_page() which only works by chance in limited
> > cases. With Leon's revision it will call hmm_dma_map_pfn() ->
> > dma_iova_link() which does call pci_p2pdma_map_type() and should do
> > the right thing.
> 
> Again none of this affects the code posted here.  It reshuffles the
> callers but has no direct affect on the patches posted here.

I didn't realize till last night that Leon's series did not have P2P
support.

What I'm trying to say is that this is a multi-series project.

A followup based on Leon's initial work will get the ODP DMA mapping
path able to support ZONE_DEVICE P2P pages.

Once that is done, this series sits on top. This series is only about
hmm and effectively allows hmm_range_fault() to return a ZONE_DEVICE
P2P page.

Yonatan should explain this better in the cover letter and mark it as
a RFC series.

So, I know we are still figuring out the P2P support on the DMA API
side, but my expectation for hmm is that hmm_range_fault() returing a
ZONE_DEVICE P2P page is going to be what we want.

> (and the current DMA series lacks P2P support, I'm trying to figure
> out how to properly handle it at the moment).

Yes, I see, I looked through those patches last night and there is a
gap there.

Broadly I think whatever flow NVMe uses for P2P will apply to ODP as
well.

Thanks,
Jason

On Thu, Oct 17, 2024 at 10:05:39AM -0300, Jason Gunthorpe wrote:
> Broadly I think whatever flow NVMe uses for P2P will apply to ODP as
> well.

ODP is a lot simpler than NVMe for P2P actually :(

On Thu, Oct 17, 2024 at 06:12:55AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 17, 2024 at 10:05:39AM -0300, Jason Gunthorpe wrote:
> > Broadly I think whatever flow NVMe uses for P2P will apply to ODP as
> > well.
> 
> ODP is a lot simpler than NVMe for P2P actually :(

What is your thinking there? I'm looking at the latest patches and I
would expect dma_iova_init() to accept a phys so it can call
pci_p2pdma_map_type() once for the whole transaction. It is a slow
operation.

Based on the result of pci_p2pdma_map_type() it would have to take one
of three paths: direct, iommu, or acs/switch.

It feels like dma_map_page() should become a new function that takes
in the state and then it can do direct or acs based on the type held
in the state.

ODP would have to refresh the state for each page, but could follow
the same code structure.

Jason

On Thu, Oct 17, 2024 at 10:46:44AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 06:12:55AM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 17, 2024 at 10:05:39AM -0300, Jason Gunthorpe wrote:
> > > Broadly I think whatever flow NVMe uses for P2P will apply to ODP as
> > > well.
> > 
> > ODP is a lot simpler than NVMe for P2P actually :(
> 
> What is your thinking there? I'm looking at the latest patches and I
> would expect dma_iova_init() to accept a phys so it can call
> pci_p2pdma_map_type() once for the whole transaction. It is a slow
> operation.

You can't do it for the whole transaction.  Here is my suggestion
for ODP:

http://git.infradead.org/?p=users/hch/misc.git;a=shortlog;h=refs/heads/dma-split-wip

For NVMe I need to figure out a way to split bios on a per P2P
type boundary as we don't have any space to record if something is a bus
mapped address.

On Thu, Oct 17, 2024 at 06:49:30AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 17, 2024 at 10:46:44AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 06:12:55AM -0700, Christoph Hellwig wrote:
> > > On Thu, Oct 17, 2024 at 10:05:39AM -0300, Jason Gunthorpe wrote:
> > > > Broadly I think whatever flow NVMe uses for P2P will apply to ODP as
> > > > well.
> > > 
> > > ODP is a lot simpler than NVMe for P2P actually :(
> > 
> > What is your thinking there? I'm looking at the latest patches and I
> > would expect dma_iova_init() to accept a phys so it can call
> > pci_p2pdma_map_type() once for the whole transaction. It is a slow
> > operation.
> 
> You can't do it for the whole transaction.  Here is my suggestion
> for ODP:
> 
> http://git.infradead.org/?p=users/hch/misc.git;a=shortlog;h=refs/heads/dma-split-wip

OK, this looks very promising. I sketched something similar to the
pci-p2pdma changes a while back too.

BTW this:

  iommu: generalize the batched sync after map interface

I am hoping to in a direction of adding a gather to the map, just like
unmap. So eventually instead of open coding iotlb_sync_map() you'd
flush the gather and it would do it.

> For NVMe I need to figure out a way to split bios on a per P2P
> type boundary as we don't have any space to record if something is a bus
> mapped address.

Yeah this came up before :\ Can't precompute the p2p type during bio
creation, splitting based on pgmap would be good enough.

Jason

On Thu, Oct 17, 2024 at 11:05:07AM -0300, Jason Gunthorpe wrote:
> BTW this:
> 
>   iommu: generalize the batched sync after map interface
> 
> I am hoping to in a direction of adding a gather to the map, just like
> unmap. So eventually instead of open coding iotlb_sync_map() you'd
> flush the gather and it would do it.

I don't really care either way, I just need something to not regress
map behavior vs map_sg.

On 16/10/2024 7:49, Christoph Hellwig wrote:
> The subject does not make sense.  All P2P is on ZONE_DEVICE pages.
> It seems like this is about device private memory?
> 
Correct, thanks, I will change it to `mm/hmm: HMM API to enable P2P DMA 
for device private pages`, on v2.

> On Tue, Oct 15, 2024 at 06:23:45PM +0300, Yonatan Maman wrote:
>> From: Yonatan Maman <Ymaman@Nvidia.com>
>>
>> hmm_range_fault() natively triggers a page fault on device private
>> pages, migrating them to RAM.
> 
> That "natively" above doesn't make sense to me.
What I meant to convey is that hmm_range_fault() by default triggered a 
page fault on device private pages, which results in them being migrated 
to RAM. In this patch, we are trying to provide a different (more 
optimized) handling flow of P2P instead of migration.

I will clarify this on v2.
> 
>> In some cases, such as with RDMA devices,
>> the migration overhead between the device (e.g., GPU) and the CPU, and
>> vice-versa, significantly damages performance. Thus, enabling Peer-to-
> 
> s/damages/degrades/
Fixed (v2), thanks
> 
>> Peer (P2P) DMA access for device private page might be crucial for
>> minimizing data transfer overhead.
>>
>> This change introduces an API to support P2P connections for device
>> private pages by implementing the following:
> 
> "This change.. " or "This patch.." is pointless, just explain what you
> are doing.
>


Fixed (v2), thanks

>>
>>   - Leveraging the struct pagemap_ops for P2P Page Callbacks. This
>>     callback involves mapping the page to MMIO and returning the
>>     corresponding PCI_P2P page.
> 
> While P2P uses the same underlying PCIe TLPs as MMIO, it is not
> MMIO by definition, as memory mapped I/O is by definition about
> the CPU memory mappping so that load and store instructions cause
> the I/O.  It also uses very different concepts in Linux.
> 

Got it. I just wanted to emphasize that this function could be used to 
dynamically map the page for MMIO. However, I understand the potential 
confusion between MMIO and P2P, so I’ll remove that part.

>>   - Utilizing hmm_range_fault for Initializing P2P Connections. The API
> 
> There is no concept of a "connection" in PCIe dta transfers.
>

what about "initializing P2P Transfers" or "initializing P2P DMA"?

>>     also adds the HMM_PFN_REQ_TRY_P2P flag option for the
>>     hmm_range_fault caller to initialize P2P. If set, hmm_range_fault
>>     attempts initializing the P2P connection first, if the owner device
>>     supports P2P, using p2p_page. In case of failure or lack of support,
>>     hmm_range_fault will continue with the regular flow of migrating the
>>     page to RAM.
> 
> What is the need for the flag?  As far as I can tell from reading
> the series, the P2P mapping is entirely transparent to the callers
> of hmm_range_fault.
> 

This flag mirrors the GUP flag (FOLL_PCI_P2PDMA). The purpose of this 
flag is to ensure compatibility with existing flows that utilizing 
hmm_range_fault but may not inherently support PCI P2P.

>> +	/*
>> +	 * Used for private (un-addressable) device memory only. Return a
>> +	 * corresponding struct page, that can be mapped to device
>> +	 * (e.g using dma_map_page)
>> +	 */
>> +	struct page *(*get_dma_page_for_device)(struct page *private_page);
> 
> We are talking about P2P memory here.  How do you manage to get a page
> that dma_map_page can be used on?  All P2P memory needs to use the P2P
> aware dma_map_sg as the pages for P2P memory are just fake zone device
> pages.
>

According to our experiments dma_map_page is fully worked with PCI_P2P 
pages, I will double check that. If not we can either return this 
information using HMM flags or adding support to dma_map_page.

> 
>> +		 * P2P for supported pages, and according to caller request
>> +		 * translate the private page to the match P2P page if it fails
>> +		 * continue with the regular flow
>> +		 */
>> +		if (is_device_private_entry(entry)) {
>> +			get_dma_page_handler =
>> +				pfn_swap_entry_to_page(entry)
>> +					->pgmap->ops->get_dma_page_for_device;
>> +			if ((hmm_vma_walk->range->default_flags &
>> +			    HMM_PFN_REQ_ALLOW_P2P) &&
>> +			    get_dma_page_handler) {
>> +				dma_page = get_dma_page_handler(
>> +					pfn_swap_entry_to_page(entry));
> 
> This is really messy.  You probably really want to share a branch
> with the private page handling for the owner so that you only need
> a single is_device_private_entry and can use a local variable for
> to shortcut finding the page.  Probably best done with a little helper:
> 
> Then  this becomes:
> 
> static bool hmm_handle_device_private(struct hmm_range *range,
> 		swp_entry_t entry, unsigned long *hmm_pfn)
> {
> 	struct page *page = pfn_swap_entry_to_page(entry);
> 	struct dev_pagemap *pgmap = page->pgmap;
> 
> 	if (pgmap->owner == range->dev_private_owner) {
> 		*hmm_pfn = swp_offset_pfn(entry);
> 		goto found;
> 	}
> 
> 	if (pgmap->ops->get_dma_page_for_device) {
> 		*hmm_pfn =
> 			page_to_pfn(pgmap->ops->get_dma_page_for_device(page));
> 		goto found;
> 	}
> 
> 	return false;
> 
> found:
> 	*hmm_pfn |= HMM_PFN_VALID
> 	if (is_writable_device_private_entry(entry))
> 		*hmm_pfn |= HMM_PFN_WRITE;
> 	return true;
> }
> 
> which also makes it clear that returning a page from the method is
> not that great, a PFN might work a lot better, e.g.
> 
> 	unsigned long (*device_private_dma_pfn)(struct page *page);

I Agree, I will fix that (v2).