[PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[KOSAKI Motohiro]

Commit cf40bd16fd (lockdep: annotate reclaim context) introduced reclaim
context annotation. But it didn't annotate zone reclaim. This patch do it.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/vmscan.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2bbee91..a039e78 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2547,6 +2547,7 @@ static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order)
 	 * and RECLAIM_SWAP.
 	 */
 	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
+	lockdep_set_current_reclaim_state(gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
@@ -2590,6 +2591,7 @@ static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order)
 
 	p->reclaim_state = NULL;
 	current->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE);
+	lockdep_clear_current_reclaim_state();
 	return sc.nr_reclaimed >= nr_pages;
 }
 
-- 
1.6.6

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[Peter Zijlstra]

On Fri, 2010-01-01 at 18:45 +0900, KOSAKI Motohiro wrote:
> Commit cf40bd16fd (lockdep: annotate reclaim context) introduced reclaim
> context annotation. But it didn't annotate zone reclaim. This patch do it.

And yet you didn't CC anyone involved in that patch, nor explain why you
think it necessary, massive FAIL.

The lockdep annotations cover all of kswapd() and direct reclaim through
__alloc_pages_direct_reclaim(). So why would you need an explicit
annotation in __zone_reclaim()?

> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> ---
>  mm/vmscan.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2bbee91..a039e78 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2547,6 +2547,7 @@ static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order)
>  	 * and RECLAIM_SWAP.
>  	 */
>  	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
> +	lockdep_set_current_reclaim_state(gfp_mask);
>  	reclaim_state.reclaimed_slab = 0;
>  	p->reclaim_state = &reclaim_state;
>  
> @@ -2590,6 +2591,7 @@ static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order)
>  
>  	p->reclaim_state = NULL;
>  	current->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE);
> +	lockdep_clear_current_reclaim_state();
>  	return sc.nr_reclaimed >= nr_pages;
>  }
>  

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[KOSAKI Motohiro]

2010/1/2 Peter Zijlstra <peterz@infradead.org>:
> On Fri, 2010-01-01 at 18:45 +0900, KOSAKI Motohiro wrote:
>> Commit cf40bd16fd (lockdep: annotate reclaim context) introduced reclaim
>> context annotation. But it didn't annotate zone reclaim. This patch do it.
>
> And yet you didn't CC anyone involved in that patch, nor explain why you
> think it necessary, massive FAIL.
>
> The lockdep annotations cover all of kswapd() and direct reclaim through
> __alloc_pages_direct_reclaim(). So why would you need an explicit
> annotation in __zone_reclaim()?

Thanks CCing. The point is zone-reclaim doesn't use
__alloc_pages_direct_reclaim.
current call graph is

__alloc_pages_nodemask
    get_page_from_freelist
        zone_reclaim()
    __alloc_pages_slowpath
        __alloc_pages_direct_reclaim
            try_to_free_pages

Actually, if zone_reclaim_mode=1, VM never call
__alloc_pages_direct_reclaim in usual VM pressure.
Thus I think zone-reclaim should be annotated explicitly too.
I know almost user don't use zone reclaim mode. but explicit
annotation doesn't have any demerit, I think.

Am I missing anything?

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[Peter Zijlstra]

On Sat, 2010-01-02 at 14:21 +0900, KOSAKI Motohiro wrote:
> 2010/1/2 Peter Zijlstra <peterz@infradead.org>:
> > On Fri, 2010-01-01 at 18:45 +0900, KOSAKI Motohiro wrote:
> >> Commit cf40bd16fd (lockdep: annotate reclaim context) introduced reclaim
> >> context annotation. But it didn't annotate zone reclaim. This patch do it.
> >
> > And yet you didn't CC anyone involved in that patch, nor explain why you
> > think it necessary, massive FAIL.
> >
> > The lockdep annotations cover all of kswapd() and direct reclaim through
> > __alloc_pages_direct_reclaim(). So why would you need an explicit
> > annotation in __zone_reclaim()?
> 
> Thanks CCing. The point is zone-reclaim doesn't use
> __alloc_pages_direct_reclaim.
> current call graph is
> 
> __alloc_pages_nodemask
>     get_page_from_freelist
>         zone_reclaim()
>     __alloc_pages_slowpath
>         __alloc_pages_direct_reclaim
>             try_to_free_pages
> 
> Actually, if zone_reclaim_mode=1, VM never call
> __alloc_pages_direct_reclaim in usual VM pressure.
> Thus I think zone-reclaim should be annotated explicitly too.
> I know almost user don't use zone reclaim mode. but explicit
> annotation doesn't have any demerit, I think.

