Re: [PATCH v3 12/14] drm/amdgpu: Use mmu_interval_notifier instead of hmm_mirror

I test v3 and it works fine.

Regards, Philip

On 2019-11-12 3:22 p.m., Jason Gunthorpe wrote:

From: Jason Gunthorpe jgg@mellanox.com

Convert the collision-retry lock around hmm_range_fault to use the one now provided by the mmu_interval notifier.

Although this driver does not seem to use the collision retry lock that hmm provides correctly, it can still be converted over to use the mmu_interval_notifier api instead of hmm_mirror without too much trouble.

This also deletes another place where a driver is associating additional data (struct amdgpu_mn) with a mmu_struct.

Signed-off-by: Philip Yang Philip.Yang@amd.com Reviewed-by: Philip Yang Philip.Yang@amd.com Tested-by: Philip Yang Philip.Yang@amd.com Signed-off-by: Jason Gunthorpe jgg@mellanox.com

---

.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 4 + drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 148 ++---------------- drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h | 49 ------ drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 116 ++++++++------ 5 files changed, 94 insertions(+), 237 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 47700302a08b7f..1bcedb9b477dce 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -1738,6 +1738,10 @@ static int update_invalid_user_pages(struct amdkfd_process_info *process_info, return ret; }

• /*
  

•  * FIXME: Cannot ignore the return code, must hold
  

•  * notifier_lock
  

•  */
  

  amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);

  /* Mark the BO as valid unless it was invalidated

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 82823d9a8ba887..22c989bca7514c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -603,8 +603,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, e->tv.num_shared = 2;

amdgpu_bo_list_get_list(p->bo_list, &p->validated);

• if (p->bo_list->first_userptr != p->bo_list->num_entries)

• p->mn = amdgpu_mn_get(p->adev, AMDGPU_MN_TYPE_GFX);
  
  

  INIT_LIST_HEAD(&duplicates); amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);

@@ -1287,11 +1285,11 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, if (r) goto error_unlock;

• /* No memory allocation is allowed while holding the mn lock.

• * p->mn is hold until amdgpu_cs_submit is finished and fence is added
  

• * to BOs.
  

• /* No memory allocation is allowed while holding the notifier lock.

• * The lock is held until amdgpu_cs_submit is finished and fence is
  

• * added to BOs.
  

  */

• amdgpu_mn_lock(p->mn);

• mutex_lock(&p->adev->notifier_lock);

  /* If userptr are invalidated after amdgpu_cs_parser_bos(), return

  • -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl.

@@ -1334,13 +1332,13 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);

ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);

• amdgpu_mn_unlock(p->mn);

• mutex_unlock(&p->adev->notifier_lock);

  return 0;

  error_abort: drm_sched_job_cleanup(&job->base);

• amdgpu_mn_unlock(p->mn);

• mutex_unlock(&p->adev->notifier_lock);

  error_unlock: amdgpu_job_free(job);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c index 9fe1c31ce17a30..828b5167ff128f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c @@ -50,28 +50,6 @@ #include "amdgpu.h" #include "amdgpu_amdkfd.h"

-/**

• • amdgpu_mn_lock - take the write side lock for this notifier
• • @mn: our notifier
• */

-void amdgpu_mn_lock(struct amdgpu_mn *mn) -{

• if (mn)

• down_write(&mn->lock);
  

-}

-/**

• • amdgpu_mn_unlock - drop the write side lock for this notifier
• • @mn: our notifier
• */

-void amdgpu_mn_unlock(struct amdgpu_mn *mn) -{

• if (mn)

• up_write(&mn->lock);
  

-}

• /**
  • amdgpu_mn_invalidate_gfx - callback to notify about mm change

@@ -94,6 +72,9 @@ static bool amdgpu_mn_invalidate_gfx(struct mmu_interval_notifier *mni, return false;

mutex_lock(&adev->notifier_lock);

• mmu_interval_set_seq(mni, cur_seq);
• r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, true, false, MAX_SCHEDULE_TIMEOUT); mutex_unlock(&adev->notifier_lock);

@@ -127,6 +108,9 @@ static bool amdgpu_mn_invalidate_hsa(struct mmu_interval_notifier *mni, return false;

mutex_lock(&adev->notifier_lock);

• mmu_interval_set_seq(mni, cur_seq);
• amdgpu_amdkfd_evict_userptr(bo->kfd_bo, bo->notifier.mm); mutex_unlock(&adev->notifier_lock);

@@ -137,92 +121,6 @@ static const struct mmu_interval_notifier_ops amdgpu_mn_hsa_ops = { .invalidate = amdgpu_mn_invalidate_hsa, };

-static int amdgpu_mn_sync_pagetables(struct hmm_mirror *mirror,

• 		     const struct mmu_notifier_range *update)
  

-{

• struct amdgpu_mn *amn = container_of(mirror, struct amdgpu_mn, mirror);
• if (!mmu_notifier_range_blockable(update))

• return -EAGAIN;
  

• down_read(&amn->lock);
• up_read(&amn->lock);
• return 0;

-}

-/* Low bits of any reasonable mm pointer will be unused due to struct

• • alignment. Use these bits to make a unique key from the mm pointer
• • and notifier type.
• */

-#define AMDGPU_MN_KEY(mm, type) ((unsigned long)(mm) + (type))

-static struct hmm_mirror_ops amdgpu_hmm_mirror_ops[] = {

• [AMDGPU_MN_TYPE_GFX] = {

• .sync_cpu_device_pagetables = amdgpu_mn_sync_pagetables,
  

• },
• [AMDGPU_MN_TYPE_HSA] = {

• .sync_cpu_device_pagetables = amdgpu_mn_sync_pagetables,
  

• },

-};

-/**

• • amdgpu_mn_get - create HMM mirror context
• • @adev: amdgpu device pointer
• • @type: type of MMU notifier context
• • Creates a HMM mirror context for current->mm.
• */

-struct amdgpu_mn *amdgpu_mn_get(struct amdgpu_device *adev,

• 		enum amdgpu_mn_type type)
  

-{

• struct mm_struct *mm = current->mm;
• struct amdgpu_mn *amn;
• unsigned long key = AMDGPU_MN_KEY(mm, type);
• int r;
• mutex_lock(&adev->mn_lock);
• if (down_write_killable(&mm->mmap_sem)) {

• mutex_unlock(&adev->mn_lock);
  

• return ERR_PTR(-EINTR);
  

• }
• hash_for_each_possible(adev->mn_hash, amn, node, key)

• if (AMDGPU_MN_KEY(amn->mirror.hmm->mmu_notifier.mm,
  

• 		  amn->type) == key)
  

• 	goto release_locks;
  

• amn = kzalloc(sizeof(*amn), GFP_KERNEL);
• if (!amn) {

• amn = ERR_PTR(-ENOMEM);
  

• goto release_locks;
  

• }
• amn->adev = adev;
• init_rwsem(&amn->lock);
• amn->type = type;
• amn->mirror.ops = &amdgpu_hmm_mirror_ops[type];
• r = hmm_mirror_register(&amn->mirror, mm);
• if (r)

• goto free_amn;
  

• hash_add(adev->mn_hash, &amn->node, AMDGPU_MN_KEY(mm, type));

-release_locks:

• up_write(&mm->mmap_sem);
• mutex_unlock(&adev->mn_lock);
• return amn;

-free_amn:

• up_write(&mm->mmap_sem);
• mutex_unlock(&adev->mn_lock);
• kfree(amn);
• return ERR_PTR(r);

-}

• /**
  • amdgpu_mn_register - register a BO for notifier updates

@@ -235,12 +133,12 @@ struct amdgpu_mn *amdgpu_mn_get(struct amdgpu_device *adev, int amdgpu_mn_register(struct amdgpu_bo *bo, unsigned long addr) { if (bo->kfd_bo)

• bo->notifier.ops = &amdgpu_mn_hsa_ops;
  

• else

• bo->notifier.ops = &amdgpu_mn_gfx_ops;
  

• return mmu_interval_notifier_insert(&bo->notifier, addr,

• 			    amdgpu_bo_size(bo), current->mm);
  

• return mmu_interval_notifier_insert(&bo->notifier, current->mm,
  

• 				    addr, amdgpu_bo_size(bo),
  

• 				    &amdgpu_mn_hsa_ops);
  

• return mmu_interval_notifier_insert(&bo->notifier, current->mm, addr,

• 			    amdgpu_bo_size(bo),
  

• 			    &amdgpu_mn_gfx_ops);
  

  }

  /**

@@ -257,25 +155,3 @@ void amdgpu_mn_unregister(struct amdgpu_bo *bo) mmu_interval_notifier_remove(&bo->notifier); bo->notifier.mm = NULL; }

-/* flags used by HMM internal, not related to CPU/GPU PTE flags */ -static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {

• (1 << 0), /* HMM_PFN_VALID */
  

• (1 << 1), /* HMM_PFN_WRITE */
  

• 0 /* HMM_PFN_DEVICE_PRIVATE */
  

-};

-static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {

• 0xfffffffffffffffeUL, /* HMM_PFN_ERROR */
  

• 0, /* HMM_PFN_NONE */
  

• 0xfffffffffffffffcUL /* HMM_PFN_SPECIAL */
  

-};

