[PATCH] mm : add check for the return value

[PATCH] mm : add check for the return value

[Huang Shijie]

When the `page' returned by __rmqueue() is NULL, the origin code
still adds -(1 << order) to zone's NR_FREE_PAGES item.

The patch fixes it.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 mm/page_alloc.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4e9f5cc..620921d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1222,10 +1222,14 @@ again:
 		}
 		spin_lock_irqsave(&zone->lock, flags);
 		page = __rmqueue(zone, order, migratetype);
-		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
-		spin_unlock(&zone->lock);
-		if (!page)
+		if (likely(page)) {
+			__mod_zone_page_state(zone, NR_FREE_PAGES,
+						-(1 << order));
+			spin_unlock(&zone->lock);
+		} else {
+			spin_unlock(&zone->lock);
 			goto failed;
+		}
 	}
 
 	__count_zone_vm_events(PGALLOC, zone, 1 << order);
-- 
1.6.5.2

--
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] mm : add check for the return value

[Minchan Kim]

Hi, Huang. 

On Mon,  4 Jan 2010 10:22:10 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> When the `page' returned by __rmqueue() is NULL, the origin code
> still adds -(1 << order) to zone's NR_FREE_PAGES item.
> 
> The patch fixes it.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> ---
>  mm/page_alloc.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4e9f5cc..620921d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1222,10 +1222,14 @@ again:
>  		}
>  		spin_lock_irqsave(&zone->lock, flags);
>  		page = __rmqueue(zone, order, migratetype);
> -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> -		spin_unlock(&zone->lock);
> -		if (!page)
> +		if (likely(page)) {
> +			__mod_zone_page_state(zone, NR_FREE_PAGES,
> +						-(1 << order));
> +			spin_unlock(&zone->lock);
> +		} else {
> +			spin_unlock(&zone->lock);
>  			goto failed;
> +		}
>  	}
>  
>  	__count_zone_vm_events(PGALLOC, zone, 1 << order);

I think it's not desirable to add new branch in hot-path even though
we could avoid that. 

How about this?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4e4b5b3..87976ad 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1244,6 +1244,9 @@ again:
        return page;
 
 failed:
+       spin_lock(&zone->lock);
+       __mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
+       spin_unlock(&zone->lock);
        local_irq_restore(flags);
        put_cpu();
        return NULL;




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


-- 
Kind regards,
Minchan Kim

--
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] mm : add check for the return value

[shijie8]

> I think it's not desirable to add new branch in hot-path even though
> we could avoid that.
>
> How about this?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4e4b5b3..87976ad 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1244,6 +1244,9 @@ again:
>          return page;
>
>   failed:
>    
you miss anther place where also  uses "goto failed".
> +       spin_lock(&zone->lock);
> +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1<<  order);
> +       spin_unlock(&zone->lock);
>          local_irq_restore(flags);
>          put_cpu();
>          return NULL;
>
>    
I also thought  over your method before I sent the patch,  but there 
already exits a
"if (!page)" , I not sure whether my patch adds too much delay in hot-path.

--
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] mm : add check for the return value

[Minchan Kim]

On Mon, 04 Jan 2010 12:10:16 +0800
shijie8 <shijie8@gmail.com> wrote:

> 
> > I think it's not desirable to add new branch in hot-path even though
> > we could avoid that.
> >
> > How about this?
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 4e4b5b3..87976ad 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1244,6 +1244,9 @@ again:
> >          return page;
> >
> >   failed:
> >    
> you miss anther place where also  uses "goto failed".

Yes. It was just for showing my intention. :)

> > +       spin_lock(&zone->lock);
> > +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1<<  order);
> > +       spin_unlock(&zone->lock);
> >          local_irq_restore(flags);
> >          put_cpu();
> >          return NULL;
> >
> >    
> I also thought  over your method before I sent the patch,  but there 
> already exits a
> "if (!page)" , I not sure whether my patch adds too much delay in hot-path.

Tend to agree. I don't object your patch. 

I think the branch itself could not a big deal but 'likely'. 

Why I suggest is that now 'if (!page)' don't have 'likely'.
As you know, 'likely' make the code relocate for reducing code footprint.

Why? It was just mistake or doesn't need it? 

I think Mel does know it. 

-- 
Kind regards,
Minchan Kim

--
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] mm : add check for the return value

[Huang Shijie]

