[patch] hugetlb: fix lockdep error

[patch] hugetlb: fix lockdep error

[Nick Piggin]

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 <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Adam Litke <agl@us.ibm.com>
Acked-by: Nishanth Aravamudan <nacc@us.ibm.com>
Signed-off-by: Nick Piggin <npiggin@suse.de>
---
 mm/hugetlb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/mm/hugetlb.c
===================================================================
--- linux-2.6.orig/mm/hugetlb.c
+++ 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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix lockdep error

[Andrew Morton]

On Thu, 29 May 2008 04:29:19 +0200 Nick Piggin <npiggin@suse.de> 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
> 
> Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Acked-by: Adam Litke <agl@us.ibm.com>
> Acked-by: Nishanth Aravamudan <nacc@us.ibm.com>
> Signed-off-by: Nick Piggin <npiggin@suse.de>

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

>  mm/hugetlb.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/mm/hugetlb.c
> ===================================================================
> --- linux-2.6.orig/mm/hugetlb.c
> +++ 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);

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

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

Re: [patch] hugetlb: fix lockdep error

[Nick Piggin]

On Wed, May 28, 2008 at 07:38:08PM -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 04:29:19 +0200 Nick Piggin <npiggin@suse.de> 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
> > 
> > Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Acked-by: Adam Litke <agl@us.ibm.com>
> > Acked-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> 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.

 
> >  mm/hugetlb.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-2.6/mm/hugetlb.c
> > ===================================================================
> > --- linux-2.6.orig/mm/hugetlb.c
> > +++ 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);
> 
> 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.

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

Re: [patch] hugetlb: fix lockdep error

[Andrew Morton]

On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <npiggin@suse.de> wrote:

> > >  mm/hugetlb.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/mm/hugetlb.c
> > > ===================================================================
> > > --- linux-2.6.orig/mm/hugetlb.c
> > > +++ 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);
> > 
> > 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.

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)

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

Re: [patch] hugetlb: fix lockdep error

[Nick Piggin]

On Wed, May 28, 2008 at 08:19:29PM -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <npiggin@suse.de> wrote:
> 
> > > >  mm/hugetlb.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/mm/hugetlb.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/mm/hugetlb.c
> > > > +++ 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);
> > > 
> > > 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.
> 
> 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?

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

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

Re: [patch] hugetlb: fix lockdep error

[Andrew Morton]

On Thu, 29 May 2008 05:26:58 +0200 Nick Piggin <npiggin@suse.de> 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.

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

Re: [patch] hugetlb: fix lockdep error

[Nish Aravamudan]

On 5/28/08, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <npiggin@suse.de> wrote:
>
>  > > >  mm/hugetlb.c |    2 +-
>  > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>  > > >
>  > > > Index: linux-2.6/mm/hugetlb.c
>  > > > ===================================================================
>  > > > --- linux-2.6.orig/mm/hugetlb.c
>  > > > +++ 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);
>  > >
>  > > 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.
>
>
> 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

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

Re: [patch] hugetlb: fix lockdep error

[Nick Piggin]

On Wed, May 28, 2008 at 08:48:36PM -0700, Nish Aravamudan wrote:
> On 5/28/08, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <npiggin@suse.de> wrote:
> >
> >  > > >  mm/hugetlb.c |    2 +-
> >  > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >  > > >
> >  > > > Index: linux-2.6/mm/hugetlb.c
> >  > > > ===================================================================
> >  > > > --- linux-2.6.orig/mm/hugetlb.c
> >  > > > +++ 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);
> >  > >
> >  > > 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.
> >
> >
> > 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

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 ;)

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

Re: [patch] hugetlb: fix lockdep error

[Adam Litke]

On Wed, 2008-05-28 at 20:19 -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 05:07:45 +0200 Nick Piggin <npiggin@suse.de> wrote:
> 
> > > >  mm/hugetlb.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/mm/hugetlb.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/mm/hugetlb.c
> > > > +++ 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);
> > > 
> > > 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.
> 
> 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.

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

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