hugetlb: fix lockdep error

PATCH In review Nick Piggin 4 participants 9 messages May 29, 2008 2 messages not in the archive

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Nick Piggin

author

2008-05-29 02:29 UTC #2 lore raw

PATCH 1/1 hugetlb: fix lockdep error

Hi Andrew,

Can you merge this up please? It is helpful in testing to avoid lockdep tripping over. I have it at the start of the multiple hugepage size patchset, but it doesn't strictly belong there...

--
hugetlb: fix lockdep error

Acked-by KOSAKI Motohiro <[email protected]>

Acked-by Adam Litke <[email protected]>

Acked-by Nishanth Aravamudan <[email protected]>

Signed-off-by Nick Piggin <[email protected]>

---
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/hugetlb.c +1 -1

Index: linux-2.6/mm/hugetlb.c
===================================================================
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2008-05-29 02:38 UTC #3 lore raw

Quoted 6 lines · On Thu, 29 May 2008 04:29:19 +0200 Nick Piggin <[email protected]> wrote:

Hi Andrew,
Can you merge this up please? It is helpful in testing to avoid lockdep tripping over. I have it at the start of the multiple hugepage size patchset, but it doesn't strictly belong there...

Quoted 7 lines

--
hugetlb: fix lockdep error
Acked-by KOSAKI Motohiro <[email protected]>
Acked-by Adam Litke <[email protected]>
Acked-by Nishanth Aravamudan <[email protected]>
Signed-off-by Nick Piggin <[email protected]>

Please prefer to quote the warning/error message/trace/etc when fixing it.

Quoted 8 lines

 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================

mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791

Quoted 8 lines

mm/hugetlb.c +1 -1

@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);

Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Nick Piggin

author

2008-05-29 03:07 UTC #4 lore raw

Quoted 11 lines · Andrew Morton

Quoted 9 lines · On Thu, 29 May 2008 04:29:19 +0200 Nick Piggin <[email protected]> wrote:
Hi Andrew,
Can you merge this up please? It is helpful in testing to avoid lockdep tripping over. I have it at the start of the multiple hugepage size patchset, but it doesn't strictly belong there...
--
hugetlb: fix lockdep error

Quoted 6 lines

Quoted 4 lines
Acked-by KOSAKI Motohiro <[email protected]>
Acked-by Adam Litke <[email protected]>
Acked-by Nishanth Aravamudan <[email protected]>
Signed-off-by Nick Piggin <[email protected]>
Please prefer to quote the warning/error message/trace/etc when fixing it.

OK.

Steps to reproduce: compile kernel with lockdep; run libhugetlbfs regression test suite.

Reult:
=============================================
[ INFO: possible recursive locking detected ]
2.6.26-rc4 #30
---------------------------------------------
heap-overflow/2250 is trying to acquire lock:
(&mm->page_table_lock){--..}, at: [<c0000000000cf2e8>] .copy_hugetlb_page_range+0x108/0x280

but task is already holding lock:
(&mm->page_table_lock){--..}, at: [<c0000000000cf2dc>] .copy_hugetlb_page_range+0xfc/0x280

other info that might help us debug this:
3 locks held by heap-overflow/2250:

 #0:  (&mm->mmap_sem){----}, at: [<c000000000050e44>] .dup_mm+0x134/0x410
 #1:  (&mm->mmap_sem/1){--..}, at: [<c000000000050e54>] .dup_mm+0x144/0x410
 #2:  (&mm->page_table_lock){--..}, at: [<c0000000000cf2dc>] .copy_hugetlb_page_range+0xfc/0x280

stack backtrace:
Call Trace:
[c00000003b2774e0] [c000000000010ce4] .show_stack+0x74/0x1f0 (unreliable)
[c00000003b2775a0] [c0000000003f10e0] .dump_stack+0x20/0x34
[c00000003b277620] [c0000000000889bc] .__lock_acquire+0xaac/0x1080
[c00000003b277740] [c000000000089000] .lock_acquire+0x70/0xb0
[c00000003b2777d0] [c0000000003ee15c] ._spin_lock+0x4c/0x80
[c00000003b277870] [c0000000000cf2e8] .copy_hugetlb_page_range+0x108/0x280
[c00000003b277950] [c0000000000bcaa8] .copy_page_range+0x558/0x790
[c00000003b277ac0] [c000000000050fe0] .dup_mm+0x2d0/0x410
[c00000003b277ba0] [c000000000051d24] .copy_process+0xb94/0x1020
[c00000003b277ca0] [c000000000052244] .do_fork+0x94/0x310
[c00000003b277db0] [c000000000011240] .sys_clone+0x60/0x80
[c00000003b277e30] [c0000000000078c4] .ppc_clone+0x8/0xc

Fix is the same way that mm/memory.c copy_page_range does the lockdep annotation.

Quoted 10 lines

Quoted 10 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c +0 -0
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 

Quoted 9 lines

mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 6 lines
mm/hugetlb.c +1 -1
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.

Andrew Morton

maintainer

2008-05-29 03:19 UTC #5 lore raw

Quoted 20 lines · On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <[email protected]> wrote:

Quoted 10 lines
Quoted 10 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c +0 -0
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 
Quoted 9 lines
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 6 lines
mm/hugetlb.c +1 -1
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Quoted 2 lines

Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.

I don't believe that it's possible that nobody has run that test suite with lockdep enabled at any time in the past three years.