> I think the branch itself could not a big deal but 'likely'.
>
> Why I suggest is that now 'if (!page)' don't have 'likely'.
> As you know, 'likely' make the code relocate for reducing code footprint.
>
> Why? It was just mistake or doesn't need it?
>
>    
I think the CPU will CACHE the `likely' code, and make it runs fast.

IMHO, "if (unlikely(page == NULL)) " is better then "if (!page)" ,just 
like the
code in rmqueue_bulk().

> I think Mel does know it.
>
>    
wait for Mel's response.

--
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] mm : add check for the return value

[Minchan Kim]

On Mon, Jan 4, 2010 at 2:18 PM, Huang Shijie <shijie8@gmail.com> wrote:
>
>> I think the branch itself could not a big deal but 'likely'.
>>
>> Why I suggest is that now 'if (!page)' don't have 'likely'.
>> As you know, 'likely' make the code relocate for reducing code footprint.
>>
>> Why? It was just mistake or doesn't need it?
>>
>>
>
> I think the CPU will CACHE the `likely' code, and make it runs fast.

I think so.

>
> IMHO, "if (unlikely(page == NULL)) " is better then "if (!page)" ,just like
> the
> code in rmqueue_bulk().
>> I think Mel does know it.
>>
>>
>
> wait for Mel's response.

Yes.
Regardless of Kosaki's patch, there is a issue about likely/unlinkely usage.

>



-- 
Kind regards,
Minchan Kim

--
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] mm : add check for the return value

[Andrew Morton]

On Mon, 4 Jan 2010 15:06:54 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:

> On Mon, Jan 4, 2010 at 2:18 PM, Huang Shijie <shijie8@gmail.com> wrote:
> >
> >> I think the branch itself could not a big deal but 'likely'.
> >>
> >> Why I suggest is that now 'if (!page)' don't have 'likely'.
> >> As you know, 'likely' make the code relocate for reducing code footprint.
> >>
> >> Why? It was just mistake or doesn't need it?
> >>
> >>
> >
> > I think the CPU will CACHE the `likely' code, and make it runs fast.
> 
> I think so.
> 
> >
> > IMHO, "if (unlikely(page == NULL)) " is better then "if (!page)" ,just like
> > the
> > code in rmqueue_bulk().
> >> I think Mel does know it.
> >>
> >>
> >
> > wait for Mel's response.
> 
> Yes.
> Regardless of Kosaki's patch, there is a issue about likely/unlinkely usage.
> 

All of this code is in the (order != 0) path, so it's relatively rarely
executed.  We've added a small expense to a rarely-executed code
path.  I think I'll apply the original patch as-is.


From: Huang Shijie <shijie8@gmail.com>

When the `page' returned by __rmqueue() is NULL, the origin code still
adds -(1 << order) to zone's NR_FREE_PAGES item.

The patch fixes it.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page_alloc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff -puN mm/page_alloc.c~mm-add-check-for-the-return-value mm/page_alloc.c
--- a/mm/page_alloc.c~mm-add-check-for-the-return-value
+++ a/mm/page_alloc.c
@@ -1219,10 +1219,14 @@ again:
 		}
 		spin_lock_irqsave(&zone->lock, flags);
 		page = __rmqueue(zone, order, migratetype);
-		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
-		spin_unlock(&zone->lock);
-		if (!page)
+		if (likely(page)) {
+			__mod_zone_page_state(zone, NR_FREE_PAGES,
+						-(1 << order));
+			spin_unlock(&zone->lock);
+		} else {
+			spin_unlock(&zone->lock);
 			goto failed;
+		}
 	}
 
 	__count_zone_vm_events(PGALLOC, zone, 1 << order);
_

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

[PATCH] page allocator: fix update NR_FREE_PAGES only as necessary

[KOSAKI Motohiro]

