[PATCH 1/3] drm/xe: Introduce helper to populate userptr
Zeng, Oak
oak.zeng at intel.com
Wed Apr 3 04:43:36 UTC 2024
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, April 3, 2024 12:15 AM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> <thomas.hellstrom at intel.com>; Welty, Brian <brian.welty at intel.com>; Ghimiray,
> Himal Prasad <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 1/3] drm/xe: Introduce helper to populate userptr
>
> On Tue, Apr 02, 2024 at 09:54:50PM -0600, Zeng, Oak wrote:
> > Hi Matt,
> >
> > For the comments to the static function, I prefer to keep it. It always helpful to
> have a comments.
> >
> > I agree most of the comments, except the mmap lock one. See inline
> >
> > Also answered a question.
> >
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost at intel.com>
> > > Sent: Tuesday, April 2, 2024 11:04 PM
> > > To: Zeng, Oak <oak.zeng at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> > > <thomas.hellstrom at intel.com>; Welty, Brian <brian.welty at intel.com>;
> Ghimiray,
> > > Himal Prasad <himal.prasad.ghimiray at intel.com>
> > > Subject: Re: [PATCH 1/3] drm/xe: Introduce helper to populate userptr
> > >
> > > On Thu, Mar 21, 2024 at 11:55:18PM -0400, Oak Zeng wrote:
> > > > Introduce a helper function xe_userptr_populate_range to populate
> > > > a a userptr range. This functions calls hmm_range_fault to read
> > > > CPU page tables and populate all pfns/pages of this virtual address
> > > > range.
> > > >
> > > > If the populated page is system memory page, dma-mapping is performed
> > > > to get a dma-address which can be used later for GPU to access pages.
> > > >
> > > > If the populated page is device private page, we calculate the dpa (
> > > > device physical address) of the page. This will be handled in future
> > > > patches.
> > > >
> > > > The dma-address or dpa is then saved in userptr's sg table. This is
> > > > prepare work to replace the get_user_pages_fast code in userptr code
> > > > path.
> > > >
> > > > v1: Address review comments:
> > > > separate a npage_in_range function (Matt)
> > > > reparameterize function xe_userptr_populate_range function (Matt)
> > > > move mmu_interval_read_begin() call into while loop (Thomas)
> > > > s/mark_range_accessed/xe_mark_range_accessed (Thomas)
> > > > use set_page_dirty_lock (vs set_page_dirty) (Thomas)
> > > > move a few checking in xe_vma_userptr_pin_pages to hmm.c (Matt)
> > > > v2: Remove device private page support. Only support system
> > > > pages for now. use dma-map-sg rather than dma-map-page
> (Matt/Thomas)
> > > >
> > > > Signed-off-by: Oak Zeng <oak.zeng at intel.com>
> > > > Co-developed-by: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura at intel.com>
> > > > Signed-off-by: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura at intel.com>
> > > > Cc: Matthew Brost <matthew.brost at intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom at intel.com>
> > > > Cc: Brian Welty <brian.welty at intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/Kconfig | 1 +
> > > > drivers/gpu/drm/xe/Makefile | 2 +
> > > > drivers/gpu/drm/xe/xe_hmm.c | 224
> > > ++++++++++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/xe/xe_hmm.h | 17 +++
> > > > 4 files changed, 244 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/xe/xe_hmm.c
> > > > create mode 100644 drivers/gpu/drm/xe/xe_hmm.h
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> > > > index 1a556d087e63..449a1ecbc92a 100644
> > > > --- a/drivers/gpu/drm/xe/Kconfig
> > > > +++ b/drivers/gpu/drm/xe/Kconfig
> > > > @@ -41,6 +41,7 @@ config DRM_XE
> > > > select MMU_NOTIFIER
> > > > select WANT_DEV_COREDUMP
> > > > select AUXILIARY_BUS
> > > > + select HMM_MIRROR
> > > > help
> > > > Experimental driver for Intel Xe series GPUs
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > > > index 3c3e67885559..a805a543056d 100644
> > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > @@ -145,6 +145,8 @@ xe-y += xe_bb.o \
> > > > xe_wa.o \
> > > > xe_wopcm.o
> > > >
> > > > +xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
> > > > +
> > > > # graphics hardware monitoring (HWMON) support
> > > > xe-$(CONFIG_HWMON) += xe_hwmon.o
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_hmm.c
> b/drivers/gpu/drm/xe/xe_hmm.c
> > > > new file mode 100644
> > > > index 000000000000..4011207630a5
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/xe/xe_hmm.c
> > > > @@ -0,0 +1,224 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation
> > > > + */
> > > > +
> > > > +#include <linux/mmu_notifier.h>
> > > > +#include <linux/dma-mapping.h>
> > > > +#include <linux/memremap.h>
> > > > +#include <linux/swap.h>
> > > > +#include <linux/hmm.h>
> > > > +#include <linux/mm.h>
> > > > +#include "xe_hmm.h"
> > > > +#include "xe_vm.h"
> > > > +#include "xe_bo.h"
> > > > +
> > > > +static u64 xe_npages_in_range(unsigned long start, unsigned long end)
> > > > +{
> > > > + return (PAGE_ALIGN(end) - PAGE_ALIGN_DOWN(start)) >> PAGE_SHIFT;
> > >
> > > Nit: start and end should always be page aligned.
> > >
> > > > +}
> > > > +
> > > > +/**
> > > > + * xe_mark_range_accessed() - mark a range is accessed, so core mm
> > > > + * have such information for memory eviction or write back to
> > > > + * hard disk
> > > > + *
> > > > + * @range: the range to mark
> > > > + * @write: if write to this range, we mark pages in this range
> > > > + * as dirty
> > > > + */
> > >
> > > Doesn't hurt but kernel doc is not required for static functions.
> > >
> > > > +static void xe_mark_range_accessed(struct hmm_range *range, bool
> write)
> > > > +{
> > > > + struct page *page;
> > > > + u64 i, npages;
> > > > +
> > > > + npages = xe_npages_in_range(range->start, range->end);
> > > > + for (i = 0; i < npages; i++) {
> > > > + page = hmm_pfn_to_page(range->hmm_pfns[i]);
> > > > + if (write)
> > > > + set_page_dirty_lock(page);
> > > > +
> > > > + mark_page_accessed(page);
> > > > + }
> > > > +}
> > > > +
> > > > +/**
> > > > + * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
> > > > + * in a hmm_range. dma-map pages if necessary. dma-address is save in sg
> > > table
> > > > + * and will be used to program GPU page table later.
> > > > + *
> > > > + * @xe: the xe device who will access the dma-address in sg table
> > > > + * @range: the hmm range that we build the sg table from. range-
> > > >hmm_pfns[]
> > > > + * has the pfn numbers of pages that back up this hmm address range.
> > > > + * @st: pointer to the sg table.
> > > > + * @write: whether we write to this range. This decides dma map direction
> > > > + * for system pages. If write we map it bi-diretional; otherwise
> > > > + * DMA_TO_DEVICE
> > > > + *
> > > > + * All the contiguous pfns will be collapsed into one entry in
> > > > + * the scatter gather table. This is for the purpose of efficiently
> > > > + * programming GPU page table.
> > > > + *
> > > > + * The dma_address in the sg table will later be used by GPU to
> > > > + * access memory. So if the memory is system memory, we need to
> > > > + * do a dma-mapping so it can be accessed by GPU/DMA.
> > > > + *
> > > > + * FIXME: This function currently only support pages in system
> > > > + * memory. If the memory is GPU local memory (of the GPU who
> > > > + * is going to access memory), we need gpu dpa (device physical
> > > > + * address), and there is no need of dma-mapping. This is TBD.
> > > > + *
> > > > + * FIXME: dma-mapping for peer gpu device to access remote gpu's
> > > > + * memory. Add this when you support p2p
> > > > + *
> > > > + * This function allocates the storage of the sg table. It is
> > > > + * caller's responsibility to free it calling sg_free_table.
> > > > + *
> > > > + * Returns 0 if successful; -ENOMEM if fails to allocate memory
> > > > + */
> > >
> > > Kinda verbose given a static function but guess that is ok.
> > >
> > > > +static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
> > > > + struct sg_table *st, bool write)
> > >
> > > Weird indentation, use look at checkpatch.
> > >
> > > > +{
> > > > + struct device *dev = xe->drm.dev;
> > > > + struct page **pages;
> > > > + u64 i, npages;
> > > > + int ret;
> > > > +
> > > > + npages = xe_npages_in_range(range->start, range->end);
> > > > + pages = kvmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
> > > > + if (!pages)
> > > > + return -ENOMEM;
> > > > +
> > > > + for (i = 0; i < npages; i++) {
> > > > + pages[i] = hmm_pfn_to_page(range->hmm_pfns[i]);
> > > > + xe_assert(xe, !is_device_private_page(pages[i]));
> > > > + }
> > > > +
> > > > + ret = sg_alloc_table_from_pages_segment(st, pages, npages, 0,
> > > > + npages << PAGE_SHIFT, xe_sg_segment_size(dev),
> > > GFP_KERNEL);
> > >
> > > Weird indentation here too.
> > >
> > >
> > >
> > > > + if (ret)
> > > > + goto free_pages;
> > > > +
> > > > + ret = dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL :
> > > DMA_TO_DEVICE,
> > > > + DMA_ATTR_SKIP_CPU_SYNC |
> > > DMA_ATTR_NO_KERNEL_MAPPING);
> > > > +
> > > > +free_pages:
> > >
> > > The SG table isn't free'd if dma_map_sgtable fails.
> > >
> > > > + kvfree(pages);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * xe_userptr_populate_range() - Populate physical pages of a virtual
> > > > + * address range
> > > > + *
> > > > + * @uvma: userptr vma which has information of the range to populate.
> > > > + *
> > > > + * This function populate the physical pages of a virtual
> > > > + * address range. The populated physical pages is saved in
> > > > + * userptr's sg table. It is similar to get_user_pages but call
> > > > + * hmm_range_fault.
> > > > + *
> > > > + * This function also read mmu notifier sequence # (
> > > > + * mmu_interval_read_begin), for the purpose of later
> > > > + * comparison (through mmu_interval_read_retry).
> > > > + *
> > > > + * This must be called with mmap read or write lock held.
> > > > + *
> > > > + * This function allocates the storage of the userptr sg table.
> > > > + * It is caller's responsibility to free it calling sg_free_table.
> > > > + *
> > > > + * returns: 0 for succuss; negative error no on failure
> > > > + */
> > > > +int xe_userptr_populate_range(struct xe_userptr_vma *uvma)
> > >
> > > See my comments about arguments in previous rev. Still would raather
> > > have this layer accept arguments from xe_userptr_vma rather than that
> > > struct but can live with xe_userptr_vma as an argument I suppose.
> > >
> > > But s/xe_userptr_populate_range/xe_hmm_userptr_populate_range/ given
> > > that this is file xe_hmm.c - exported functions should try to have the
> > > 'xe_hmm' prefix.
> > >
> > > > +{
> > > > + unsigned long timeout =
> > > > + jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> > > > + unsigned long *pfns, flags = HMM_PFN_REQ_FAULT;
> > > > + struct xe_userptr *userptr;
> > > > + struct xe_vma *vma = &uvma->vma;
> > > > + u64 start = xe_vma_userptr(vma);
> > > > + u64 end = start + xe_vma_size(vma);
> > > > + struct xe_vm *vm = xe_vma_vm(vma);
> > > > + struct hmm_range hmm_range;
> > > > + bool write = !xe_vma_read_only(vma);
> > > > + bool in_kthread = !current->mm;
> > > > + unsigned long notifier_seq;
> > > > + u64 npages;
> > > > + int ret;
> > > > +
> > > > + userptr = &uvma->userptr;
> > > > + mmap_assert_locked(userptr->notifier.mm);
> > >
> > > See my comments about locking in an eariler rev, while it may be true
> > > the system allocator code has to call this function with mmap locked it
> > > certainly non-system allocator code does not.
> >
> > In the original non-system allocator codes, the mmap lock is just embedded in
> the get_user_page function. So they do have mmap lock.
> >
>
> Yes, so that aligns with grabbing / releasing the lock directly before /
> after hmm_range_fault. General a good practice hold a lock for a little
> period of time as possible. This especially is true for a widly used
> lock like mmap lock.
>
> > Or do you see a problem to ask caller to hold mmap lock? This scheme is
> suggested by hmm design document. It works for both userptr and hmmptr. I
> don't see an obvious problem.
> >
>
> Not a problem, just see above. For non-system allocator code the lock
> only needs to held around the hmm_range_fault call. This lock might need
> to held longer in the system allocator prevent races between a CPU fault
> and a GPU fault. If that is needed, see my suggestion about a locked
> version or argument for this function.
Yes for system allocator we need to hold mmap lock for migration. It looks like this:
Mmap lock
Migration
Populate range
Mmap unlock
I have no objection to introduce a bool lock argument to this function
Oak
>
> Matt
>
> > Oak
> >
> >
> >
> > I'd rather move the mmap
> > > lock around the hmm_range_fault call for now. If the system allocator
> > > needs to hold mmap, probably add locked argument (or *_locked) function
> > > with the proper lockdep asserts / do not take mmap lock around
> > > hmm_range_fault.
> > >
> > > > +
> > > > + if (vma->gpuva.flags & XE_VMA_DESTROYED)
> > > > + return 0;
> > > > +
> > > > + notifier_seq = mmu_interval_read_begin(&userptr->notifier);
> > > > + if (notifier_seq == userptr->notifier_seq)
> > > > + return 0;
> > > > +
> > > > + npages = xe_npages_in_range(start, end);
> > > > + pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
> > > > + if (unlikely(!pfns))
> > > > + return -ENOMEM;
> > > > +
> > > > + if (write)
> > > > + flags |= HMM_PFN_REQ_WRITE;
> > > > +
> > > > + if (in_kthread) {
> > > > + if (!mmget_not_zero(userptr->notifier.mm)) {
> > > > + ret = -EFAULT;
> > > > + goto free_pfns;
> > > > + }
> > > > + kthread_use_mm(userptr->notifier.mm);
> > >
> > > I think this can actually be dropped as hmm_range_fault takes the
> > > notifier (and the MM) as an argument. Do you want to test this out?
> > >
> > > If we can drop kthread_use_mm, we blindly (no kthread test) probably
> > > just call mmget_not_zero.
> > >
> > > > + }
> > > > +
> > > > + memset64((u64 *)pfns, (u64)flags, npages);
> > >
> > > See my comment in an ealier rev about using default flags. Still valid.
> > >
> > > > + hmm_range.hmm_pfns = pfns;
> > > > + hmm_range.notifier = &userptr->notifier;
> > > > + hmm_range.start = start;
> > > > + hmm_range.end = end;
> > > > + hmm_range.pfn_flags_mask = HMM_PFN_REQ_FAULT |
> > > HMM_PFN_REQ_WRITE;
> > > > + /**
> > > > + * FIXME:
> > > > + * Set the dev_private_owner can prevent hmm_range_fault to fault
> > > > + * in the device private pages owned by caller. See function
> > > > + * hmm_vma_handle_pte. In multiple GPU case, this should be set to the
> > > > + * device owner of the best migration destination. e.g., device0/vm0
> > > > + * has a page fault, but we have determined the best placement of
> > > > + * the fault address should be on device1, we should set below to
> > > > + * device1 instead of device0.
> > > > + */
> > >
> > > I'd drop this FIXME, I prefer not to predict the future in comments.
> > >
> > > > + hmm_range.dev_private_owner = vm->xe;
> > > > +
> > >
> > > Longterm, probably a helper to derive private owner. Likely just
> > > something like:
> > >
> > > xe_dev_private_owner(xe)
> > > return xe
> > >
> > > But it help avoid bugs setting the private owner to the wrong value in
> > > various places.
> > >
> > > > + while (true) {
> > > > + hmm_range.notifier_seq = mmu_interval_read_begin(&userptr-
> > > >notifier);
> > >
> > > We set hmm_range.notifier_seq above... maybe set this after if (ret ==-
> EBUSY)
> > >
> > > Again see my comment about locking.
> > >
> > > > + ret = hmm_range_fault(&hmm_range);
> > > > + if (time_after(jiffies, timeout))
> > > > + break;
> > >
> > > Maybe also move this under the if (ret ==-EBUSY) logic too.
> > >
> > > > +
> > > > + if (ret == -EBUSY)
> > > > + continue;
> > > > + break;
> > > > + }
> > > > +
> > > > + if (in_kthread) {
> > > > + kthread_unuse_mm(userptr->notifier.mm);
> > > > + mmput(userptr->notifier.mm);
> > >
> > > See comment above about kthread_unuse_mm.
> > >
> > > > + }
> > > > +
> > > > + if (ret)
> > > > + goto free_pfns;
> > > > +
> > > > + ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt, write);
> > > > + if (ret)
> > > > + goto free_pfns;
> > > > +
> > > > + xe_mark_range_accessed(&hmm_range, write);
> > > > + userptr->sg = &userptr->sgt;
> > > > + userptr->notifier_seq = hmm_range.notifier_seq;
> > > > +
> > > > +free_pfns:
> > > > + kvfree(pfns);
> > > > + return ret;
> > > > +}
> > > > +
> > > > diff --git a/drivers/gpu/drm/xe/xe_hmm.h
> b/drivers/gpu/drm/xe/xe_hmm.h
> > > > new file mode 100644
> > > > index 000000000000..91686a751711
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/xe/xe_hmm.h
> > > > @@ -0,0 +1,17 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation
> > > > + */
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct xe_userptr_vma;
> > > > +
> > > > +#if IS_ENABLED(CONFIG_HMM_MIRROR)
> > >
> > > Do we need this? Don't we just require CONFIG_HMM_MIRROR to build our
> > > driver?
> >
> > Yes we need this. Our driver/userptr should build even if HMM_MIRROR is
> disabled. I had a build error without this.
> >
> > Oak
> >
> >
> > >
> > > Matt
> > >
> > > > +int xe_userptr_populate_range(struct xe_userptr_vma *uvma);
> > > > +#else
> > > > +static inline int xe_userptr_populate_range(struct xe_userptr_vma *uvma)
> > > > +{
> > > > + return -ENODEV;
> > > > +}
> > > > +#endif
> > > > --
> > > > 2.26.3
> > > >
More information about the Intel-xe
mailing list