* [PATCH] mm : add check for the return value
@ 2010-01-04 2:22 Huang Shijie
2010-01-04 3:21 ` Minchan Kim
0 siblings, 1 reply; 17+ messages in thread
From: Huang Shijie @ 2010-01-04 2:22 UTC (permalink / raw)
To: akpm; +Cc: mel, linux-mm, 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>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 2:22 [PATCH] mm : add check for the return value Huang Shijie
@ 2010-01-04 3:21 ` Minchan Kim
2010-01-04 4:10 ` shijie8
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
0 siblings, 2 replies; 17+ messages in thread
From: Minchan Kim @ 2010-01-04 3:21 UTC (permalink / raw)
To: Huang Shijie; +Cc: akpm, mel, linux-mm
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>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 3:21 ` Minchan Kim
@ 2010-01-04 4:10 ` shijie8
2010-01-04 4:48 ` Minchan Kim
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
1 sibling, 1 reply; 17+ messages in thread
From: shijie8 @ 2010-01-04 4:10 UTC (permalink / raw)
To: Minchan Kim; +Cc: akpm, mel, linux-mm
> 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 4:10 ` shijie8
@ 2010-01-04 4:48 ` Minchan Kim
2010-01-04 5:18 ` Huang Shijie
0 siblings, 1 reply; 17+ messages in thread
From: Minchan Kim @ 2010-01-04 4:48 UTC (permalink / raw)
To: shijie8; +Cc: Minchan Kim, akpm, mel, linux-mm
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 4:48 ` Minchan Kim
@ 2010-01-04 5:18 ` Huang Shijie
2010-01-04 6:06 ` Minchan Kim
0 siblings, 1 reply; 17+ messages in thread
From: Huang Shijie @ 2010-01-04 5:18 UTC (permalink / raw)
To: Minchan Kim; +Cc: akpm, mel, linux-mm
> 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 3:21 ` Minchan Kim
2010-01-04 4:10 ` shijie8
@ 2010-01-04 5:52 ` KOSAKI Motohiro
2010-01-04 6:03 ` Minchan Kim
` (3 more replies)
1 sibling, 4 replies; 17+ messages in thread
From: KOSAKI Motohiro @ 2010-01-04 5:52 UTC (permalink / raw)
To: Minchan Kim; +Cc: kosaki.motohiro, Huang Shijie, akpm, mel, linux-mm
> 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>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
@ 2010-01-04 6:03 ` Minchan Kim
2010-01-04 6:11 ` Huang Shijie
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Minchan Kim @ 2010-01-04 6:03 UTC (permalink / raw)
To: KOSAKI Motohiro; +Cc: Minchan Kim, Huang Shijie, akpm, mel, linux-mm
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 5:18 ` Huang Shijie
@ 2010-01-04 6:06 ` Minchan Kim
2010-01-08 22:59 ` Andrew Morton
0 siblings, 1 reply; 17+ messages in thread
From: Minchan Kim @ 2010-01-04 6:06 UTC (permalink / raw)
To: Huang Shijie; +Cc: akpm, mel, linux-mm, KOSAKI Motohiro
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
2010-01-04 6:03 ` Minchan Kim
@ 2010-01-04 6:11 ` Huang Shijie
2010-01-04 6:16 ` KOSAKI Motohiro
2010-01-04 9:58 ` Mel Gorman
2010-01-08 23:02 ` Andrew Morton
3 siblings, 1 reply; 17+ messages in thread
From: Huang Shijie @ 2010-01-04 6:11 UTC (permalink / raw)
To: KOSAKI Motohiro; +Cc: Minchan Kim, akpm, mel, linux-mm
> 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 6:11 ` Huang Shijie
@ 2010-01-04 6:16 ` KOSAKI Motohiro
2010-01-04 9:35 ` Huang Shijie
0 siblings, 1 reply; 17+ messages in thread
From: KOSAKI Motohiro @ 2010-01-04 6:16 UTC (permalink / raw)
To: Huang Shijie; +Cc: kosaki.motohiro, Minchan Kim, akpm, mel, linux-mm
>
> > 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 6:16 ` KOSAKI Motohiro
@ 2010-01-04 9:35 ` Huang Shijie
2010-01-05 7:24 ` KOSAKI Motohiro
0 siblings, 1 reply; 17+ messages in thread
From: Huang Shijie @ 2010-01-04 9:35 UTC (permalink / raw)
To: KOSAKI Motohiro; +Cc: Minchan Kim, akpm, mel, linux-mm
>
>> 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
2010-01-04 6:03 ` Minchan Kim
2010-01-04 6:11 ` Huang Shijie
@ 2010-01-04 9:58 ` Mel Gorman
2010-01-04 17:46 ` Christoph Lameter
2010-01-05 1:08 ` KOSAKI Motohiro
2010-01-08 23:02 ` Andrew Morton
3 siblings, 2 replies; 17+ messages in thread
From: Mel Gorman @ 2010-01-04 9:58 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Minchan Kim, Huang Shijie, akpm, linux-mm, Christoph Lameter
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 9:58 ` Mel Gorman
@ 2010-01-04 17:46 ` Christoph Lameter
2010-01-05 1:08 ` KOSAKI Motohiro
1 sibling, 0 replies; 17+ messages in thread
From: Christoph Lameter @ 2010-01-04 17:46 UTC (permalink / raw)
To: Mel Gorman; +Cc: KOSAKI Motohiro, Minchan Kim, Huang Shijie, akpm, linux-mm
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 9:58 ` Mel Gorman
2010-01-04 17:46 ` Christoph Lameter
@ 2010-01-05 1:08 ` KOSAKI Motohiro
1 sibling, 0 replies; 17+ messages in thread
From: KOSAKI Motohiro @ 2010-01-05 1:08 UTC (permalink / raw)
To: Mel Gorman
Cc: kosaki.motohiro, Minchan Kim, Huang Shijie, akpm, linux-mm,
Christoph Lameter
> > > 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 9:35 ` Huang Shijie
@ 2010-01-05 7:24 ` KOSAKI Motohiro
0 siblings, 0 replies; 17+ messages in thread
From: KOSAKI Motohiro @ 2010-01-05 7:24 UTC (permalink / raw)
To: Huang Shijie; +Cc: kosaki.motohiro, Minchan Kim, akpm, mel, linux-mm
> >
> >> 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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mm : add check for the return value
2010-01-04 6:06 ` Minchan Kim
@ 2010-01-08 22:59 ` Andrew Morton
0 siblings, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2010-01-08 22:59 UTC (permalink / raw)
To: Minchan Kim; +Cc: Huang Shijie, mel, linux-mm, KOSAKI Motohiro
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
` (2 preceding siblings ...)
2010-01-04 9:58 ` Mel Gorman
@ 2010-01-08 23:02 ` Andrew Morton
3 siblings, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2010-01-08 23:02 UTC (permalink / raw)
To: KOSAKI Motohiro; +Cc: Minchan Kim, Huang Shijie, mel, linux-mm
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2010-01-08 23:03 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-04 2:22 [PATCH] mm : add check for the return value Huang Shijie
2010-01-04 3:21 ` Minchan Kim
2010-01-04 4:10 ` shijie8
2010-01-04 4:48 ` Minchan Kim
2010-01-04 5:18 ` Huang Shijie
2010-01-04 6:06 ` Minchan Kim
2010-01-08 22:59 ` Andrew Morton
2010-01-04 5:52 ` [PATCH] page allocator: fix update NR_FREE_PAGES only as necessary KOSAKI Motohiro
2010-01-04 6:03 ` Minchan Kim
2010-01-04 6:11 ` Huang Shijie
2010-01-04 6:16 ` KOSAKI Motohiro
2010-01-04 9:35 ` Huang Shijie
2010-01-05 7:24 ` KOSAKI Motohiro
2010-01-04 9:58 ` Mel Gorman
2010-01-04 17:46 ` Christoph Lameter
2010-01-05 1:08 ` KOSAKI Motohiro
2010-01-08 23:02 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).