> Hi, Huang. 
> 
> On Mon,  4 Jan 2010 10:22:10 +0800
> Huang Shijie <shijie8@gmail.com> wrote:
> 
> > When the `page' returned by __rmqueue() is NULL, the origin code
> > still adds -(1 << order) to zone's NR_FREE_PAGES item.
> > 
> > The patch fixes it.
> > 
> > Signed-off-by: Huang Shijie <shijie8@gmail.com>
> > ---
> >  mm/page_alloc.c |   10 +++++++---
> >  1 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 4e9f5cc..620921d 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1222,10 +1222,14 @@ again:
> >  		}
> >  		spin_lock_irqsave(&zone->lock, flags);
> >  		page = __rmqueue(zone, order, migratetype);
> > -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> > -		spin_unlock(&zone->lock);
> > -		if (!page)
> > +		if (likely(page)) {
> > +			__mod_zone_page_state(zone, NR_FREE_PAGES,
> > +						-(1 << order));
> > +			spin_unlock(&zone->lock);
> > +		} else {
> > +			spin_unlock(&zone->lock);
> >  			goto failed;
> > +		}
> >  	}
> >  
> >  	__count_zone_vm_events(PGALLOC, zone, 1 << order);
> 
> I think it's not desirable to add new branch in hot-path even though
> we could avoid that. 
> 
> How about this?
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4e4b5b3..87976ad 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1244,6 +1244,9 @@ again:
>         return page;
>  
>  failed:
> +       spin_lock(&zone->lock);
> +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
> +       spin_unlock(&zone->lock);
>         local_irq_restore(flags);
>         put_cpu();
>         return NULL;

Why can't we write following? __mod_zone_page_state() only require irq
disabling, it doesn't need spin lock. I think.



From 72011ff2b0bba6544ae35c6ee52715c8c824a34b Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Mon, 4 Jan 2010 14:38:20 +0900
Subject: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary

commit f2260e6b (page allocator: update NR_FREE_PAGES only as necessary)
made one minor regression.
if __rmqueue() was failed, NR_FREE_PAGES stat go wrong. this patch fixes
it.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Huang Shijie <shijie8@gmail.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 11ae66e..ecf75a1 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1227,10 +1227,10 @@ again:
 		}
 		spin_lock_irqsave(&zone->lock, flags);
 		page = __rmqueue(zone, order, migratetype);
-		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
 		spin_unlock(&zone->lock);
 		if (!page)
 			goto failed;
+		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
 	}
 
 	__count_zone_vm_events(PGALLOC, zone, 1 << order);
-- 
1.6.5.2




--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Minchan Kim]

On Mon,  4 Jan 2010 14:52:36 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > Hi, Huang. 
> > 
> > On Mon,  4 Jan 2010 10:22:10 +0800
> > Huang Shijie <shijie8@gmail.com> wrote:
> > 
> > > When the `page' returned by __rmqueue() is NULL, the origin code
> > > still adds -(1 << order) to zone's NR_FREE_PAGES item.
> > > 
> > > The patch fixes it.
> > > 
> > > Signed-off-by: Huang Shijie <shijie8@gmail.com>
> > > ---
> > >  mm/page_alloc.c |   10 +++++++---
> > >  1 files changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 4e9f5cc..620921d 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1222,10 +1222,14 @@ again:
> > >  		}
> > >  		spin_lock_irqsave(&zone->lock, flags);
> > >  		page = __rmqueue(zone, order, migratetype);
> > > -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> > > -		spin_unlock(&zone->lock);
> > > -		if (!page)
> > > +		if (likely(page)) {
> > > +			__mod_zone_page_state(zone, NR_FREE_PAGES,
> > > +						-(1 << order));
> > > +			spin_unlock(&zone->lock);
> > > +		} else {
> > > +			spin_unlock(&zone->lock);
> > >  			goto failed;
> > > +		}
> > >  	}
> > >  
> > >  	__count_zone_vm_events(PGALLOC, zone, 1 << order);
> > 
> > I think it's not desirable to add new branch in hot-path even though
> > we could avoid that. 
> > 
> > How about this?
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 4e4b5b3..87976ad 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1244,6 +1244,9 @@ again:
> >         return page;
> >  
> >  failed:
> > +       spin_lock(&zone->lock);
> > +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
> > +       spin_unlock(&zone->lock);
> >         local_irq_restore(flags);
> >         put_cpu();
> >         return NULL;
> 
> Why can't we write following? __mod_zone_page_state() only require irq
> disabling, it doesn't need spin lock. I think.

That's true. I missed that :)

> 
> 
> 
> From 72011ff2b0bba6544ae35c6ee52715c8c824a34b Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date: Mon, 4 Jan 2010 14:38:20 +0900
> Subject: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
> 
> commit f2260e6b (page allocator: update NR_FREE_PAGES only as necessary)
> made one minor regression.
> if __rmqueue() was failed, NR_FREE_PAGES stat go wrong. this patch fixes
> it.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Huang Shijie <shijie8@gmail.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
Reviewed-by : Minchan Kim <minchan.kim@gmail.com>

-- 
Kind regards,
Minchan Kim

--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Huang Shijie]

