| Message ID | 20241015152348.3055360-2-ymaman@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GPU Direct RDMA (P2P DMA) for Device Private Pages | expand |
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 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).
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 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
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
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
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 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 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
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) {