[PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Souptick Joarder]

walk_page_range() already has a check for lockdep_assert_held().
So additional check for lockdep_assert_held() can be removed from
hmm_range_fault().

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 mm/hmm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 72e5a6d..b201e1e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
 	struct mm_struct *mm = range->notifier->mm;
 	int ret;
 
-	lockdep_assert_held(&mm->mmap_sem);
 
 	do {
 		/* If range is no longer valid force retry. */
-- 
1.9.1

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Andrew Morton]

On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> walk_page_range() already has a check for lockdep_assert_held().
> So additional check for lockdep_assert_held() can be removed from
> hmm_range_fault().
> 
> ...
>
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
>  	struct mm_struct *mm = range->notifier->mm;
>  	int ret;
>  
> -	lockdep_assert_held(&mm->mmap_sem);
>  
>  	do {
>  		/* If range is no longer valid force retry. */

It isn't very obvious that hmm_range_fault() is and will only be called
from walk_page_range() (is it?)

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Souptick Joarder]

On Fri, Mar 13, 2020 at 8:28 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> > walk_page_range() already has a check for lockdep_assert_held().
> > So additional check for lockdep_assert_held() can be removed from
> > hmm_range_fault().
> >
> > ...
> >
> > --- a/mm/hmm.c
> > +++ b/mm/hmm.c
> > @@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
> >       struct mm_struct *mm = range->notifier->mm;
> >       int ret;
> >
> > -     lockdep_assert_held(&mm->mmap_sem);
> >
> >       do {
> >               /* If range is no longer valid force retry. */
>
> It isn't very obvious that hmm_range_fault() is and will only be called
> from walk_page_range() (is it?)
>

Sorry Andrew, didn't get this part ?
* hmm_range_fault() is and will only be called
 from walk_page_range() (is it?) *

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Andrew Morton]

On Fri, 13 Mar 2020 09:17:22 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> On Fri, Mar 13, 2020 at 8:28 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >
> > > walk_page_range() already has a check for lockdep_assert_held().
> > > So additional check for lockdep_assert_held() can be removed from
> > > hmm_range_fault().
> > >
> > > ...
> > >
> > > --- a/mm/hmm.c
> > > +++ b/mm/hmm.c
> > > @@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
> > >       struct mm_struct *mm = range->notifier->mm;
> > >       int ret;
> > >
> > > -     lockdep_assert_held(&mm->mmap_sem);
> > >
> > >       do {
> > >               /* If range is no longer valid force retry. */
> >
> > It isn't very obvious that hmm_range_fault() is and will only be called
> > from walk_page_range() (is it?)
> >
> 
> Sorry Andrew, didn't get this part ?
> * hmm_range_fault() is and will only be called
>  from walk_page_range() (is it?) *

The patch assumes that hmm_range_fault() will only ever be called via
walk_page_range().  How do we know this is the case?  And that it
always will be the case?

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Souptick Joarder]

On Fri, Mar 13, 2020 at 9:27 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Mar 2020 09:17:22 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> > On Fri, Mar 13, 2020 at 8:28 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:
> > >
> > > > walk_page_range() already has a check for lockdep_assert_held().
> > > > So additional check for lockdep_assert_held() can be removed from
> > > > hmm_range_fault().
> > > >
> > > > ...
> > > >
> > > > --- a/mm/hmm.c
> > > > +++ b/mm/hmm.c
> > > > @@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
> > > >       struct mm_struct *mm = range->notifier->mm;
> > > >       int ret;
> > > >
> > > > -     lockdep_assert_held(&mm->mmap_sem);
> > > >
> > > >       do {
> > > >               /* If range is no longer valid force retry. */
> > >
> > > It isn't very obvious that hmm_range_fault() is and will only be called
> > > from walk_page_range() (is it?)
> > >
> >
> > Sorry Andrew, didn't get this part ?
> > * hmm_range_fault() is and will only be called
> >  from walk_page_range() (is it?) *
>
> The patch assumes that hmm_range_fault() will only ever be called via
> walk_page_range().  How do we know this is the case?  And that it
> always will be the case?
>

Ahh, Sorry, I think change log creates confusion.

The patch assumes that walk_page_range() is called from hmm_range_fault().
currently there are two caller for hmm_range_fault().
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c, line 859
drivers/gpu/drm/nouveau/nouveau_svm.c, line 544

in both case, * &mm->mmap_sem * lock is taken before calling hmm_range_fault().
Now inside hmm_range_fault() there is a check for
lockdep_assert_held(&mm->mmap_sem)
and again inside loop walk_page_range() is called which cross check
same lockdep_assert_held().

So the idea is to remove the first check
lockdep_assert_held(&mm->mmap_sem) in hmm_range_fault().

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Jason Gunthorpe]

On Fri, Mar 13, 2020 at 07:41:00AM +0530, Souptick Joarder wrote:
> walk_page_range() already has a check for lockdep_assert_held().
> So additional check for lockdep_assert_held() can be removed from
> hmm_range_fault().

Is there a reason why you think this redundancy is bad?

IMHO it makes it easier to understand the API contract if key top
level APIs have their assumptions coded in lockdep.

Jason

Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()

[Souptick Joarder]

On Fri, Mar 13, 2020 at 5:52 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Fri, Mar 13, 2020 at 07:41:00AM +0530, Souptick Joarder wrote:
> > walk_page_range() already has a check for lockdep_assert_held().
> > So additional check for lockdep_assert_held() can be removed from
> > hmm_range_fault().
>
> Is there a reason why you think this redundancy is bad?

Other than removing an extra check , I don't have any other strong
reason to support this patch.
>
> IMHO it makes it easier to understand the API contract if key top
> level APIs have their assumptions coded in lockdep.

Ok, I will drop this patch. Sorry for the noise.