> Why can't we write following? __mod_zone_page_state() only require irq
> disabling, it doesn't need spin lock. I think.
>
>
>
struct per_cpu_pageset {
  .................................................
#ifdef CONFIG_SMP
     s8 stat_threshold;
     s8 vm_stat_diff[NR_VM_ZONE_STAT_ITEMS];
#endif
} ____cacheline_aligned_in_smp;

The field 'stat_threshold' is in the CONFIG_SMP macro, does it not need 
the spinlock? I will read the code more carefully.
I saw the macro, so I thought it need the spinlock. :)


--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[KOSAKI Motohiro]

> 
> > Why can't we write following? __mod_zone_page_state() only require irq
> > disabling, it doesn't need spin lock. I think.
> >
> >
> >
> struct per_cpu_pageset {
>   .................................................
> #ifdef CONFIG_SMP
>      s8 stat_threshold;
>      s8 vm_stat_diff[NR_VM_ZONE_STAT_ITEMS];
> #endif
> } ____cacheline_aligned_in_smp;
> 
> The field 'stat_threshold' is in the CONFIG_SMP macro, does it not need 
> the spinlock? I will read the code more carefully.
> I saw the macro, so I thought it need the spinlock. :)

Generally,  per-cpu data isn't accessed from another cpu. it only need to care
process-context vs irq-context race.



--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Huang Shijie]

>
>> struct per_cpu_pageset {
>>    .................................................
>> #ifdef CONFIG_SMP
>>       s8 stat_threshold;
>>       s8 vm_stat_diff[NR_VM_ZONE_STAT_ITEMS];
>> #endif
>> } ____cacheline_aligned_in_smp;
>>
>> The field 'stat_threshold' is in the CONFIG_SMP macro, does it not need
>> the spinlock? I will read the code more carefully.
>> I saw the macro, so I thought it need the spinlock. :)
>>      
> Generally,  per-cpu data isn't accessed from another cpu. it only need to care
> process-context vs irq-context race.
>
>
>    
If the  __mod_zone_page_state() can be used without caring about the 
spinlock, I think there
are several places we can move __mod_zone_page_state() out the guard 
area of spinlock to
release the pressure of the zone->lock,such as in rmqueue_bulk().





--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[KOSAKI Motohiro]

> >
> >> struct per_cpu_pageset {
> >>    .................................................
> >> #ifdef CONFIG_SMP
> >>       s8 stat_threshold;
> >>       s8 vm_stat_diff[NR_VM_ZONE_STAT_ITEMS];
> >> #endif
> >> } ____cacheline_aligned_in_smp;
> >>
> >> The field 'stat_threshold' is in the CONFIG_SMP macro, does it not need
> >> the spinlock? I will read the code more carefully.
> >> I saw the macro, so I thought it need the spinlock. :)
> >>      
> > Generally,  per-cpu data isn't accessed from another cpu. it only need to care
> > process-context vs irq-context race.
> >    
> If the  __mod_zone_page_state() can be used without caring about the 
> spinlock, I think there
> are several places we can move __mod_zone_page_state() out the guard 
> area of spinlock to
> release the pressure of the zone->lock,such as in rmqueue_bulk().

Welcome to your patch :)



--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Mel Gorman]