-void amdgpu_hmm_init_range(struct hmm_range *range) -{

• if (range) {

• range->flags = hmm_range_flags;
  

• range->values = hmm_range_values;
  

• range->pfn_shift = PAGE_SHIFT;
  

• }

-} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h index d73ab2947b22b2..a292238f75ebae 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h @@ -30,59 +30,10 @@ #include <linux/workqueue.h> #include <linux/interval_tree.h>

-enum amdgpu_mn_type {

• AMDGPU_MN_TYPE_GFX,
• AMDGPU_MN_TYPE_HSA,

-};

-/**

• • struct amdgpu_mn
• • @adev: amdgpu device pointer
• • @type: type of MMU notifier
• • @work: destruction work item
• • @node: hash table node to find structure by adev and mn
• • @lock: rw semaphore protecting the notifier nodes
• • @mirror: HMM mirror function support
• • Data for each amdgpu device and process address space.
• */

-struct amdgpu_mn {

• /* constant after initialisation */
• struct amdgpu_device *adev;
• enum amdgpu_mn_type type;
• /* only used on destruction */
• struct work_struct work;
• /* protected by adev->mn_lock */
• struct hlist_node node;
• /* objects protected by lock */
• struct rw_semaphore lock;

-#ifdef CONFIG_HMM_MIRROR

• /* HMM mirror */
• struct hmm_mirror mirror;

-#endif -};

• #if defined(CONFIG_HMM_MIRROR)

-void amdgpu_mn_lock(struct amdgpu_mn *mn); -void amdgpu_mn_unlock(struct amdgpu_mn *mn); -struct amdgpu_mn *amdgpu_mn_get(struct amdgpu_device *adev,

• 		enum amdgpu_mn_type type);
  

  int amdgpu_mn_register(struct amdgpu_bo *bo, unsigned long addr); void amdgpu_mn_unregister(struct amdgpu_bo *bo);

-void amdgpu_hmm_init_range(struct hmm_range *range); #else -static inline void amdgpu_mn_lock(struct amdgpu_mn *mn) {} -static inline void amdgpu_mn_unlock(struct amdgpu_mn *mn) {} -static inline struct amdgpu_mn *amdgpu_mn_get(struct amdgpu_device *adev,

• 			      enum amdgpu_mn_type type)
  

-{

• return NULL;

-} static inline int amdgpu_mn_register(struct amdgpu_bo *bo, unsigned long addr) { DRM_WARN_ONCE("HMM_MIRROR kernel config option is not enabled, " diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index c0e41f1f0c2365..c41a26bde852e6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -773,6 +773,20 @@ struct amdgpu_ttm_tt { #endif };

+#ifdef CONFIG_DRM_AMDGPU_USERPTR +/* flags used by HMM internal, not related to CPU/GPU PTE flags */ +static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {

• (1 << 0), /* HMM_PFN_VALID */
• (1 << 1), /* HMM_PFN_WRITE */
• 0 /* HMM_PFN_DEVICE_PRIVATE */

+};

+static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {

• 0xfffffffffffffffeUL, /* HMM_PFN_ERROR */
• 0, /* HMM_PFN_NONE */
• 0xfffffffffffffffcUL /* HMM_PFN_SPECIAL */

+};

• /**
  • amdgpu_ttm_tt_get_user_pages - get device accessible pages that back user
  • memory and start HMM tracking CPU page table update

@@ -780,29 +794,28 @@ struct amdgpu_ttm_tt {

• Calling function must call amdgpu_ttm_tt_userptr_range_done() once and only
• once afterwards to stop HMM tracking

*/ -#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)

-#define MAX_RETRY_HMM_RANGE_FAULT 16

• int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) {
• struct hmm_mirror *mirror = bo->mn ? &bo->mn->mirror : NULL; struct ttm_tt *ttm = bo->tbo.ttm; struct amdgpu_ttm_tt *gtt = (void *)ttm;
• struct mm_struct *mm; unsigned long start = gtt->userptr; struct vm_area_struct *vma; struct hmm_range *range;

• unsigned long timeout;
• struct mm_struct *mm; unsigned long i;

• uint64_t *pfns; int r = 0;

• if (unlikely(!mirror)) {

• DRM_DEBUG_DRIVER("Failed to get hmm_mirror\n");
  

• mm = bo->notifier.mm;
• if (unlikely(!mm)) {

• DRM_DEBUG_DRIVER("BO is not registered?\n");
  

  return -EFAULT; }

• mm = mirror->hmm->mmu_notifier.mm;

• /* Another get_user_pages is running at the same time?? */
• if (WARN_ON(gtt->range))

• return -EFAULT;
  

• if (!mmget_not_zero(mm)) /* Happens during process shutdown */ return -ESRCH;

@@ -811,31 +824,23 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) r = -ENOMEM; goto out; }

• range->notifier = &bo->notifier;
• range->flags = hmm_range_flags;
• range->values = hmm_range_values;
• range->pfn_shift = PAGE_SHIFT;
• range->start = bo->notifier.interval_tree.start;
• range->end = bo->notifier.interval_tree.last + 1;
• range->default_flags = hmm_range_flags[HMM_PFN_VALID];
• if (!amdgpu_ttm_tt_is_readonly(ttm))

• range->default_flags |= range->flags[HMM_PFN_WRITE];
  
  

• pfns = kvmalloc_array(ttm->num_pages, sizeof(*pfns), GFP_KERNEL);
• if (unlikely(!pfns)) {

• range->pfns = kvmalloc_array(ttm->num_pages, sizeof(*range->pfns),

• 		     GFP_KERNEL);
  

• if (unlikely(!range->pfns)) { r = -ENOMEM; goto out_free_ranges; }

• amdgpu_hmm_init_range(range);
• range->default_flags = range->flags[HMM_PFN_VALID];
• range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?

• 		0 : range->flags[HMM_PFN_WRITE];
  

• range->pfn_flags_mask = 0;
• range->pfns = pfns;
• range->start = start;
• range->end = start + ttm->num_pages * PAGE_SIZE;
• hmm_range_register(range, mirror);
• /*

• * Just wait for range to be valid, safe to ignore return value as we
  

• * will use the return value of hmm_range_fault() below under the
  

• * mmap_sem to ascertain the validity of the range.
  

• */
  

• hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT);
• down_read(&mm->mmap_sem); vma = find_vma(mm, start); if (unlikely(!vma || start < vma->vm_start)) {

@@ -847,18 +852,31 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) r = -EPERM; goto out_unlock; }