If that's really the case then perhaps we should make the ability to recite Documentation/SubmitChecklist in three languages a prerequisite for MM development.

(sheesh)

Nick Piggin

author

2008-05-29 03:26 UTC #6 lore raw

Quoted 22 lines · Andrew Morton

Quoted 20 lines · On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <[email protected]> wrote:
Quoted 19 lines
Quoted 8 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 8 lines
mm/hugetlb.c +1 -1
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Quoted 5 lines

Quoted 2 lines
Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.
I don't believe that it's possible that nobody has run that test suite with lockdep enabled at any time in the past three years.

Could be it was ignored as a false positive?

Quoted 3 lines

If that's really the case then perhaps we should make the ability to recite Documentation/SubmitChecklist in three languages a prerequisite for MM development.

It does seem unusual, I don't know...

Would it help to have a big button in kconfig called "test your kernel patches with this", which then selects various other things?

Andrew Morton

maintainer

2008-05-29 03:35 UTC #7 lore raw

Quoted 2 lines · On Thu, 29 May 2008 05:26:58 +0200 Nick Piggin <[email protected]> wrote:

Would it help to have a big button in kconfig called "test your kernel patches with this", which then selects various other things?

Sigh. Maybe. A big stick to whack people with would be nice too.

It would be good to have some mechanism to detect the kernel version within Kconfig. So we could at least do things in Kconfig which make it really really really hard to disable debug features if CONFIG_RC_KERNEL=y.

Nish Aravamudan

2008-05-29 03:48 UTC #8 lore raw

In reply to Andrew Morton

Quoted 22 lines · Andrew Morton

Quoted 20 lines · On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <[email protected]> wrote:
Quoted 19 lines
Quoted 8 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 8 lines
mm/hugetlb.c +1 -1
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
                  continue;
 
          spin_lock(&dst->page_table_lock);
-         spin_lock(&src->page_table_lock);
+         spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
          if (!huge_pte_none(huge_ptep_get(src_pte))) {
                  if (cow)
                          huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Quoted 6 lines

Quoted 2 lines
Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.
I don't believe that it's possible that nobody has run that test suite
with lockdep enabled at any time in the past three years.

I can't tell from Nick's mail if the lockdep error is specific to this particular testcase or not, but if so, that would make it the past two (almost three) months, as that was when this particular testcase was added. And I'm not sure when we released the first development snapshot that would have included it (for non-git users, that is). In any case, I also don't know how we wouldn't have seen this issue on our systems and that's a problem. I will make a concerted effort to work with the other libhugetlbfs developers to make sure all possible debug options are on (as you've pointed out to me elsewhere/when).

Thanks,
Nish

Nick Piggin

author

2008-05-29 08:58 UTC #9 lore raw

Quoted 30 lines · Nish Aravamudan

Quoted 22 lines · Andrew Morton
Quoted 20 lines · On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <[email protected]> wrote:
Quoted 19 lines
Quoted 8 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 8 lines
mm/hugetlb.c +1 -1
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
                  continue;
 
          spin_lock(&dst->page_table_lock);
-         spin_lock(&src->page_table_lock);
+         spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
          if (!huge_pte_none(huge_ptep_get(src_pte))) {
                  if (cow)
                          huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?
Quoted 6 lines
Quoted 2 lines
Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.
I don't believe that it's possible that nobody has run that test suite
with lockdep enabled at any time in the past three years.

Quoted 7 lines

I can't tell from Nick's mail if the lockdep error is specific to this particular testcase or not, but if so, that would make it the past two (almost three) months, as that was when this particular testcase was added. And I'm not sure when we released the first development snapshot that would have included it (for non-git users, that is). In any case, I also don't know how we wouldn't have seen this issue on our systems and that's a problem. I will make a concerted effort to

I didn't check if other tests trigger it (lockdep I think turns off after the first message...). But I guess anything which calls fork and has a non shared, populated hugepage pagetable entry should trigger it... Anyway the good news is that no other lockdep problems ever came up after this false positive was fixed, so not much harm done ;)

Adam Litke

2008-05-29 12:35 UTC #10 lore raw

In reply to Andrew Morton

Quoted 22 lines · Andrew Morton

Quoted 20 lines · On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <[email protected]> wrote:
Quoted 19 lines
Quoted 8 lines
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/hugetlb.c
===================================================================
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_st · line 791
Quoted 8 lines
mm/hugetlb.c +1 -1
@@ -785,7 +785,7 @@ int copy_hugetlb_page_range(struct mm_st
 			continue;
 
 		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
+		spin_lock_nested(&src->page_table_lock, SINGLE_DEPTH_NESTING);
 		if (!huge_pte_none(huge_ptep_get(src_pte))) {
 			if (cow)
 				huge_ptep_set_wrprotect(src, addr, src_pte);
Confused. This code has been there since October 2005. Why are we only seeing lockdep warnings now?

Quoted 5 lines

Quoted 2 lines
Can't say. Haven't looked at hugetlb code or tested it much until now. I am using a recent libhugetlbfs test suite, FWIW.
I don't believe that it's possible that nobody has run that test suite with lockdep enabled at any time in the past three years.

I have to confess that I have seen this from time to time. Since it was clearly a false positive, it was easy to get distracted by other things. I'll go and update all my default kernel configs to turn on everything all the time so things like this annoy me until I fix them.

Signature · 8 lines

Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>