On Mon, Jan 04, 2010 at 02:52:36PM +0900, KOSAKI Motohiro wrote:
> > Hi, Huang. 
> > 
> > On Mon,  4 Jan 2010 10:22:10 +0800
> > Huang Shijie <shijie8@gmail.com> wrote:
> > 
> > > When the `page' returned by __rmqueue() is NULL, the origin code
> > > still adds -(1 << order) to zone's NR_FREE_PAGES item.
> > > 
> > > The patch fixes it.
> > > 
> > > Signed-off-by: Huang Shijie <shijie8@gmail.com>
> > > ---
> > >  mm/page_alloc.c |   10 +++++++---
> > >  1 files changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 4e9f5cc..620921d 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1222,10 +1222,14 @@ again:
> > >  		}
> > >  		spin_lock_irqsave(&zone->lock, flags);
> > >  		page = __rmqueue(zone, order, migratetype);
> > > -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> > > -		spin_unlock(&zone->lock);
> > > -		if (!page)
> > > +		if (likely(page)) {
> > > +			__mod_zone_page_state(zone, NR_FREE_PAGES,
> > > +						-(1 << order));
> > > +			spin_unlock(&zone->lock);
> > > +		} else {
> > > +			spin_unlock(&zone->lock);
> > >  			goto failed;
> > > +		}
> > >  	}
> > >  
> > >  	__count_zone_vm_events(PGALLOC, zone, 1 << order);
> > 
> > I think it's not desirable to add new branch in hot-path even though
> > we could avoid that. 
> > 
> > How about this?
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 4e4b5b3..87976ad 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1244,6 +1244,9 @@ again:
> >         return page;
> >  
> >  failed:
> > +       spin_lock(&zone->lock);
> > +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
> > +       spin_unlock(&zone->lock);
> >         local_irq_restore(flags);
> >         put_cpu();
> >         return NULL;
> 
> Why can't we write following? __mod_zone_page_state() only require irq
> disabling, it doesn't need spin lock. I think.
> 

Adding Christoph to be sure but yes, as this is a per-cpu variable it
should be safe to update with __mod_zone_page_state() as long as
interrupts and preempt are disabled. If true, then this is a neater fix
and is also needed for -stable 2.6.31 and 2.6.32.

Well spotted and thanks.

> From 72011ff2b0bba6544ae35c6ee52715c8c824a34b Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date: Mon, 4 Jan 2010 14:38:20 +0900
> Subject: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
> 
> commit f2260e6b (page allocator: update NR_FREE_PAGES only as necessary)
> made one minor regression.
> if __rmqueue() was failed, NR_FREE_PAGES stat go wrong. this patch fixes
> it.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Huang Shijie <shijie8@gmail.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/page_alloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 11ae66e..ecf75a1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1227,10 +1227,10 @@ again:
>  		}
>  		spin_lock_irqsave(&zone->lock, flags);
>  		page = __rmqueue(zone, order, migratetype);
> -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
>  		spin_unlock(&zone->lock);
>  		if (!page)
>  			goto failed;
> +		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
>  	}
>  
>  	__count_zone_vm_events(PGALLOC, zone, 1 << order);
> -- 
> 1.6.5.2
> 
> 
> 
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Christoph Lameter]

On Mon, 4 Jan 2010, Mel Gorman wrote:

> > Why can't we write following? __mod_zone_page_state() only require irq
> > disabling, it doesn't need spin lock. I think.

Correct.

> > commit f2260e6b (page allocator: update NR_FREE_PAGES only as necessary)
> > made one minor regression.
> > if __rmqueue() was failed, NR_FREE_PAGES stat go wrong. this patch fixes
> > it.
> >
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Mel Gorman <mel@csn.ul.ie>
> > Cc: Huang Shijie <shijie8@gmail.com>
> > Cc: Minchan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/page_alloc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 11ae66e..ecf75a1 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1227,10 +1227,10 @@ again:
> >  		}
> >  		spin_lock_irqsave(&zone->lock, flags);
> >  		page = __rmqueue(zone, order, migratetype);
> > -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> >  		spin_unlock(&zone->lock);
> >  		if (!page)
> >  			goto failed;
> > +		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
> >  	}
> >
> >  	__count_zone_vm_events(PGALLOC, zone, 1 << order);

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[KOSAKI Motohiro]

> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 4e4b5b3..87976ad 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1244,6 +1244,9 @@ again:
> > >         return page;
> > >  
> > >  failed:
> > > +       spin_lock(&zone->lock);
> > > +       __mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
> > > +       spin_unlock(&zone->lock);
> > >         local_irq_restore(flags);
> > >         put_cpu();
> > >         return NULL;
> > 
> > Why can't we write following? __mod_zone_page_state() only require irq
> > disabling, it doesn't need spin lock. I think.
> > 
> 
> Adding Christoph to be sure but yes, as this is a per-cpu variable it
> should be safe to update with __mod_zone_page_state() as long as
> interrupts and preempt are disabled. If true, then this is a neater fix
> and is also needed for -stable 2.6.31 and 2.6.32.
> 
> Well spotted and thanks.

Yes, it should be sent to -stable tree. I hope this fix also solve recent mysterious
allocation failure problem ;-)



--
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] page allocator: fix update NR_FREE_PAGES only as necessary

[Andrew Morton]

On Mon,  4 Jan 2010 14:52:36 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1227,10 +1227,10 @@ again:
>  		}
>  		spin_lock_irqsave(&zone->lock, flags);
>  		page = __rmqueue(zone, order, migratetype);
> -		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
>  		spin_unlock(&zone->lock);
>  		if (!page)
>  			goto failed;
> +		__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << order));
>  	}
>  
>  	__count_zone_vm_events(PGALLOC, zone, 1 << order);

hm, yes, OK, obviously better.

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