* [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
@ 2020-03-13 2:11 Souptick Joarder
2020-03-13 2:58 ` Andrew Morton
2020-03-13 12:22 ` Jason Gunthorpe
0 siblings, 2 replies; 7+ messages in thread
From: Souptick Joarder @ 2020-03-13 2:11 UTC (permalink / raw)
To: jglisse, akpm; +Cc: linux-mm, linux-kernel, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 2:11 [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held() Souptick Joarder
@ 2020-03-13 2:58 ` Andrew Morton
2020-03-13 3:47 ` Souptick Joarder
2020-03-13 12:22 ` Jason Gunthorpe
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2020-03-13 2:58 UTC (permalink / raw)
To: Souptick Joarder; +Cc: jglisse, linux-mm, linux-kernel
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?)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 2:58 ` Andrew Morton
@ 2020-03-13 3:47 ` Souptick Joarder
2020-03-13 3:57 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Souptick Joarder @ 2020-03-13 3:47 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jérôme Glisse, Linux-MM, linux-kernel
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?) *
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 3:47 ` Souptick Joarder
@ 2020-03-13 3:57 ` Andrew Morton
2020-03-13 4:22 ` Souptick Joarder
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2020-03-13 3:57 UTC (permalink / raw)
To: Souptick Joarder; +Cc: Jérôme Glisse, Linux-MM, linux-kernel
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?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 3:57 ` Andrew Morton
@ 2020-03-13 4:22 ` Souptick Joarder
0 siblings, 0 replies; 7+ messages in thread
From: Souptick Joarder @ 2020-03-13 4:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jérôme Glisse, Linux-MM, linux-kernel
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().
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 2:11 [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held() Souptick Joarder
2020-03-13 2:58 ` Andrew Morton
@ 2020-03-13 12:22 ` Jason Gunthorpe
2020-03-22 15:16 ` Souptick Joarder
1 sibling, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2020-03-13 12:22 UTC (permalink / raw)
To: Souptick Joarder; +Cc: jglisse, akpm, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held()
2020-03-13 12:22 ` Jason Gunthorpe
@ 2020-03-22 15:16 ` Souptick Joarder
0 siblings, 0 replies; 7+ messages in thread
From: Souptick Joarder @ 2020-03-22 15:16 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jérôme Glisse, Andrew Morton, Linux-MM, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-22 15:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 2:11 [PATCH] mm/hmm.c : Remove additional check for lockdep_assert_held() Souptick Joarder
2020-03-13 2:58 ` Andrew Morton
2020-03-13 3:47 ` Souptick Joarder
2020-03-13 3:57 ` Andrew Morton
2020-03-13 4:22 ` Souptick Joarder
2020-03-13 12:22 ` Jason Gunthorpe
2020-03-22 15:16 ` Souptick Joarder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).