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

PATCH In review Souptick Joarder 3 participants 7 messages Mar 13, 2020 – Mar 22, 2020

Subsystems (2)

Memory management HMM - Heterogeneous Memory Management

lore

Share & tools

lore

Souptick Joarder

author

2020-03-13 02:03 UTC #0 lore raw

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

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 <[email protected]>

---
 mm/hmm.c | 1 -
 1 file changed, 1 deletion(-)

mm/hmm.c +0 -1

index 72e5a6d..b201e1e 100644
@@ -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. */

Signature · 1 line

1.9.1

Andrew Morton

maintainer

2020-03-13 02:58 UTC #1 lore raw

Quoted 9 lines · On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <[email protected]>…

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().
...

mm/hmm.c long hmm_range_fault(struct hmm_range *range, unsigned int flags) · line 686

Quoted 7 lines

mm/hmm.c +0 -1

@@ -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?)

Souptick Joarder

author

2020-03-13 03:47 UTC #2 lore raw

Quoted 15 lines · Andrew Morton

Quoted 12 lines · On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <[email protected]>…
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().
...
mm/hmm.c +0 -0
@@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
      struct mm_struct *mm = range->notifier->mm;
      int ret;
 

Quoted 8 lines

mm/hmm.c long hmm_range_fault(struct hmm_range *range, unsigned int flags) · line 686
Quoted 4 lines
mm/hmm.c +0 -1
-     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?) *

Andrew Morton

maintainer

2020-03-13 03:57 UTC #3 lore raw

Quoted 25 lines · On Fri, 13 Mar 2020 09:17:22 +0530 Souptick Joarder <[email protected]>…

Quoted 15 lines · Andrew Morton
Quoted 12 lines · On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <[email protected]>…
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().
...
mm/hmm.c +0 -0
@@ -681,7 +681,6 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
      struct mm_struct *mm = range->notifier->mm;
      int ret;
 
Quoted 8 lines
mm/hmm.c long hmm_range_fault(struct hmm_range *range, unsigned int flags) · line 686
Quoted 4 lines
mm/hmm.c +0 -1
-     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?)

Quoted 3 lines

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?

Souptick Joarder

author

2020-03-13 04:22 UTC #4 lore raw

Quoted 28 lines · Andrew Morton

Quoted 25 lines · On Fri, 13 Mar 2020 09:17:22 +0530 Souptick Joarder <[email protected]>…
Quoted 23 lines · Andrew Morton
Quoted 9 lines · On Fri, 13 Mar 2020 07:41:00 +0530 Souptick Joarder <[email protected]>…
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().
...
mm/hmm.c long hmm_range_fault(struct hmm_range *range, unsigned int flags) · line 686
Quoted 7 lines
mm/hmm.c +0 -1
@@ -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?)

Quoted 8 lines

Quoted 3 lines
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().

Jason Gunthorpe

2020-03-13 12:22 UTC #5 lore raw

In reply to Souptick Joarder

Quoted 3 lines · 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().

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

Souptick Joarder

author

2020-03-22 15:16 UTC #6 lore raw

Quoted 7 lines · Jason Gunthorpe

Quoted 3 lines · 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().
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.

Quoted 3 lines

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.