* [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-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-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: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).