[PATCH] mm: don't invoke __alloc_pages_direct_compact when order 0

[PATCH] mm: don't invoke __alloc_pages_direct_compact when order 0

[Joonsoo Kim]

__alloc_pages_direct_compact has many arguments so invoking it is very costly.
And in almost invoking case, order is 0, so return immediately.

Let's not invoke it when order 0

Signed-off-by: Joonsoo Kim <js1304@gmail.com>

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6092f33..f4039aa 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2056,7 +2056,10 @@ out:
 }
 
 #ifdef CONFIG_COMPACTION
-/* Try memory compaction for high-order allocations before reclaim */
+/*
+ * Try memory compaction for high-order allocations before reclaim
+ * Must be called with order > 0
+ */
 static struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	struct zonelist *zonelist, enum zone_type high_zoneidx,
@@ -2067,8 +2070,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 {
 	struct page *page;
 
-	if (!order)
-		return NULL;
+	BUG_ON(!order);
 
 	if (compaction_deferred(preferred_zone, order)) {
 		*deferred_compaction = true;
@@ -2363,15 +2365,17 @@ rebalance:
 	 * Try direct compaction. The first pass is asynchronous. Subsequent
 	 * attempts after direct reclaim are synchronous
 	 */
-	page = __alloc_pages_direct_compact(gfp_mask, order,
-					zonelist, high_zoneidx,
-					nodemask,
-					alloc_flags, preferred_zone,
-					migratetype, sync_migration,
-					&deferred_compaction,
-					&did_some_progress);
-	if (page)
-		goto got_pg;
+	if (unlikely(order)) {
+		page = __alloc_pages_direct_compact(gfp_mask, order,
+						zonelist, high_zoneidx,
+						nodemask,
+						alloc_flags, preferred_zone,
+						migratetype, sync_migration,
+						&deferred_compaction,
+						&did_some_progress);
+		if (page)
+			goto got_pg;
+	}
 	sync_migration = true;
 
 	/*
@@ -2446,15 +2450,17 @@ rebalance:
 		 * direct reclaim and reclaim/compaction depends on compaction
 		 * being called after reclaim so call directly if necessary
 		 */
-		page = __alloc_pages_direct_compact(gfp_mask, order,
-					zonelist, high_zoneidx,
-					nodemask,
-					alloc_flags, preferred_zone,
-					migratetype, sync_migration,
-					&deferred_compaction,
-					&did_some_progress);
-		if (page)
-			goto got_pg;
+		if (unlikely(order)) {
+			page = __alloc_pages_direct_compact(gfp_mask, order,
+						zonelist, high_zoneidx,
+						nodemask,
+						alloc_flags, preferred_zone,
+						migratetype, sync_migration,
+						&deferred_compaction,
+						&did_some_progress);
+			if (page)
+				goto got_pg;
+		}
 	}
 
 nopage:
-- 
1.7.9.5

--
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: don't invoke __alloc_pages_direct_compact when order 0

[Minchan Kim]

Hi Joonsoo,

On Sat, Jul 07, 2012 at 12:28:41AM +0900, Joonsoo Kim wrote:
> __alloc_pages_direct_compact has many arguments so invoking it is very costly.

It's already slow path so it's pointless for such optimization.

> And in almost invoking case, order is 0, so return immediately.

You can't make sure it.

> 
> Let's not invoke it when order 0

Let's not ruin git blame.

> 
> Signed-off-by: Joonsoo Kim <js1304@gmail.com>
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6092f33..f4039aa 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2056,7 +2056,10 @@ out:
>  }
>  
>  #ifdef CONFIG_COMPACTION
> -/* Try memory compaction for high-order allocations before reclaim */
> +/*
> + * Try memory compaction for high-order allocations before reclaim
> + * Must be called with order > 0
> + */
>  static struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	struct zonelist *zonelist, enum zone_type high_zoneidx,
> @@ -2067,8 +2070,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  {
>  	struct page *page;
>  
> -	if (!order)
> -		return NULL;
> +	BUG_ON(!order);
>  
>  	if (compaction_deferred(preferred_zone, order)) {
>  		*deferred_compaction = true;
> @@ -2363,15 +2365,17 @@ rebalance:
>  	 * Try direct compaction. The first pass is asynchronous. Subsequent
>  	 * attempts after direct reclaim are synchronous
>  	 */
> -	page = __alloc_pages_direct_compact(gfp_mask, order,
> -					zonelist, high_zoneidx,
> -					nodemask,
> -					alloc_flags, preferred_zone,
> -					migratetype, sync_migration,
> -					&deferred_compaction,
> -					&did_some_progress);
> -	if (page)
> -		goto got_pg;
> +	if (unlikely(order)) {
> +		page = __alloc_pages_direct_compact(gfp_mask, order,
> +						zonelist, high_zoneidx,
> +						nodemask,
> +						alloc_flags, preferred_zone,
> +						migratetype, sync_migration,
> +						&deferred_compaction,
> +						&did_some_progress);
> +		if (page)
> +			goto got_pg;
> +	}
>  	sync_migration = true;
>  
>  	/*
> @@ -2446,15 +2450,17 @@ rebalance:
>  		 * direct reclaim and reclaim/compaction depends on compaction
>  		 * being called after reclaim so call directly if necessary
>  		 */
> -		page = __alloc_pages_direct_compact(gfp_mask, order,
> -					zonelist, high_zoneidx,
> -					nodemask,
> -					alloc_flags, preferred_zone,
> -					migratetype, sync_migration,
> -					&deferred_compaction,
> -					&did_some_progress);
> -		if (page)
> -			goto got_pg;
> +		if (unlikely(order)) {
> +			page = __alloc_pages_direct_compact(gfp_mask, order,
> +						zonelist, high_zoneidx,
> +						nodemask,
> +						alloc_flags, preferred_zone,
> +						migratetype, sync_migration,
> +						&deferred_compaction,
> +						&did_some_progress);
> +			if (page)
> +				goto got_pg;
> +		}
>  	}
>  
>  nopage:
> -- 
> 1.7.9.5
> 
> --
> 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>

--
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: don't invoke __alloc_pages_direct_compact when order 0

[JoonSoo Kim]

2012/7/7 Minchan Kim <minchan@kernel.org>:
> Hi Joonsoo,
>
> On Sat, Jul 07, 2012 at 12:28:41AM +0900, Joonsoo Kim wrote:
>> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
>
> It's already slow path so it's pointless for such optimization.

I know this is so minor optimization.
But why don't we do such a one?
Is there any weak point?

>> And in almost invoking case, order is 0, so return immediately.
>
> You can't make sure it.

Okay.

>>
>> Let's not invoke it when order 0
>
> Let's not ruin git blame.

Hmm...
When I do git blame, I can't find anything related to this.

Thanks for comments.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[Minchan Kim]

On Sat, Jul 07, 2012 at 01:58:24AM +0900, JoonSoo Kim wrote:
> 2012/7/7 Minchan Kim <minchan@kernel.org>:
> > Hi Joonsoo,
> >
> > On Sat, Jul 07, 2012 at 12:28:41AM +0900, Joonsoo Kim wrote:
> >> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
> >
> > It's already slow path so it's pointless for such optimization.
> 
> I know this is so minor optimization.
> But why don't we do such a one?
> Is there any weak point?

Let's think about it.
You are adding *new rule* for minor optimization.
If new users uses __alloc_pages_direct_compact in future, they always have to
check whether order is zero not not. So it could increase code size as well as
bad for readbility. Even, I'm not sure adding branch is always win than
just passing the some arguement in all architecures.

> 
> >> And in almost invoking case, order is 0, so return immediately.
> >
> > You can't make sure it.
> 
> Okay.
> 
> >>
> >> Let's not invoke it when order 0
> >
> > Let's not ruin git blame.
> 
> Hmm...
> When I do git blame, I can't find anything related to this.

I mean if we merge the pointless patch, it could be showed firstly instead of
meaningful patch when we do git blame. It makes us bothering when we find blame-patch.

> 
> Thanks for comments

--
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: don't invoke __alloc_pages_direct_compact when order 0

[JoonSoo Kim]

>>
>> >> And in almost invoking case, order is 0, so return immediately.
>> >
>> > You can't make sure it.
>>
>> Okay.
>>
>> >>
>> >> Let's not invoke it when order 0
>> >
>> > Let's not ruin git blame.
>>
>> Hmm...
>> When I do git blame, I can't find anything related to this.
>
> I mean if we merge the pointless patch, it could be showed firstly instead of
> meaningful patch when we do git blame. It makes us bothering when we find blame-patch.

Oh... I see.

Thanks for comments.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[David Rientjes]

On Sat, 7 Jul 2012, Joonsoo Kim wrote:

> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
> And in almost invoking case, order is 0, so return immediately.
> 

If "zero cost" is "very costly", then this might make sense.

__alloc_pages_direct_compact() is inlined by gcc.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[JoonSoo Kim]

2012/7/7 David Rientjes <rientjes@google.com>:
> On Sat, 7 Jul 2012, Joonsoo Kim wrote:
>
>> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
>> And in almost invoking case, order is 0, so return immediately.
>>
>
> If "zero cost" is "very costly", then this might make sense.
>
> __alloc_pages_direct_compact() is inlined by gcc.

In my kernel image, __alloc_pages_direct_compact() is not inlined by gcc.
So I send this patch.
But, currently I think it is not useful, so drop it.

Thanks for comments.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[David Rientjes]

On Sun, 8 Jul 2012, JoonSoo Kim wrote:

> >> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
> >> And in almost invoking case, order is 0, so return immediately.
> >>
> >
> > If "zero cost" is "very costly", then this might make sense.
> >
> > __alloc_pages_direct_compact() is inlined by gcc.
> 
> In my kernel image, __alloc_pages_direct_compact() is not inlined by gcc.

Adding Andrew and Mel to the thread since this would require that we 
revert 11e33f6a55ed ("page allocator: break up the allocator entry point 
into fast and slow paths") which would obviously not be a clean revert 
since there have been several changes to these functions over the past 
three years.

I'm stunned (and skeptical) that __alloc_pages_direct_compact() is not 
inlined by your gcc, especially since the kernel must be compiled with 
optimization (either -O1 or -O2 which causes these functions to be 
inlined).  What version of gcc are you using and on what architecture?  
Please do "make mm/page_alloc.s" and send it to me privately, I'll file 
this and fix it up on gcc-bugs.

I'll definitely be following up on this.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[JoonSoo Kim]

2012/7/9 David Rientjes <rientjes@google.com>:
> On Sun, 8 Jul 2012, JoonSoo Kim wrote:
>
>> >> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
>> >> And in almost invoking case, order is 0, so return immediately.
>> >>
>> >
>> > If "zero cost" is "very costly", then this might make sense.
>> >
>> > __alloc_pages_direct_compact() is inlined by gcc.
>>
>> In my kernel image, __alloc_pages_direct_compact() is not inlined by gcc.
>
> Adding Andrew and Mel to the thread since this would require that we
> revert 11e33f6a55ed ("page allocator: break up the allocator entry point
> into fast and slow paths") which would obviously not be a clean revert
> since there have been several changes to these functions over the past
> three years.

Only "__alloc_pages_direct_compact()" is not inlined.
All others (__alloc_pages_high_priority, __alloc_pages_direct_reclaim,
...) are inlined correctly.
So revert is not needed.

I think __alloc_pages_direct_compact() can't be inlined by gcc,
because it is so big and is invoked two times in __alloc_pages_nodemask().

> I'm stunned (and skeptical) that __alloc_pages_direct_compact() is not
> inlined by your gcc, especially since the kernel must be compiled with
> optimization (either -O1 or -O2 which causes these functions to be
> inlined).  What version of gcc are you using and on what architecture?
> Please do "make mm/page_alloc.s" and send it to me privately, I'll file
> this and fix it up on gcc-bugs.

I will send result of "make mm/page_alloc.s" to you privately.
My environments is "x86_64, GNU C version 4.6.3"

> I'll definitely be following up on this.

Thanks for comments.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[David Rientjes]

On Mon, 9 Jul 2012, JoonSoo Kim wrote:

> I think __alloc_pages_direct_compact() can't be inlined by gcc,
> because it is so big and is invoked two times in __alloc_pages_nodemask().
> 

We could fix that by doing

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2057,7 +2057,7 @@ out:
 
 #ifdef CONFIG_COMPACTION
 /* Try memory compaction for high-order allocations before reclaim */
-static struct page *
+static __always_inline struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	struct zonelist *zonelist, enum zone_type high_zoneidx,
 	nodemask_t *nodemask, int alloc_flags, struct zone *preferred_zone,

but I'm not convinced that it's helpful for performance in the slowpath 
and there's no guarantee that it's called more often for order-0 
allocations since it is called as a fallback when should_alloc_retry() 
fails.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[Andrew Morton]

On Mon, 9 Jul 2012 23:13:50 +0900
JoonSoo Kim <js1304@gmail.com> wrote:

> >> In my kernel image, __alloc_pages_direct_compact() is not inlined by gcc.

My gcc-4.4.4 doesn't inline it either.

> I think __alloc_pages_direct_compact() can't be inlined by gcc,
> because it is so big and is invoked two times in __alloc_pages_nodemask().

This.  Large function, two callsites.

Making __alloc_pages_direct_compact() __always_inline adds only 26
bytes to my page_alloc.o's .text.  Such is the suckiness of passing
eleven arguments!

--
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: don't invoke __alloc_pages_direct_compact when order 0

[Mel Gorman]

On Sun, Jul 08, 2012 at 11:33:14AM +0900, JoonSoo Kim wrote:
> 2012/7/7 David Rientjes <rientjes@google.com>:
> > On Sat, 7 Jul 2012, Joonsoo Kim wrote:
> >
> >> __alloc_pages_direct_compact has many arguments so invoking it is very costly.
> >> And in almost invoking case, order is 0, so return immediately.
> >>
> >
> > If "zero cost" is "very costly", then this might make sense.
> >
> > __alloc_pages_direct_compact() is inlined by gcc.
> 
> In my kernel image, __alloc_pages_direct_compact() is not inlined by gcc.

Indeed it is due to their being two callsites. In most cases, the page
allocator takes care that functions have only one callsite so they get
inlined.

You say that invoking the function is very costly. I agree that a function
call with that many parameters is hefty but it is also in the slow path of
the allocator. For order-0 allocations we are about to enter direct reclaim
where I would expect the cost far exceeds the cost of a function call.

If the cost is indeed high and you have seen this in profiles then I
suggest you create a forced inline function alloc_pages_direct_compact
that does this;

if (order != 0)
	__alloc_pages_direct_compact(...)

and then call alloc_pages_direct_compact instead of
__alloc_pages_direct_compact. After that, recheck the profiles (although I
expect the difference to be marginal) and the size of vmlinux (if it gets
bigger, it's probably not worth it).

That would be functionally similar to your patch but it will preserve git
blame, churn less code and be harder to make mistakes with in the unlikely
event a third call to alloc_pages_direct_compact is ever added.

Thanks.

-- 
Mel Gorman
SUSE Labs

--
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: don't invoke __alloc_pages_direct_compact when order 0

[JoonSoo Kim]

2012/7/10 Mel Gorman <mgorman@suse.de>:
> You say that invoking the function is very costly. I agree that a function
> call with that many parameters is hefty but it is also in the slow path of
> the allocator. For order-0 allocations we are about to enter direct reclaim
> where I would expect the cost far exceeds the cost of a function call.

Yes, I agree.

> If the cost is indeed high and you have seen this in profiles then I
> suggest you create a forced inline function alloc_pages_direct_compact
> that does this;
>
> if (order != 0)
>         __alloc_pages_direct_compact(...)
>
> and then call alloc_pages_direct_compact instead of
> __alloc_pages_direct_compact. After that, recheck the profiles (although I
> expect the difference to be marginal) and the size of vmlinux (if it gets
> bigger, it's probably not worth it).
> That would be functionally similar to your patch but it will preserve git
> blame, churn less code and be harder to make mistakes with in the unlikely
> event a third call to alloc_pages_direct_compact is ever added.

Your suggestion looks good.
But, the size of page_alloc.o is more than before.

I test 3 approaches, vanilla, always_inline and
wrapping(alloc_page_direct_compact which is your suggestion).
In my environment (v3.5-rc5, gcc 4.6.3, x86_64), page_alloc.o shows
below number.

                                         total, .text section, .text.unlikely
page_alloc_vanilla.o:     93432,   0x510a,        0x243
page_alloc_inline.o:       93336,   0x52ca,          0xa4
page_alloc_wrapping.o: 93528,   0x515a,        0x238

Andrew said that inlining add only 26 bytes to .text of page_alloc.o,
but in my system, need more bytes.
Currently, I think this patch doesn't have obvious benefit, so I want
to drop it.
Any objections?

Thanks for good comments.

--
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: don't invoke __alloc_pages_direct_compact when order 0

[Mel Gorman]

On Wed, Jul 11, 2012 at 12:24:41AM +0900, JoonSoo Kim wrote:
> > That would be functionally similar to your patch but it will preserve git
> > blame, churn less code and be harder to make mistakes with in the unlikely
> > event a third call to alloc_pages_direct_compact is ever added.
> 
> Your suggestion looks good.
> But, the size of page_alloc.o is more than before.
> 
> I test 3 approaches, vanilla, always_inline and
> wrapping(alloc_page_direct_compact which is your suggestion).
> In my environment (v3.5-rc5, gcc 4.6.3, x86_64), page_alloc.o shows
> below number.
> 
>                                          total, .text section, .text.unlikely
> page_alloc_vanilla.o:     93432,   0x510a,        0x243
> page_alloc_inline.o:       93336,   0x52ca,          0xa4
> page_alloc_wrapping.o: 93528,   0x515a,        0x238
> 
> Andrew said that inlining add only 26 bytes to .text of page_alloc.o,
> but in my system, need more bytes.
> Currently, I think this patch doesn't have obvious benefit, so I want
> to drop it.
> Any objections?
> 

No objections to dropping the patch. It was at worth looking at so thanks
for that.

-- 
Mel Gorman
SUSE Labs

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