* [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
@ 2016-07-09 15:55 chengang
2016-07-11 0:26 ` Minchan Kim
0 siblings, 1 reply; 9+ messages in thread
From: chengang @ 2016-07-09 15:55 UTC (permalink / raw)
To: akpm, minchan, vbabka, mgorman, mhocko
Cc: gi-oh.kim, iamjoonsoo.kim, hillf.zj, rientjes, linux-mm,
linux-kernel, Chen Gang, Chen Gang
From: Chen Gang <chengang@emindsoft.com.cn>
For pure bool function's return value, bool is a little better more or
less than int.
And return boolean result directly, since 'if' statement is also for
boolean checking, and return boolean result, too.
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
include/linux/migrate.h | 4 ++--
mm/compaction.c | 9 +++------
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index ae8d475..0e366f8 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -72,11 +72,11 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
#endif /* CONFIG_MIGRATION */
#ifdef CONFIG_COMPACTION
-extern int PageMovable(struct page *page);
+extern bool PageMovable(struct page *page);
extern void __SetPageMovable(struct page *page, struct address_space *mapping);
extern void __ClearPageMovable(struct page *page);
#else
-static inline int PageMovable(struct page *page) { return 0; };
+static inline bool PageMovable(struct page *page) { return false; };
static inline void __SetPageMovable(struct page *page,
struct address_space *mapping)
{
diff --git a/mm/compaction.c b/mm/compaction.c
index 0bd53fb..cfcfe88 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -95,19 +95,16 @@ static inline bool migrate_async_suitable(int migratetype)
#ifdef CONFIG_COMPACTION
-int PageMovable(struct page *page)
+bool PageMovable(struct page *page)
{
struct address_space *mapping;
VM_BUG_ON_PAGE(!PageLocked(page), page);
if (!__PageMovable(page))
- return 0;
+ return false;
mapping = page_mapping(page);
- if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
- return 1;
-
- return 0;
+ return mapping && mapping->a_ops && mapping->a_ops->isolate_page;
}
EXPORT_SYMBOL(PageMovable);
--
1.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-09 15:55 [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable chengang
@ 2016-07-11 0:26 ` Minchan Kim
2016-07-11 19:47 ` Chen Gang
0 siblings, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2016-07-11 0:26 UTC (permalink / raw)
To: chengang
Cc: akpm, vbabka, mgorman, mhocko, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On Sat, Jul 09, 2016 at 11:55:04PM +0800, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
>
> For pure bool function's return value, bool is a little better more or
> less than int.
>
> And return boolean result directly, since 'if' statement is also for
> boolean checking, and return boolean result, too.
I just wanted to consistent with other PageXXX flags functions, PageAnon,
PageMappingFlags which returns int rather than bool. Although I agree bool
is natural, I want to be consistent with others rather than breaking at
the moment.
Maybe if you feel it's really helpful, you might be able to handle all
of places I mentioned for better readability and keeping consistency.
But I doubt it's worth.
Thanks.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> include/linux/migrate.h | 4 ++--
> mm/compaction.c | 9 +++------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index ae8d475..0e366f8 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -72,11 +72,11 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
> #endif /* CONFIG_MIGRATION */
>
> #ifdef CONFIG_COMPACTION
> -extern int PageMovable(struct page *page);
> +extern bool PageMovable(struct page *page);
> extern void __SetPageMovable(struct page *page, struct address_space *mapping);
> extern void __ClearPageMovable(struct page *page);
> #else
> -static inline int PageMovable(struct page *page) { return 0; };
> +static inline bool PageMovable(struct page *page) { return false; };
> static inline void __SetPageMovable(struct page *page,
> struct address_space *mapping)
> {
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 0bd53fb..cfcfe88 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -95,19 +95,16 @@ static inline bool migrate_async_suitable(int migratetype)
>
> #ifdef CONFIG_COMPACTION
>
> -int PageMovable(struct page *page)
> +bool PageMovable(struct page *page)
> {
> struct address_space *mapping;
>
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> if (!__PageMovable(page))
> - return 0;
> + return false;
>
> mapping = page_mapping(page);
> - if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
> - return 1;
> -
> - return 0;
> + return mapping && mapping->a_ops && mapping->a_ops->isolate_page;
> }
> EXPORT_SYMBOL(PageMovable);
>
> --
> 1.9.3
>
> --
> 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] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-11 0:26 ` Minchan Kim
@ 2016-07-11 19:47 ` Chen Gang
2016-07-12 7:15 ` Vlastimil Babka
2016-07-12 7:48 ` Michal Hocko
0 siblings, 2 replies; 9+ messages in thread
From: Chen Gang @ 2016-07-11 19:47 UTC (permalink / raw)
To: Minchan Kim
Cc: akpm, vbabka, mgorman, mhocko, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On 7/11/16 08:26, Minchan Kim wrote:
> On Sat, Jul 09, 2016 at 11:55:04PM +0800, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> For pure bool function's return value, bool is a little better more or
>> less than int.
>>
>> And return boolean result directly, since 'if' statement is also for
>> boolean checking, and return boolean result, too.
>
> I just wanted to consistent with other PageXXX flags functions, PageAnon,
> PageMappingFlags which returns int rather than bool. Although I agree bool
> is natural, I want to be consistent with others rather than breaking at
> the moment.
>
> Maybe if you feel it's really helpful, you might be able to handle all
> of places I mentioned for better readability and keeping consistency.
OK, I guess, we can send another patch for include/linux/page-flags.h
for PageXXX.
> But I doubt it's worth.
>
In our case, the 2 output size are same, but under x86_64, the insns are
different. After uses bool, it uses push/pop instead of branch, for me,
it should be a little better for catching.
The orig:
0000000000001290 <PageMovable>:
1290: 48 8b 47 08 mov 0x8(%rdi),%rax
1294: 83 e0 03 and $0x3,%eax
1297: 48 83 f8 02 cmp $0x2,%rax
129b: 74 03 je 12a0 <__SetPageMovable+0x12a0>
129d: 31 c0 xor %eax,%eax
129f: c3 retq
12a0: 55 push %rbp
12a1: 48 89 e5 mov %rsp,%rbp
12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9>
12a9: 48 85 c0 test %rax,%rax
12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5>
12ae: 48 8b 50 68 mov 0x68(%rax),%rdx
12b2: 48 85 d2 test %rdx,%rdx
12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5>
12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx)
12bc: b8 01 00 00 00 mov $0x1,%eax
12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5>
12c3: 5d pop %rbp
12c4: c3 retq
12c5: 31 c0 xor %eax,%eax
12c7: 5d pop %rbp
12c8: c3 retq
12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
The new:
0000000000001290 <PageMovable>:
1290: 48 8b 47 08 mov 0x8(%rdi),%rax
1294: 55 push %rbp
1295: 48 89 e5 mov %rsp,%rbp
1298: 53 push %rbx
1299: 31 db xor %ebx,%ebx
129b: 83 e0 03 and $0x3,%eax
129e: 48 83 f8 02 cmp $0x2,%rax
12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9>
12a4: 89 d8 mov %ebx,%eax
12a6: 5b pop %rbx
12a7: 5d pop %rbp
12a8: c3 retq
12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae>
12ae: 48 85 c0 test %rax,%rax
12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4>
12b3: 48 8b 40 68 mov 0x68(%rax),%rax
12b7: 48 85 c0 test %rax,%rax
12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4>
12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
12c1: 0f 95 c3 setne %bl
12c4: 89 d8 mov %ebx,%eax
12c6: 5b pop %rbx
12c7: 5d pop %rbp
12c8: c3 retq
12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-11 19:47 ` Chen Gang
@ 2016-07-12 7:15 ` Vlastimil Babka
2016-07-12 16:42 ` Chen Gang
2016-07-12 7:48 ` Michal Hocko
1 sibling, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2016-07-12 7:15 UTC (permalink / raw)
To: Chen Gang, Minchan Kim
Cc: akpm, mgorman, mhocko, gi-oh.kim, iamjoonsoo.kim, hillf.zj,
rientjes, linux-mm, linux-kernel, Chen Gang
On 07/11/2016 09:47 PM, Chen Gang wrote:
>
> On 7/11/16 08:26, Minchan Kim wrote:
>> On Sat, Jul 09, 2016 at 11:55:04PM +0800, chengang@emindsoft.com.cn wrote:
>>> From: Chen Gang <chengang@emindsoft.com.cn>
>>>
>>> For pure bool function's return value, bool is a little better more or
>>> less than int.
>>>
>>> And return boolean result directly, since 'if' statement is also for
>>> boolean checking, and return boolean result, too.
>>
>> I just wanted to consistent with other PageXXX flags functions, PageAnon,
>> PageMappingFlags which returns int rather than bool. Although I agree bool
>> is natural, I want to be consistent with others rather than breaking at
>> the moment.
>>
>> Maybe if you feel it's really helpful, you might be able to handle all
>> of places I mentioned for better readability and keeping consistency.
>
> OK, I guess, we can send another patch for include/linux/page-flags.h
> for PageXXX.
>
>> But I doubt it's worth.
>>
>
> In our case, the 2 output size are same, but under x86_64, the insns are
> different. After uses bool, it uses push/pop instead of branch, for me,
> it should be a little better for catching.
You mean "caching"? I don't see how this is better for caching. After
the push/pop, the same branch is still there, so it's not eliminated
(which would be indeed better). Somehow the original version just avoids
the function prologue (push rbp, mov rsp, rbp) for the
!__PageMovable(page) case. That's something I would expect e.g. if it
was marked likely(), but here it's probably just accidental that the
heuristics think it's likely in the "int" case and not "bool". So it's
not a valid reason for prefering int over bool. The question is perhaps
if it's indeed likely or unlikely and should be marked as such :)
> The orig:
>
> 0000000000001290 <PageMovable>:
> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
> 1294: 83 e0 03 and $0x3,%eax
> 1297: 48 83 f8 02 cmp $0x2,%rax
> 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0>
> 129d: 31 c0 xor %eax,%eax
> 129f: c3 retq
> 12a0: 55 push %rbp
> 12a1: 48 89 e5 mov %rsp,%rbp
> 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9>
> 12a9: 48 85 c0 test %rax,%rax
> 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5>
> 12ae: 48 8b 50 68 mov 0x68(%rax),%rdx
> 12b2: 48 85 d2 test %rdx,%rdx
> 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5>
> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx)
> 12bc: b8 01 00 00 00 mov $0x1,%eax
> 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5>
> 12c3: 5d pop %rbp
> 12c4: c3 retq
> 12c5: 31 c0 xor %eax,%eax
> 12c7: 5d pop %rbp
> 12c8: c3 retq
> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>
> The new:
>
> 0000000000001290 <PageMovable>:
> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
> 1294: 55 push %rbp
> 1295: 48 89 e5 mov %rsp,%rbp
> 1298: 53 push %rbx
> 1299: 31 db xor %ebx,%ebx
> 129b: 83 e0 03 and $0x3,%eax
> 129e: 48 83 f8 02 cmp $0x2,%rax
> 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9>
> 12a4: 89 d8 mov %ebx,%eax
> 12a6: 5b pop %rbx
> 12a7: 5d pop %rbp
> 12a8: c3 retq
> 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae>
> 12ae: 48 85 c0 test %rax,%rax
> 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4>
> 12b3: 48 8b 40 68 mov 0x68(%rax),%rax
> 12b7: 48 85 c0 test %rax,%rax
> 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4>
> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
> 12c1: 0f 95 c3 setne %bl
> 12c4: 89 d8 mov %ebx,%eax
> 12c6: 5b pop %rbx
> 12c7: 5d pop %rbp
> 12c8: c3 retq
> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>
> Thanks.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-11 19:47 ` Chen Gang
2016-07-12 7:15 ` Vlastimil Babka
@ 2016-07-12 7:48 ` Michal Hocko
2016-07-12 16:50 ` Chen Gang
1 sibling, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2016-07-12 7:48 UTC (permalink / raw)
To: Chen Gang
Cc: Minchan Kim, akpm, vbabka, mgorman, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On Tue 12-07-16 03:47:42, Chen Gang wrote:
[...]
> In our case, the 2 output size are same, but under x86_64, the insns are
> different. After uses bool, it uses push/pop instead of branch, for me,
> it should be a little better for catching.
The code generated for bool version looks much worse. Look at the fast
path. Gcc tries to reuse the retq from the fast path in the bool case
and so it has to push rbp and rbx on the stack.
That being said, gcc doesn't seem to generate a better code for bool so
I do not think this is really worth it.
> The orig:
>
> 0000000000001290 <PageMovable>:
> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
> 1294: 83 e0 03 and $0x3,%eax
> 1297: 48 83 f8 02 cmp $0x2,%rax
> 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0>
> 129d: 31 c0 xor %eax,%eax
> 129f: c3 retq
> 12a0: 55 push %rbp
> 12a1: 48 89 e5 mov %rsp,%rbp
> 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9>
> 12a9: 48 85 c0 test %rax,%rax
> 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5>
> 12ae: 48 8b 50 68 mov 0x68(%rax),%rdx
> 12b2: 48 85 d2 test %rdx,%rdx
> 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5>
> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx)
> 12bc: b8 01 00 00 00 mov $0x1,%eax
> 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5>
> 12c3: 5d pop %rbp
> 12c4: c3 retq
> 12c5: 31 c0 xor %eax,%eax
> 12c7: 5d pop %rbp
> 12c8: c3 retq
> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>
> The new:
>
> 0000000000001290 <PageMovable>:
> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
> 1294: 55 push %rbp
> 1295: 48 89 e5 mov %rsp,%rbp
> 1298: 53 push %rbx
> 1299: 31 db xor %ebx,%ebx
> 129b: 83 e0 03 and $0x3,%eax
> 129e: 48 83 f8 02 cmp $0x2,%rax
> 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9>
> 12a4: 89 d8 mov %ebx,%eax
> 12a6: 5b pop %rbx
> 12a7: 5d pop %rbp
> 12a8: c3 retq
> 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae>
> 12ae: 48 85 c0 test %rax,%rax
> 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4>
> 12b3: 48 8b 40 68 mov 0x68(%rax),%rax
> 12b7: 48 85 c0 test %rax,%rax
> 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4>
> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
> 12c1: 0f 95 c3 setne %bl
> 12c4: 89 d8 mov %ebx,%eax
> 12c6: 5b pop %rbx
> 12c7: 5d pop %rbp
> 12c8: c3 retq
> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>
> Thanks.
> --
> Chen Gang (陈刚)
>
> Managing Natural Environments is the Duty of Human Beings.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-12 7:15 ` Vlastimil Babka
@ 2016-07-12 16:42 ` Chen Gang
0 siblings, 0 replies; 9+ messages in thread
From: Chen Gang @ 2016-07-12 16:42 UTC (permalink / raw)
To: Vlastimil Babka, Minchan Kim
Cc: akpm, mgorman, mhocko, gi-oh.kim, iamjoonsoo.kim, hillf.zj,
rientjes, linux-mm, linux-kernel, Chen Gang
On 7/12/16 15:15, Vlastimil Babka wrote:
> On 07/11/2016 09:47 PM, Chen Gang wrote:
>>
>>
>> In our case, the 2 output size are same, but under x86_64, the insns are
>> different. After uses bool, it uses push/pop instead of branch, for me,
>> it should be a little better for catching.
>
> You mean "caching"? I don't see how this is better for caching. After the push/pop, the same branch is still there, so it's not eliminated (which would be indeed better). Somehow the original version just avoids the function prologue (push rbp, mov rsp, rbp) for the !__PageMovable(page) case. That's something I would expect e.g. if it was marked likely(), but here it's probably just accidental that the heuristics think it's likely in the "int" case and not "bool". So it's not a valid reason for prefering int over bool. The question is perhaps if it's indeed likely or unlikely and should be marked as such :)
>
Oh, sorry, after check the details, the result is a little complex (2
things are mixed together, and likely can be also considered):
- One return statement instead of the 3 statements which will change
the detail instructions (in fact, it has negative effect).
- gcc 6.0 and redhat gcc 4.8 generate the different results.
The related output are:
- If use one return statement instead of the 3 statements with gcc 6.0,
the result is my original outputs which we discussed before.
- If still use 3 statements (only use true, false instead of 1, 0) with
gcc 6.0, the 2 outputs are equal.
- If still use 3 statements (only use true, false instead of 1, 0) with
gcc 4.8, the 2 outputs are different, and obviously, the bool will be
a little better (no "xor %ebx,%ebx").
- If use one return statement instead of the 3 statements with gcc 4.8,
the result is a little bad than keeping 3 statements.
- If we add likely(), can get the same result: bool is a little better
(no "movzbl %al,%eax").
All together:
- For return statement, merging multi-statement together is not a good
idea, it will let compiler generates a little bad code.
- For gcc 6.0, in our case, the outputs are the same (and both enable
'likely', too).
- For gcc 4.8, in our case, 'bool' output is a little better than 'int'
(after enable 'likely', also get the same result)
The int output by gcc 4.8:
0000000000001150 <PageMovable>:
1150: 48 8b 57 08 mov 0x8(%rdi),%rdx
1154: 55 push %rbp
1155: 48 89 e5 mov %rsp,%rbp
1158: 53 push %rbx
1159: 31 db xor %ebx,%ebx
115b: 83 e2 03 and $0x3,%edx
115e: 48 83 fa 02 cmp $0x2,%rdx
1162: 74 05 je 1169 <__SetPageMovable+0x1169>
1164: 89 d8 mov %ebx,%eax
1166: 5b pop %rbx
1167: 5d pop %rbp
1168: c3 retq
1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e>
116e: 48 85 c0 test %rax,%rax
1171: 74 f1 je 1164 <__SetPageMovable+0x1164>
1173: 48 8b 40 68 mov 0x68(%rax),%rax
1177: 48 85 c0 test %rax,%rax
117a: 74 e8 je 1164 <__SetPageMovable+0x1164>
117c: 31 db xor %ebx,%ebx
117e: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
1183: 0f 95 c3 setne %bl
1186: 89 d8 mov %ebx,%eax
1188: 5b pop %rbx
1189: 5d pop %rbp
118a: c3 retq
118b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
The bool output by gcc 4.8:
0000000000001150 <PageMovable>:
1150: 48 8b 57 08 mov 0x8(%rdi),%rdx
1154: 55 push %rbp
1155: 48 89 e5 mov %rsp,%rbp
1158: 53 push %rbx
1159: 31 db xor %ebx,%ebx
115b: 83 e2 03 and $0x3,%edx
115e: 48 83 fa 02 cmp $0x2,%rdx
1162: 74 05 je 1169 <__SetPageMovable+0x1169>
1164: 89 d8 mov %ebx,%eax
1166: 5b pop %rbx
1167: 5d pop %rbp
1168: c3 retq
1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e>
116e: 48 85 c0 test %rax,%rax
1171: 74 f1 je 1164 <__SetPageMovable+0x1164>
1173: 48 8b 40 68 mov 0x68(%rax),%rax
1177: 48 85 c0 test %rax,%rax
117a: 74 e8 je 1164 <__SetPageMovable+0x1164>
117c: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
1181: 0f 95 c3 setne %bl
1184: 89 d8 mov %ebx,%eax
1186: 5b pop %rbx
1187: 5d pop %rbp
1188: c3 retq
1189: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
The int output by gcc 4.8 with likely():
0000000000001150 <PageMovable>:
1150: 48 8b 47 08 mov 0x8(%rdi),%rax
1154: 83 e0 03 and $0x3,%eax
1157: 48 83 f8 02 cmp $0x2,%rax
115b: 74 03 je 1160 <__SetPageMovable+0x1160>
115d: 31 c0 xor %eax,%eax
115f: c3 retq
1160: 55 push %rbp
1161: 48 89 e5 mov %rsp,%rbp
1164: e8 00 00 00 00 callq 1169 <__SetPageMovable+0x1169>
1169: 48 85 c0 test %rax,%rax
116c: 74 16 je 1184 <__SetPageMovable+0x1184>
116e: 48 8b 40 68 mov 0x68(%rax),%rax
1172: 48 85 c0 test %rax,%rax
1175: 74 0d je 1184 <__SetPageMovable+0x1184>
1177: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
117c: 5d pop %rbp
117d: 0f 95 c0 setne %al
1180: 0f b6 c0 movzbl %al,%eax
1183: c3 retq
1184: 31 c0 xor %eax,%eax
1186: 5d pop %rbp
1187: c3 retq
1188: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
118f: 00
The bool output by gcc 4.8 with likely():
0000000000001150 <PageMovable>:
1150: 48 8b 47 08 mov 0x8(%rdi),%rax
1154: 83 e0 03 and $0x3,%eax
1157: 48 83 f8 02 cmp $0x2,%rax
115b: 74 03 je 1160 <__SetPageMovable+0x1160>
115d: 31 c0 xor %eax,%eax
115f: c3 retq
1160: 55 push %rbp
1161: 48 89 e5 mov %rsp,%rbp
1164: e8 00 00 00 00 callq 1169 <__SetPageMovable+0x1169>
1169: 48 85 c0 test %rax,%rax
116c: 74 13 je 1181 <__SetPageMovable+0x1181>
116e: 48 8b 40 68 mov 0x68(%rax),%rax
1172: 48 85 c0 test %rax,%rax
1175: 74 0a je 1181 <__SetPageMovable+0x1181>
1177: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
117c: 5d pop %rbp
117d: 0f 95 c0 setne %al
1180: c3 retq
1181: 31 c0 xor %eax,%eax
1183: 5d pop %rbp
1184: c3 retq
1185: 66 66 2e 0f 1f 84 00 data32 nopw %cs:0x0(%rax,%rax,1)
118c: 00 00 00 00
Thanks.
>> The orig:
>>
>> 0000000000001290 <PageMovable>:
>> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
>> 1294: 83 e0 03 and $0x3,%eax
>> 1297: 48 83 f8 02 cmp $0x2,%rax
>> 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0>
>> 129d: 31 c0 xor %eax,%eax
>> 129f: c3 retq
>> 12a0: 55 push %rbp
>> 12a1: 48 89 e5 mov %rsp,%rbp
>> 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9>
>> 12a9: 48 85 c0 test %rax,%rax
>> 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5>
>> 12ae: 48 8b 50 68 mov 0x68(%rax),%rdx
>> 12b2: 48 85 d2 test %rdx,%rdx
>> 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5>
>> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx)
>> 12bc: b8 01 00 00 00 mov $0x1,%eax
>> 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5>
>> 12c3: 5d pop %rbp
>> 12c4: c3 retq
>> 12c5: 31 c0 xor %eax,%eax
>> 12c7: 5d pop %rbp
>> 12c8: c3 retq
>> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>>
>> The new:
>>
>> 0000000000001290 <PageMovable>:
>> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
>> 1294: 55 push %rbp
>> 1295: 48 89 e5 mov %rsp,%rbp
>> 1298: 53 push %rbx
>> 1299: 31 db xor %ebx,%ebx
>> 129b: 83 e0 03 and $0x3,%eax
>> 129e: 48 83 f8 02 cmp $0x2,%rax
>> 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9>
>> 12a4: 89 d8 mov %ebx,%eax
>> 12a6: 5b pop %rbx
>> 12a7: 5d pop %rbp
>> 12a8: c3 retq
>> 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae>
>> 12ae: 48 85 c0 test %rax,%rax
>> 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4>
>> 12b3: 48 8b 40 68 mov 0x68(%rax),%rax
>> 12b7: 48 85 c0 test %rax,%rax
>> 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4>
>> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
>> 12c1: 0f 95 c3 setne %bl
>> 12c4: 89 d8 mov %ebx,%eax
>> 12c6: 5b pop %rbx
>> 12c7: 5d pop %rbp
>> 12c8: c3 retq
>> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>>
>> Thanks.
>>
>
>
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-12 7:48 ` Michal Hocko
@ 2016-07-12 16:50 ` Chen Gang
2016-07-13 7:53 ` Michal Hocko
0 siblings, 1 reply; 9+ messages in thread
From: Chen Gang @ 2016-07-12 16:50 UTC (permalink / raw)
To: Michal Hocko
Cc: Minchan Kim, akpm, vbabka, mgorman, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On 7/12/16 15:48, Michal Hocko wrote:
> On Tue 12-07-16 03:47:42, Chen Gang wrote:
> [...]
>> In our case, the 2 output size are same, but under x86_64, the insns are
>> different. After uses bool, it uses push/pop instead of branch, for me,
>> it should be a little better for catching.
>
> The code generated for bool version looks much worse. Look at the fast
> path. Gcc tries to reuse the retq from the fast path in the bool case
> and so it has to push rbp and rbx on the stack.
>
> That being said, gcc doesn't seem to generate a better code for bool so
> I do not think this is really worth it.
>
The code below also merge 3 statements into 1 return statement, although
for me, it is a little more readable, it will generate a little bad code.
That is the reason why the output looks a little bad.
In our case, for gcc 6.0, using bool instead of int for bool function
will get the same output under x86_64.
In our case, for gcc 4.8, using bool instead of int for bool function
will get a little better output under x86_64.
Thanks.
>> The orig:
>>
>> 0000000000001290 <PageMovable>:
>> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
>> 1294: 83 e0 03 and $0x3,%eax
>> 1297: 48 83 f8 02 cmp $0x2,%rax
>> 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0>
>> 129d: 31 c0 xor %eax,%eax
>> 129f: c3 retq
>> 12a0: 55 push %rbp
>> 12a1: 48 89 e5 mov %rsp,%rbp
>> 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9>
>> 12a9: 48 85 c0 test %rax,%rax
>> 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5>
>> 12ae: 48 8b 50 68 mov 0x68(%rax),%rdx
>> 12b2: 48 85 d2 test %rdx,%rdx
>> 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5>
>> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx)
>> 12bc: b8 01 00 00 00 mov $0x1,%eax
>> 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5>
>> 12c3: 5d pop %rbp
>> 12c4: c3 retq
>> 12c5: 31 c0 xor %eax,%eax
>> 12c7: 5d pop %rbp
>> 12c8: c3 retq
>> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>>
>> The new:
>>
>> 0000000000001290 <PageMovable>:
>> 1290: 48 8b 47 08 mov 0x8(%rdi),%rax
>> 1294: 55 push %rbp
>> 1295: 48 89 e5 mov %rsp,%rbp
>> 1298: 53 push %rbx
>> 1299: 31 db xor %ebx,%ebx
>> 129b: 83 e0 03 and $0x3,%eax
>> 129e: 48 83 f8 02 cmp $0x2,%rax
>> 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9>
>> 12a4: 89 d8 mov %ebx,%eax
>> 12a6: 5b pop %rbx
>> 12a7: 5d pop %rbp
>> 12a8: c3 retq
>> 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae>
>> 12ae: 48 85 c0 test %rax,%rax
>> 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4>
>> 12b3: 48 8b 40 68 mov 0x68(%rax),%rax
>> 12b7: 48 85 c0 test %rax,%rax
>> 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4>
>> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax)
>> 12c1: 0f 95 c3 setne %bl
>> 12c4: 89 d8 mov %ebx,%eax
>> 12c6: 5b pop %rbx
>> 12c7: 5d pop %rbp
>> 12c8: c3 retq
>> 12c9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
>>
>> Thanks.
>> --
>> Chen Gang (陈刚)
>>
>> Managing Natural Environments is the Duty of Human Beings.
>
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-12 16:50 ` Chen Gang
@ 2016-07-13 7:53 ` Michal Hocko
2016-07-17 0:51 ` Chen Gang
0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2016-07-13 7:53 UTC (permalink / raw)
To: Chen Gang
Cc: Minchan Kim, akpm, vbabka, mgorman, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On Wed 13-07-16 00:50:12, Chen Gang wrote:
>
>
> On 7/12/16 15:48, Michal Hocko wrote:
> > On Tue 12-07-16 03:47:42, Chen Gang wrote:
> > [...]
> >> In our case, the 2 output size are same, but under x86_64, the insns are
> >> different. After uses bool, it uses push/pop instead of branch, for me,
> >> it should be a little better for catching.
> >
> > The code generated for bool version looks much worse. Look at the fast
> > path. Gcc tries to reuse the retq from the fast path in the bool case
> > and so it has to push rbp and rbx on the stack.
> >
> > That being said, gcc doesn't seem to generate a better code for bool so
> > I do not think this is really worth it.
> >
>
> The code below also merge 3 statements into 1 return statement, although
> for me, it is a little more readable, it will generate a little bad code.
> That is the reason why the output looks a little bad.
>
> In our case, for gcc 6.0, using bool instead of int for bool function
> will get the same output under x86_64.
If the output is same then there is no reason to change it.
> In our case, for gcc 4.8, using bool instead of int for bool function
> will get a little better output under x86_64.
I had a different impression and the fast path code had more
instructions. But anyway, is there really a strong reason to change
those return values in the first place? Isn't that just a pointless code
churn?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
2016-07-13 7:53 ` Michal Hocko
@ 2016-07-17 0:51 ` Chen Gang
0 siblings, 0 replies; 9+ messages in thread
From: Chen Gang @ 2016-07-17 0:51 UTC (permalink / raw)
To: Michal Hocko
Cc: Minchan Kim, akpm, vbabka, mgorman, gi-oh.kim, iamjoonsoo.kim,
hillf.zj, rientjes, linux-mm, linux-kernel, Chen Gang
On 7/13/16 15:53, Michal Hocko wrote:
> On Wed 13-07-16 00:50:12, Chen Gang wrote:
>>
>>
>> On 7/12/16 15:48, Michal Hocko wrote:
>>> On Tue 12-07-16 03:47:42, Chen Gang wrote:
>>> [...]
>>>> In our case, the 2 output size are same, but under x86_64, the insns are
>>>> different. After uses bool, it uses push/pop instead of branch, for me,
>>>> it should be a little better for catching.
>>>
>>> The code generated for bool version looks much worse. Look at the fast
>>> path. Gcc tries to reuse the retq from the fast path in the bool case
>>> and so it has to push rbp and rbx on the stack.
>>>
>>> That being said, gcc doesn't seem to generate a better code for bool so
>>> I do not think this is really worth it.
>>>
>>
>> The code below also merge 3 statements into 1 return statement, although
>> for me, it is a little more readable, it will generate a little bad code.
>> That is the reason why the output looks a little bad.
>>
>> In our case, for gcc 6.0, using bool instead of int for bool function
>> will get the same output under x86_64.
>
> If the output is same then there is no reason to change it.
>
For the new version gcc, the output is same. But bool is a little more
readable than int for the pure bool function.
But for the current widely used gcc version (I guess, gcc-4.8 is still
widely used), bool will get a little better output than int for the pure
bool function.
>> In our case, for gcc 4.8, using bool instead of int for bool function
>> will get a little better output under x86_64.
>
> I had a different impression and the fast path code had more
> instructions. But anyway, is there really a strong reason to change
> those return values in the first place? Isn't that just a pointless code
> churn?
>
Excuse me, maybe, I do not quite understand your meanings, but I shall
try to explain as far as I can understand (welcome additional detail
explanation, e.g. "return values" means c code or assembly output code).
In the previous reply, I did not mention 3 things directly and clearly
(about my 2 mistakes, and the comparation between gcc 6.0 and 4.8):
- Mistake 1: "Use one return statement instead of several statements"
is not good, the modification may be a little more readable, but it
may get a little bad output code by compiler.
- Mistake 2: I only notice there is more branches, but did not notice
the real execution path (I guess, your "fast path" is about it).
- The optimization of upstream gcc 6.0 is better than redhat gcc 4.8:
in this case, gcc 6.0 will:
generate the same better code for both bool and int for the pure
bool function.
optimize the first checking branch (no prologue) -- gcc 4.8 need
mark 'likely' for it.
skip the 'likely' optimization when "use 1 return statement instead
of several statements" -- generation a little bad code too.
All together, for me:
- Only use bool instead of int for pure bool functions' return value
will get a little better code
- I shall send patch v2, only change bool to int for all Page_XXX, and
keep all the other things no touch.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-17 0:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-09 15:55 [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable chengang
2016-07-11 0:26 ` Minchan Kim
2016-07-11 19:47 ` Chen Gang
2016-07-12 7:15 ` Vlastimil Babka
2016-07-12 16:42 ` Chen Gang
2016-07-12 7:48 ` Michal Hocko
2016-07-12 16:50 ` Chen Gang
2016-07-13 7:53 ` Michal Hocko
2016-07-17 0:51 ` Chen Gang
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).