Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
On Tue, May 14, 2019 at 5:12 PM Kuehling, Felix wrote: > > > On 2019-05-13 4:21 p.m., Deucher, Alexander wrote: > > [CAUTION: External Email] > > I reverted all the amdgpu HMM patches for 5.2 because they also > > depended on this patch: > > https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=ce05ef71564f7cbe270cd4337c36ee720ea534db > > which did not have a clear line of sight for 5.2 either. > > When was that? I saw "Use HMM for userptr" in Dave's 5.2-rc1 pull > request to Linus. https://patchwork.kernel.org/patch/10875587/ Alex > > > Regards, >Felix > > > > > > Alex > > > > *From:* amd-gfx on behalf of > > Kuehling, Felix > > *Sent:* Monday, May 13, 2019 3:36 PM > > *To:* Jerome Glisse > > *Cc:* linux...@kvack.org; airl...@gmail.com; > > amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > > alex.deuc...@amd.com > > *Subject:* Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for > > non-blocking > > [CAUTION: External Email] > > > > Hi Jerome, > > > > Do you want me to push the patches to your branch? Or are you going to > > apply them yourself? > > > > Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you > > know when? I'd like to coordinate with Dave Airlie so that we can also > > get that update into a drm-next branch soon. > > > > I see that Linus merged Dave's pull request for Linux 5.2, which > > includes the first changes in amdgpu using HMM. They're currently broken > > without these two patches. > > > > Thanks, > >Felix > > > > On 2019-05-10 4:14 p.m., Jerome Glisse wrote: > > > [CAUTION: External Email] > > > > > > On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: > > >> Don't set this flag by default in hmm_vma_do_fault. It is set > > >> conditionally just a few lines below. Setting it unconditionally > > >> can lead to handle_mm_fault doing a non-blocking fault, returning > > >> -EBUSY and unlocking mmap_sem unexpectedly. > > >> > > >> Signed-off-by: Felix Kuehling > > > Reviewed-by: Jérôme Glisse > > > > > >> --- > > >> mm/hmm.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/mm/hmm.c b/mm/hmm.c > > >> index b65c27d5c119..3c4f1d62202f 100644 > > >> --- a/mm/hmm.c > > >> +++ b/mm/hmm.c > > >> @@ -339,7 +339,7 @@ struct hmm_vma_walk { > > >> static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, > > >>bool write_fault, uint64_t *pfn) > > >> { > > >> - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; > > >> + unsigned int flags = FAULT_FLAG_REMOTE; > > >>struct hmm_vma_walk *hmm_vma_walk = walk->private; > > >>struct hmm_range *range = hmm_vma_walk->range; > > >>struct vm_area_struct *vma = walk->vma; > > >> -- > > >> 2.17.1 > > >> > > ___ > > amd-gfx mailing list > > amd-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > ___ > amd-gfx mailing list > amd-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
On 2019-05-13 4:21 p.m., Deucher, Alexander wrote: > [CAUTION: External Email] > I reverted all the amdgpu HMM patches for 5.2 because they also > depended on this patch: > https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=ce05ef71564f7cbe270cd4337c36ee720ea534db > which did not have a clear line of sight for 5.2 either. When was that? I saw "Use HMM for userptr" in Dave's 5.2-rc1 pull request to Linus. Regards, Felix > > Alex > > *From:* amd-gfx on behalf of > Kuehling, Felix > *Sent:* Monday, May 13, 2019 3:36 PM > *To:* Jerome Glisse > *Cc:* linux...@kvack.org; airl...@gmail.com; > amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > alex.deuc...@amd.com > *Subject:* Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for > non-blocking > [CAUTION: External Email] > > Hi Jerome, > > Do you want me to push the patches to your branch? Or are you going to > apply them yourself? > > Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you > know when? I'd like to coordinate with Dave Airlie so that we can also > get that update into a drm-next branch soon. > > I see that Linus merged Dave's pull request for Linux 5.2, which > includes the first changes in amdgpu using HMM. They're currently broken > without these two patches. > > Thanks, > Felix > > On 2019-05-10 4:14 p.m., Jerome Glisse wrote: > > [CAUTION: External Email] > > > > On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: > >> Don't set this flag by default in hmm_vma_do_fault. It is set > >> conditionally just a few lines below. Setting it unconditionally > >> can lead to handle_mm_fault doing a non-blocking fault, returning > >> -EBUSY and unlocking mmap_sem unexpectedly. > >> > >> Signed-off-by: Felix Kuehling > > Reviewed-by: Jérôme Glisse > > > >> --- > >> mm/hmm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/hmm.c b/mm/hmm.c > >> index b65c27d5c119..3c4f1d62202f 100644 > >> --- a/mm/hmm.c > >> +++ b/mm/hmm.c > >> @@ -339,7 +339,7 @@ struct hmm_vma_walk { > >> static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, > >> bool write_fault, uint64_t *pfn) > >> { > >> - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; > >> + unsigned int flags = FAULT_FLAG_REMOTE; > >> struct hmm_vma_walk *hmm_vma_walk = walk->private; > >> struct hmm_range *range = hmm_vma_walk->range; > >> struct vm_area_struct *vma = walk->vma; > >> -- > >> 2.17.1 > >> > ___ > amd-gfx mailing list > amd-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
[Fixed Alex's email address, sorry for getting it wrong first] On 2019-05-13 3:49 p.m., Jerome Glisse wrote: > [CAUTION: External Email] > > Andrew can we get this 2 fixes line up for 5.2 ? > > On Mon, May 13, 2019 at 07:36:44PM +, Kuehling, Felix wrote: >> Hi Jerome, >> >> Do you want me to push the patches to your branch? Or are you going to >> apply them yourself? >> >> Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you >> know when? I'd like to coordinate with Dave Airlie so that we can also >> get that update into a drm-next branch soon. >> >> I see that Linus merged Dave's pull request for Linux 5.2, which >> includes the first changes in amdgpu using HMM. They're currently broken >> without these two patches. > HMM patch do not go through any git branch they go through the mmotm > collection. So it is not something you can easily coordinate with drm > branch. > > By broken i expect you mean that if numabalance happens it breaks ? > Or it might sleep when you are not expecting it too ? Without the NUMA fix we'd end up using an outdated physical address in the GPU page table. The problem was caught by a test that got incorrect computation results using OpenCL on a NUMA system. Without the FAULT_FLAG_ALLOW_RETRY patch, there can be kernel oopses due to incorrect locking/unlocking of mmap_sem. It breaks the promise that hmm_range_fault should not unlock the mmap_sem if block==true. It takes some memory pressure to trigger this. Regards, Felix > > Cheers, > Jérôme > >> Thanks, >> Felix >> >> On 2019-05-10 4:14 p.m., Jerome Glisse wrote: >>> [CAUTION: External Email] >>> >>> On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: Don't set this flag by default in hmm_vma_do_fault. It is set conditionally just a few lines below. Setting it unconditionally can lead to handle_mm_fault doing a non-blocking fault, returning -EBUSY and unlocking mmap_sem unexpectedly. Signed-off-by: Felix Kuehling >>> Reviewed-by: Jérôme Glisse >>> --- mm/hmm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/hmm.c b/mm/hmm.c index b65c27d5c119..3c4f1d62202f 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -339,7 +339,7 @@ struct hmm_vma_walk { static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, bool write_fault, uint64_t *pfn) { - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; + unsigned int flags = FAULT_FLAG_REMOTE; struct hmm_vma_walk *hmm_vma_walk = walk->private; struct hmm_range *range = hmm_vma_walk->range; struct vm_area_struct *vma = walk->vma; -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
I reverted all the amdgpu HMM patches for 5.2 because they also depended on this patch: https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=ce05ef71564f7cbe270cd4337c36ee720ea534db which did not have a clear line of sight for 5.2 either. Alex From: amd-gfx on behalf of Kuehling, Felix Sent: Monday, May 13, 2019 3:36 PM To: Jerome Glisse Cc: linux...@kvack.org; airl...@gmail.com; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; alex.deuc...@amd.com Subject: Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking [CAUTION: External Email] Hi Jerome, Do you want me to push the patches to your branch? Or are you going to apply them yourself? Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you know when? I'd like to coordinate with Dave Airlie so that we can also get that update into a drm-next branch soon. I see that Linus merged Dave's pull request for Linux 5.2, which includes the first changes in amdgpu using HMM. They're currently broken without these two patches. Thanks, Felix On 2019-05-10 4:14 p.m., Jerome Glisse wrote: > [CAUTION: External Email] > > On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: >> Don't set this flag by default in hmm_vma_do_fault. It is set >> conditionally just a few lines below. Setting it unconditionally >> can lead to handle_mm_fault doing a non-blocking fault, returning >> -EBUSY and unlocking mmap_sem unexpectedly. >> >> Signed-off-by: Felix Kuehling > Reviewed-by: Jérôme Glisse > >> --- >> mm/hmm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/hmm.c b/mm/hmm.c >> index b65c27d5c119..3c4f1d62202f 100644 >> --- a/mm/hmm.c >> +++ b/mm/hmm.c >> @@ -339,7 +339,7 @@ struct hmm_vma_walk { >> static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, >>bool write_fault, uint64_t *pfn) >> { >> - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; >> + unsigned int flags = FAULT_FLAG_REMOTE; >>struct hmm_vma_walk *hmm_vma_walk = walk->private; >>struct hmm_range *range = hmm_vma_walk->range; >>struct vm_area_struct *vma = walk->vma; >> -- >> 2.17.1 >> ___ amd-gfx mailing list amd-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
Andrew can we get this 2 fixes line up for 5.2 ? On Mon, May 13, 2019 at 07:36:44PM +, Kuehling, Felix wrote: > Hi Jerome, > > Do you want me to push the patches to your branch? Or are you going to > apply them yourself? > > Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you > know when? I'd like to coordinate with Dave Airlie so that we can also > get that update into a drm-next branch soon. > > I see that Linus merged Dave's pull request for Linux 5.2, which > includes the first changes in amdgpu using HMM. They're currently broken > without these two patches. HMM patch do not go through any git branch they go through the mmotm collection. So it is not something you can easily coordinate with drm branch. By broken i expect you mean that if numabalance happens it breaks ? Or it might sleep when you are not expecting it too ? Cheers, Jérôme > > Thanks, > Felix > > On 2019-05-10 4:14 p.m., Jerome Glisse wrote: > > [CAUTION: External Email] > > > > On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: > >> Don't set this flag by default in hmm_vma_do_fault. It is set > >> conditionally just a few lines below. Setting it unconditionally > >> can lead to handle_mm_fault doing a non-blocking fault, returning > >> -EBUSY and unlocking mmap_sem unexpectedly. > >> > >> Signed-off-by: Felix Kuehling > > Reviewed-by: Jérôme Glisse > > > >> --- > >> mm/hmm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/hmm.c b/mm/hmm.c > >> index b65c27d5c119..3c4f1d62202f 100644 > >> --- a/mm/hmm.c > >> +++ b/mm/hmm.c > >> @@ -339,7 +339,7 @@ struct hmm_vma_walk { > >> static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, > >>bool write_fault, uint64_t *pfn) > >> { > >> - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; > >> + unsigned int flags = FAULT_FLAG_REMOTE; > >>struct hmm_vma_walk *hmm_vma_walk = walk->private; > >>struct hmm_range *range = hmm_vma_walk->range; > >>struct vm_area_struct *vma = walk->vma; > >> -- > >> 2.17.1 > >> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
Hi Jerome, Do you want me to push the patches to your branch? Or are you going to apply them yourself? Is your hmm-5.2-v3 branch going to make it into Linux 5.2? If so, do you know when? I'd like to coordinate with Dave Airlie so that we can also get that update into a drm-next branch soon. I see that Linus merged Dave's pull request for Linux 5.2, which includes the first changes in amdgpu using HMM. They're currently broken without these two patches. Thanks, Felix On 2019-05-10 4:14 p.m., Jerome Glisse wrote: > [CAUTION: External Email] > > On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: >> Don't set this flag by default in hmm_vma_do_fault. It is set >> conditionally just a few lines below. Setting it unconditionally >> can lead to handle_mm_fault doing a non-blocking fault, returning >> -EBUSY and unlocking mmap_sem unexpectedly. >> >> Signed-off-by: Felix Kuehling > Reviewed-by: Jérôme Glisse > >> --- >> mm/hmm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/hmm.c b/mm/hmm.c >> index b65c27d5c119..3c4f1d62202f 100644 >> --- a/mm/hmm.c >> +++ b/mm/hmm.c >> @@ -339,7 +339,7 @@ struct hmm_vma_walk { >> static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, >>bool write_fault, uint64_t *pfn) >> { >> - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; >> + unsigned int flags = FAULT_FLAG_REMOTE; >>struct hmm_vma_walk *hmm_vma_walk = walk->private; >>struct hmm_range *range = hmm_vma_walk->range; >>struct vm_area_struct *vma = walk->vma; >> -- >> 2.17.1 >> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
On Fri, May 10, 2019 at 07:53:24PM +, Kuehling, Felix wrote: > Don't set this flag by default in hmm_vma_do_fault. It is set > conditionally just a few lines below. Setting it unconditionally > can lead to handle_mm_fault doing a non-blocking fault, returning > -EBUSY and unlocking mmap_sem unexpectedly. > > Signed-off-by: Felix Kuehling Reviewed-by: Jérôme Glisse > --- > mm/hmm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index b65c27d5c119..3c4f1d62202f 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -339,7 +339,7 @@ struct hmm_vma_walk { > static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, > bool write_fault, uint64_t *pfn) > { > - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; > + unsigned int flags = FAULT_FLAG_REMOTE; > struct hmm_vma_walk *hmm_vma_walk = walk->private; > struct hmm_range *range = hmm_vma_walk->range; > struct vm_area_struct *vma = walk->vma; > -- > 2.17.1 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] mm/hmm: Only set FAULT_FLAG_ALLOW_RETRY for non-blocking
Don't set this flag by default in hmm_vma_do_fault. It is set conditionally just a few lines below. Setting it unconditionally can lead to handle_mm_fault doing a non-blocking fault, returning -EBUSY and unlocking mmap_sem unexpectedly. Signed-off-by: Felix Kuehling --- mm/hmm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/hmm.c b/mm/hmm.c index b65c27d5c119..3c4f1d62202f 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -339,7 +339,7 @@ struct hmm_vma_walk { static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, bool write_fault, uint64_t *pfn) { - unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_REMOTE; + unsigned int flags = FAULT_FLAG_REMOTE; struct hmm_vma_walk *hmm_vma_walk = walk->private; struct hmm_range *range = hmm_vma_walk->range; struct vm_area_struct *vma = walk->vma; -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel