| Message ID | 20190701062020.19239-21-hch@lst.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/22] mm/hmm.c: suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set | expand |
On 6/30/19 11:20 PM, Christoph Hellwig wrote: > hmm_vma_fault is marked as a legacy API to get rid of, but quite suites > the current nouvea flow. Move it to the only user in preparation for I didn't quite parse the phrase "quite suites the current nouvea flow." s/nouvea/nouveau/ > fixing a locking bug involving caller and callee. > > Signed-off-by: Christoph Hellwig <hch@lst.de> I see where you are going with this and it looks like straightforward code movement so, Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > --- > drivers/gpu/drm/nouveau/nouveau_svm.c | 54 ++++++++++++++++++++++++++- > include/linux/hmm.h | 54 --------------------------- > 2 files changed, 53 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c > index 9d40114d7949..e831f4184a17 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > @@ -36,6 +36,13 @@ > #include <linux/sort.h> > #include <linux/hmm.h> > > +/* > + * When waiting for mmu notifiers we need some kind of time out otherwise we > + * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to > + * wait already. > + */ > +#define NOUVEAU_RANGE_FAULT_TIMEOUT 1000 > + > struct nouveau_svm { > struct nouveau_drm *drm; > struct mutex mutex; > @@ -475,6 +482,51 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm, > fault->inst, fault->addr, fault->access); > } > > +static int > +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, > + bool block) > +{ > + long ret; > + > + /* > + * With the old API the driver must set each individual entries with > + * the requested flags (valid, write, ...). So here we set the mask to > + * keep intact the entries provided by the driver and zero out the > + * default_flags. > + */ > + range->default_flags = 0; > + range->pfn_flags_mask = -1UL; > + > + ret = hmm_range_register(range, mirror, > + range->start, range->end, > + PAGE_SHIFT); > + if (ret) > + return (int)ret; > + > + if (!hmm_range_wait_until_valid(range, NOUVEAU_RANGE_FAULT_TIMEOUT)) { > + /* > + * The mmap_sem was taken by driver we release it here and > + * returns -EAGAIN which correspond to mmap_sem have been > + * drop in the old API. > + */ > + up_read(&range->vma->vm_mm->mmap_sem); > + return -EAGAIN; > + } > + > + ret = hmm_range_fault(range, block); > + if (ret <= 0) { > + if (ret == -EBUSY || !ret) { > + /* Same as above, drop mmap_sem to match old API. */ > + up_read(&range->vma->vm_mm->mmap_sem); > + ret = -EBUSY; > + } else if (ret == -EAGAIN) > + ret = -EBUSY; > + hmm_range_unregister(range); > + return ret; > + } > + return 0; > +} > + > static int > nouveau_svm_fault(struct nvif_notify *notify) > { > @@ -649,7 +701,7 @@ nouveau_svm_fault(struct nvif_notify *notify) > range.values = nouveau_svm_pfn_values; > range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT; > again: > - ret = hmm_vma_fault(&svmm->mirror, &range, true); > + ret = nouveau_range_fault(&svmm->mirror, &range, true); > if (ret == 0) { > mutex_lock(&svmm->mutex); > if (!hmm_range_unregister(&range)) { > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 4b185d286c3b..3457cf9182e5 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -478,60 +478,6 @@ long hmm_range_dma_unmap(struct hmm_range *range, > dma_addr_t *daddrs, > bool dirty); > > -/* > - * HMM_RANGE_DEFAULT_TIMEOUT - default timeout (ms) when waiting for a range > - * > - * When waiting for mmu notifiers we need some kind of time out otherwise we > - * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to > - * wait already. > - */ > -#define HMM_RANGE_DEFAULT_TIMEOUT 1000 > - > -/* This is a temporary helper to avoid merge conflict between trees. */ > -static inline int hmm_vma_fault(struct hmm_mirror *mirror, > - struct hmm_range *range, bool block) > -{ > - long ret; > - > - /* > - * With the old API the driver must set each individual entries with > - * the requested flags (valid, write, ...). So here we set the mask to > - * keep intact the entries provided by the driver and zero out the > - * default_flags. > - */ > - range->default_flags = 0; > - range->pfn_flags_mask = -1UL; > - > - ret = hmm_range_register(range, mirror, > - range->start, range->end, > - PAGE_SHIFT); > - if (ret) > - return (int)ret; > - > - if (!hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT)) { > - /* > - * The mmap_sem was taken by driver we release it here and > - * returns -EAGAIN which correspond to mmap_sem have been > - * drop in the old API. > - */ > - up_read(&range->vma->vm_mm->mmap_sem); > - return -EAGAIN; > - } > - > - ret = hmm_range_fault(range, block); > - if (ret <= 0) { > - if (ret == -EBUSY || !ret) { > - /* Same as above, drop mmap_sem to match old API. */ > - up_read(&range->vma->vm_mm->mmap_sem); > - ret = -EBUSY; > - } else if (ret == -EAGAIN) > - ret = -EBUSY; > - hmm_range_unregister(range); > - return ret; > - } > - return 0; > -} > - > /* Below are for HMM internal use only! Not to be used by device driver! */ > static inline void hmm_mm_init(struct mm_struct *mm) > { >
On Wed, Jul 3, 2019 at 1:49 PM Ralph Campbell <rcampbell@nvidia.com> wrote: > On 6/30/19 11:20 PM, Christoph Hellwig wrote: > > hmm_vma_fault is marked as a legacy API to get rid of, but quite suites > > the current nouvea flow. Move it to the only user in preparation for > > I didn't quite parse the phrase "quite suites the current nouvea flow." > s/nouvea/nouveau/ As long as you're fixing typos, suites -> suits.
On Mon, Jul 01, 2019 at 08:20:18AM +0200, Christoph Hellwig wrote: > hmm_vma_fault is marked as a legacy API to get rid of, but quite suites > the current nouvea flow. Move it to the only user in preparation for > fixing a locking bug involving caller and callee. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > drivers/gpu/drm/nouveau/nouveau_svm.c | 54 ++++++++++++++++++++++++++- > include/linux/hmm.h | 54 --------------------------- > 2 files changed, 53 insertions(+), 55 deletions(-) I was thinking about doing exactly this too, but amdgpu started using this already obsolete API in their latest driver :( So, we now need to get both drivers to move to the modern API. Jason
On Wed, Jul 03, 2019 at 03:03:56PM -0300, Jason Gunthorpe wrote: > I was thinking about doing exactly this too, but amdgpu started using > this already obsolete API in their latest driver :( > > So, we now need to get both drivers to move to the modern API. Actually the AMD folks fixed this up after we pointed it out to them, so even in linux-next it just is nouveau that needs fixing.
On Wed, Jul 03, 2019 at 08:05:25PM +0200, Christoph Hellwig wrote: > On Wed, Jul 03, 2019 at 03:03:56PM -0300, Jason Gunthorpe wrote: > > I was thinking about doing exactly this too, but amdgpu started using > > this already obsolete API in their latest driver :( > > > > So, we now need to get both drivers to move to the modern API. > > Actually the AMD folks fixed this up after we pointed it out to them, > so even in linux-next it just is nouveau that needs fixing. Oh, I looked at an older -next, my mistake. Lets do it then. Jason
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 9d40114d7949..e831f4184a17 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -36,6 +36,13 @@ #include <linux/sort.h> #include <linux/hmm.h> +/* + * When waiting for mmu notifiers we need some kind of time out otherwise we + * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to + * wait already. + */ +#define NOUVEAU_RANGE_FAULT_TIMEOUT 1000 + struct nouveau_svm { struct nouveau_drm *drm; struct mutex mutex; @@ -475,6 +482,51 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm, fault->inst, fault->addr, fault->access); } +static int +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, + bool block) +{ + long ret; + + /* + * With the old API the driver must set each individual entries with + * the requested flags (valid, write, ...). So here we set the mask to + * keep intact the entries provided by the driver and zero out the + * default_flags. + */ + range->default_flags = 0; + range->pfn_flags_mask = -1UL; + + ret = hmm_range_register(range, mirror, + range->start, range->end, + PAGE_SHIFT); + if (ret) + return (int)ret; + + if (!hmm_range_wait_until_valid(range, NOUVEAU_RANGE_FAULT_TIMEOUT)) { + /* + * The mmap_sem was taken by driver we release it here and + * returns -EAGAIN which correspond to mmap_sem have been + * drop in the old API. + */ + up_read(&range->vma->vm_mm->mmap_sem); + return -EAGAIN; + } + + ret = hmm_range_fault(range, block); + if (ret <= 0) { + if (ret == -EBUSY || !ret) { + /* Same as above, drop mmap_sem to match old API. */ + up_read(&range->vma->vm_mm->mmap_sem); + ret = -EBUSY; + } else if (ret == -EAGAIN) + ret = -EBUSY; + hmm_range_unregister(range); + return ret; + } + return 0; +} + static int nouveau_svm_fault(struct nvif_notify *notify) { @@ -649,7 +701,7 @@ nouveau_svm_fault(struct nvif_notify *notify) range.values = nouveau_svm_pfn_values; range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT; again: - ret = hmm_vma_fault(&svmm->mirror, &range, true); + ret = nouveau_range_fault(&svmm->mirror, &range, true); if (ret == 0) { mutex_lock(&svmm->mutex); if (!hmm_range_unregister(&range)) { diff --git a/include/linux/hmm.h b/include/linux/hmm.h index 4b185d286c3b..3457cf9182e5 100644 --- a/include/linux/hmm.h +++ b/include/linux/hmm.h @@ -478,60 +478,6 @@ long hmm_range_dma_unmap(struct hmm_range *range, dma_addr_t *daddrs, bool dirty); -/* - * HMM_RANGE_DEFAULT_TIMEOUT - default timeout (ms) when waiting for a range - * - * When waiting for mmu notifiers we need some kind of time out otherwise we - * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to - * wait already. - */ -#define HMM_RANGE_DEFAULT_TIMEOUT 1000 - -/* This is a temporary helper to avoid merge conflict between trees. */ -static inline int hmm_vma_fault(struct hmm_mirror *mirror, - struct hmm_range *range, bool block) -{ - long ret; - - /* - * With the old API the driver must set each individual entries with - * the requested flags (valid, write, ...). So here we set the mask to - * keep intact the entries provided by the driver and zero out the - * default_flags. - */ - range->default_flags = 0; - range->pfn_flags_mask = -1UL; - - ret = hmm_range_register(range, mirror, - range->start, range->end, - PAGE_SHIFT); - if (ret) - return (int)ret; - - if (!hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT)) { - /* - * The mmap_sem was taken by driver we release it here and - * returns -EAGAIN which correspond to mmap_sem have been - * drop in the old API. - */ - up_read(&range->vma->vm_mm->mmap_sem); - return -EAGAIN; - } - - ret = hmm_range_fault(range, block); - if (ret <= 0) { - if (ret == -EBUSY || !ret) { - /* Same as above, drop mmap_sem to match old API. */ - up_read(&range->vma->vm_mm->mmap_sem); - ret = -EBUSY; - } else if (ret == -EAGAIN) - ret = -EBUSY; - hmm_range_unregister(range); - return ret; - } - return 0; -} - /* Below are for HMM internal use only! Not to be used by device driver! */ static inline void hmm_mm_init(struct mm_struct *mm) {
hmm_vma_fault is marked as a legacy API to get rid of, but quite suites the current nouvea flow. Move it to the only user in preparation for fixing a locking bug involving caller and callee. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/gpu/drm/nouveau/nouveau_svm.c | 54 ++++++++++++++++++++++++++- include/linux/hmm.h | 54 --------------------------- 2 files changed, 53 insertions(+), 55 deletions(-)