Just be aware that the annotation isn't recursive, I'd have to trace all
calls to __zone_reclaim, but if kswapd were ever to call it you'd just
wrecked things by getting lockdep_clear_current_reclaim_state() called.

So just make sure you don't shorten the existing notations by adding it
here. Other than that it seems ok.

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[KOSAKI Motohiro]

2010/1/2 Peter Zijlstra <peterz@infradead.org>:
> On Sat, 2010-01-02 at 14:21 +0900, KOSAKI Motohiro wrote:
>> 2010/1/2 Peter Zijlstra <peterz@infradead.org>:
>> > On Fri, 2010-01-01 at 18:45 +0900, KOSAKI Motohiro wrote:
>> >> Commit cf40bd16fd (lockdep: annotate reclaim context) introduced reclaim
>> >> context annotation. But it didn't annotate zone reclaim. This patch do it.
>> >
>> > And yet you didn't CC anyone involved in that patch, nor explain why you
>> > think it necessary, massive FAIL.
>> >
>> > The lockdep annotations cover all of kswapd() and direct reclaim through
>> > __alloc_pages_direct_reclaim(). So why would you need an explicit
>> > annotation in __zone_reclaim()?
>>
>> Thanks CCing. The point is zone-reclaim doesn't use
>> __alloc_pages_direct_reclaim.
>> current call graph is
>>
>> __alloc_pages_nodemask
>>     get_page_from_freelist
>>         zone_reclaim()
>>     __alloc_pages_slowpath
>>         __alloc_pages_direct_reclaim
>>             try_to_free_pages
>>
>> Actually, if zone_reclaim_mode=1, VM never call
>> __alloc_pages_direct_reclaim in usual VM pressure.
>> Thus I think zone-reclaim should be annotated explicitly too.
>> I know almost user don't use zone reclaim mode. but explicit
>> annotation doesn't have any demerit, I think.
>
> Just be aware that the annotation isn't recursive, I'd have to trace all
> calls to __zone_reclaim, but if kswapd were ever to call it you'd just
> wrecked things by getting lockdep_clear_current_reclaim_state() called.
>
> So just make sure you don't shorten the existing notations by adding it
> here. Other than that it seems ok.

Umm, probably I haven't catch your mention. currently kswapd never
call __zone_reclaim() because
kswapd has PF_MEMALLOC and PF_MEMALLOC prevent to call __zone_reclaim
(see zone_reclaim()).

When recursive annotation occur?

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[Peter Zijlstra]

On Sat, 2010-01-02 at 22:29 +0900, KOSAKI Motohiro wrote:

> When recursive annotation occur?

Dunno, I told you you'd have to make sure it doesn't.

Re: [PATCH] mm, lockdep: annotate reclaim context to zone reclaim too

[KOSAKI Motohiro]

2010/1/2 Peter Zijlstra <peterz@infradead.org>:
> On Sat, 2010-01-02 at 22:29 +0900, KOSAKI Motohiro wrote:
>
>> When recursive annotation occur?
>
> Dunno, I told you you'd have to make sure it doesn't.

Please see PF_MEMALLOC turning on/off operation points. all
PF_MEMALLOC turing on point prevent recersive already.
(because, otherwise we lost PF_MEMALLOC and makes deadlock...)