[PATCH 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[PATCH 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[SeungHun Lee]

"order >= MAX_ORDER" case is occur rarely.

So I add unlikely for this check.
---
 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 b8475ed..e644cf5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 * be using allocators in order of preference for an area that is
 	 * too large.
 	 */
-	if (order >= MAX_ORDER) {
+	if (unlikely(order >= MAX_ORDER)) {
 		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
 		return NULL;
 	}
-- 
1.7.0.4

--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[KOSAKI Motohiro]

(7/28/13 10:48 AM), SeungHun Lee wrote:
> "order >= MAX_ORDER" case is occur rarely.
> 
> So I add unlikely for this check.
> ---
>   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 b8475ed..e644cf5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   	 * be using allocators in order of preference for an area that is
>   	 * too large.
>   	 */
> -	if (order >= MAX_ORDER) {
> +	if (unlikely(order >= MAX_ORDER)) {
>   		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
>   		return NULL;

I don't think this improve any performance because here is a slowpath. However
I also don't find any issue to have this hint.

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[David Rientjes]

On Sun, 28 Jul 2013, SeungHun Lee wrote:

> "order >= MAX_ORDER" case is occur rarely.
> 
> So I add unlikely for this check.

This needs your signed-off-by line.

When that's done:

Acked-by: David Rientjes <rientjes@google.com>

--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[zhouxinxing]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

unlikely indeed makes this code more elegant, however, it's difficult to tell how much the performance will be improved.

David Rientjes <rientjes@google.com> wrote:

>On Sun, 28 Jul 2013, SeungHun Lee wrote:
>
>> "order >= MAX_ORDER" case is occur rarely.
>> 
>> So I add unlikely for this check.
>
>This needs your signed-off-by line.
>
>When that's done:
>
>Acked-by: David Rientjes <rientjes@google.com>
>
>--
>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>

-- 
Sent from my Android device with Gmail Plus. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 1334 bytes --]

Re: [PATCH 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[Dave Hansen]

On 07/28/2013 07:48 AM, SeungHun Lee wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b8475ed..e644cf5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	 * be using allocators in order of preference for an area that is
>  	 * too large.
>  	 */
> -	if (order >= MAX_ORDER) {
> +	if (unlikely(order >= MAX_ORDER)) {
>  		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
>  		return NULL;
>  	}

What problem is this patch solving?  I can see doing this in hot paths,
or places where the compiler is known to be generating bad or suboptimal
code.  but, this costs me 512 bytes of text size:

 898384 Jul 29 15:40 mm/page_alloc.o.nothing
 898896 Jul 29 15:40 mm/page_alloc.o.unlikely

I really don't think we should be adding these without having _concrete_
reasons for it.

--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[Cody P Schafer]

On 07/29/2013 03:45 PM, Dave Hansen wrote:
> On 07/28/2013 07:48 AM, SeungHun Lee wrote:
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index b8475ed..e644cf5 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>   	 * be using allocators in order of preference for an area that is
>>   	 * too large.
>>   	 */
>> -	if (order >= MAX_ORDER) {
>> +	if (unlikely(order >= MAX_ORDER)) {
>>   		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
>>   		return NULL;
>>   	}
>
> What problem is this patch solving?  I can see doing this in hot paths,
> or places where the compiler is known to be generating bad or suboptimal
> code.  but, this costs me 512 bytes of text size:
>
>   898384 Jul 29 15:40 mm/page_alloc.o.nothing
>   898896 Jul 29 15:40 mm/page_alloc.o.unlikely

I took a look at this on my system.

With gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5):

-rw-rw-r-- 1 cody cody 841160 Jul 29 16:47 unlikely/mm/page_alloc.o
-rw-rw-r-- 1 cody cody 840584 Jul 29 16:59 normal/mm/page_alloc.o

    text	   data	    bss	    dec	    hex	filename
   33799	   1414	    184	  35397	   8a45	unlikely/mm/page_alloc.o
   33799	   1414	    184	  35397	   8a45	normal/mm/page_alloc.o

Well, where are are those extra bytes coming from, then?

Using readelf -S + `git diff --no-index --word-diff` shows:
.debug_info      shrinks from 1e991 to 1e98f
.rela.debug_info shrinks from 33a80 to 33a68
.debug_loc         grows from 15e1d to 15ecb
.rela.debug_loc    grows from 26f40 to 270f0
.debug_line        grows from 038eb to 038ed
.debug_str       shrinks from 0adb6 to 0adb2

The sizes of all other sections are unchanged.

Also: comparing vmlinux sizes:
-rwxrwxr-x 1 cody cody 94121230 Jul 29 17:00 normal/vmlinux
-rwxrwxr-x 1 cody cody 94121294 Jul 29 16:51 unlikely/vmlinux

And the bzImage sizes:
-rw-rw-r-- 1 cody cody 2942240 Jul 29 16:51 unlikely/arch/x86/boot/bzImage
-rw-rw-r-- 1 cody cody 2942208 Jul 29 17:00 normal/arch/x86/boot/bzImage

I build this kernel with debug info built in though, what happens when 
it is removed?

-rwxrwxr-x 1 cody cody 16392454 Jul 29 17:33 normal/vmlinux
-rwxrwxr-x 1 cody cody 16392454 Jul 29 17:33 unlikely/vmlinux

-rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 normal/arch/x86/boot/bzImage
-rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 unlikely/arch/x86/boot/bzImage

So, it looks like the only difference in size due to this patch is in 
the debug info.

> I really don't think we should be adding these without having _concrete_
> reasons for it.

--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[Cody P Schafer]

On 07/29/2013 05:36 PM, Cody P Schafer wrote:
> On 07/29/2013 03:45 PM, Dave Hansen wrote:
>> On 07/28/2013 07:48 AM, SeungHun Lee wrote:
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index b8475ed..e644cf5 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned
>>> int order,
>>>        * be using allocators in order of preference for an area that is
>>>        * too large.
>>>        */
>>> -    if (order >= MAX_ORDER) {
>>> +    if (unlikely(order >= MAX_ORDER)) {
>>>           WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
>>>           return NULL;
>>>       }
>>
>> What problem is this patch solving?  I can see doing this in hot paths,
>> or places where the compiler is known to be generating bad or suboptimal
>> code.  but, this costs me 512 bytes of text size:
>>
>>   898384 Jul 29 15:40 mm/page_alloc.o.nothing
>>   898896 Jul 29 15:40 mm/page_alloc.o.unlikely

[...]
>
> -rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 normal/arch/x86/boot/bzImage
> -rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 unlikely/arch/x86/boot/bzImage

So I screwed this last one up and didn't reapply/unapply the patch, so 
they probably are actually different sizes. I'll run a build and check 
tomorrow.

>> I really don't think we should be adding these without having _concrete_
>> reasons for it.
>
> --
> 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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[Cody P Schafer]

On 07/29/2013 05:36 PM, Cody P Schafer wrote:
> On 07/29/2013 03:45 PM, Dave Hansen wrote:
>> On 07/28/2013 07:48 AM, SeungHun Lee wrote:
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index b8475ed..e644cf5 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -2408,7 +2408,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned
>>> int order,
>>>        * be using allocators in order of preference for an area that is
>>>        * too large.
>>>        */
>>> -    if (order >= MAX_ORDER) {
>>> +    if (unlikely(order >= MAX_ORDER)) {
>>>           WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
>>>           return NULL;
>>>       }
>>
>> What problem is this patch solving?  I can see doing this in hot paths,
>> or places where the compiler is known to be generating bad or suboptimal
>> code.  but, this costs me 512 bytes of text size:
>>
>>   898384 Jul 29 15:40 mm/page_alloc.o.nothing
>>   898896 Jul 29 15:40 mm/page_alloc.o.unlikely
>
> I took a look at this on my system.
>
> With gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5):
>
> -rw-rw-r-- 1 cody cody 841160 Jul 29 16:47 unlikely/mm/page_alloc.o
> -rw-rw-r-- 1 cody cody 840584 Jul 29 16:59 normal/mm/page_alloc.o
>
>     text       data        bss        dec        hex    filename
>    33799       1414        184      35397       8a45
> unlikely/mm/page_alloc.o
>    33799       1414        184      35397       8a45
> normal/mm/page_alloc.o
>
> Well, where are are those extra bytes coming from, then?
>
> Using readelf -S + `git diff --no-index --word-diff` shows:
> .debug_info      shrinks from 1e991 to 1e98f
> .rela.debug_info shrinks from 33a80 to 33a68
> .debug_loc         grows from 15e1d to 15ecb
> .rela.debug_loc    grows from 26f40 to 270f0
> .debug_line        grows from 038eb to 038ed
> .debug_str       shrinks from 0adb6 to 0adb2
>
> The sizes of all other sections are unchanged.
>
> Also: comparing vmlinux sizes:
> -rwxrwxr-x 1 cody cody 94121230 Jul 29 17:00 normal/vmlinux
> -rwxrwxr-x 1 cody cody 94121294 Jul 29 16:51 unlikely/vmlinux
>
> And the bzImage sizes:
> -rw-rw-r-- 1 cody cody 2942240 Jul 29 16:51 unlikely/arch/x86/boot/bzImage
> -rw-rw-r-- 1 cody cody 2942208 Jul 29 17:00 normal/arch/x86/boot/bzImage
>
> I build this kernel with debug info built in though, what happens when
> it is removed?
>
> -rwxrwxr-x 1 cody cody 16392454 Jul 29 17:33 normal/vmlinux
> -rwxrwxr-x 1 cody cody 16392454 Jul 29 17:33 unlikely/vmlinux
>
> -rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 normal/arch/x86/boot/bzImage
> -rw-rw-r-- 1 cody cody 2942208 Jul 29 17:33 unlikely/arch/x86/boot/bzImage

Corrected size for bzImage and vmlinux with patch applied:
-rwxrwxr-x 1 cody cody 16392454 Jul 29 18:15 unlikely/vmlinux
-rw-rw-r-- 1 cody cody 2942240 Jul 29 18:15 unlikely/arch/x86/boot/bzImage

--
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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[Dave Hansen]

Cody, it's a good point that we shouldn't be looking at something as
simplistic as the file sizes.  I also used whole vmlinux's and turned
off debuginfo:

   text	   data	    bss	    dec	    hex	filename
10064322	1980968	3051520	15096810	 e65bea	vmlinux.nothing
10064451	1980968	3051520	15096939	 e65c6b	vmlinux.unlikely

So it still cost ~130 bytes of text.  Also, perusing the vmlinux
objdump, adding the unlikely() does look to take
__alloc_pages_direct_compact and move it _closer_ to the page allocation
code.

What does this all mean?  Hell if I know.  It's up to the patch
submitter to explain the implications of the 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 2/2] mm: page_alloc: Add unlikely for MAX_ORDER check

[이승훈]

I submit the patch to make sure "order >= MAX_ORDER" happen unlikely.

But, I couldn't think generated code by compiler.

I think I was wrong.

Thanks your comments.

2013/7/31 Dave Hansen <dave.hansen@intel.com>:
> Cody, it's a good point that we shouldn't be looking at something as
> simplistic as the file sizes.  I also used whole vmlinux's and turned
> off debuginfo:
>
>    text    data     bss     dec     hex filename
> 10064322        1980968 3051520 15096810         e65bea vmlinux.nothing
> 10064451        1980968 3051520 15096939         e65c6b vmlinux.unlikely
>
> So it still cost ~130 bytes of text.  Also, perusing the vmlinux
> objdump, adding the unlikely() does look to take
> __alloc_pages_direct_compact and move it _closer_ to the page allocation
> code.
>
> What does this all mean?  Hell if I know.  It's up to the patch
> submitter to explain the implications of the 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>