• up_read(&mm->mmap_sem);
• timeout = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);

+retry:

• range->notifier_seq = mmu_interval_read_begin(&bo->notifier);
• down_read(&mm->mmap_sem); r = hmm_range_fault(range, 0); up_read(&mm->mmap_sem);

• if (unlikely(r < 0))

• if (unlikely(r <= 0)) {

• /*
  

•  * FIXME: This timeout should encompass the retry from
  

•  * mmu_interval_read_retry() as well.
  

•  */
  

• if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout))
  

• 	goto retry;
  

  goto out_free_pfns;

• }

  for (i = 0; i < ttm->num_pages; i++) {

• pages[i] = hmm_device_entry_to_page(range, pfns[i]);
  

• /* FIXME: The pages cannot be touched outside the notifier_lock */
  

• pages[i] = hmm_device_entry_to_page(range, range->pfns[i]);
  

  if (unlikely(!pages[i])) { pr_err("Page fault failed for pfn[%lu] = 0x%llx\n",

• 	       i, pfns[i]);
  

• 	       i, range->pfns[i]);
  r = -ENOMEM;
  
  goto out_free_pfns;
  

@@ -873,8 +891,7 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) out_unlock: up_read(&mm->mmap_sem); out_free_pfns:

• hmm_range_unregister(range);
• kvfree(pfns);

• kvfree(range->pfns); out_free_ranges: kfree(range); out:

@@ -903,15 +920,18 @@ bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm) "No user pages to check\n");

if (gtt->range) {

• r = hmm_range_valid(gtt->range);
  

• hmm_range_unregister(gtt->range);
  

• /*
  

•  * FIXME: Must always hold notifier_lock for this, and must
  

•  * not ignore the return code.
  

•  */
  

• r = mmu_interval_read_retry(gtt->range->notifier,
  

• 			 gtt->range->notifier_seq);
  

  kvfree(gtt->range->pfns); kfree(gtt->range); gtt->range = NULL; }

• return r;

• return !r; } #endif

@@ -992,10 +1012,18 @@ static void amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm) sg_free_table(ttm->sg);

#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)

• if (gtt->range &&

•    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
  

• 				      gtt->range->pfns[0]))
  

• WARN_ONCE(1, "Missing get_user_page_done\n");
  

• if (gtt->range) {

• unsigned long i;
  

• for (i = 0; i < ttm->num_pages; i++) {
  

• 	if (ttm->pages[i] !=
  

• 		hmm_device_entry_to_page(gtt->range,
  

• 			      gtt->range->pfns[i]))
  

• 		break;
  

• }
  

• WARN((i == ttm->num_pages), "Missing get_user_page_done\n");
  

• } #endif }