linux-kernel - Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 12 Jul 2016 09:15:00 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Chen Gang <chengang@...ndsoft.com.cn>,
	Minchan Kim <minchan@...nel.org>
Cc:	akpm@...ux-foundation.org, mgorman@...hsingularity.net,
	mhocko@...e.com, gi-oh.kim@...fitbricks.com,
	iamjoonsoo.kim@....com, hillf.zj@...baba-inc.com,
	rientjes@...gle.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Chen Gang <gang.chen.5i5j@...il.com>
Subject: Re: [PATCH] mm: migrate: Use bool instead of int for the return value
 of PageMovable

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@...ndsoft.com.cn wrote:
>>> From: Chen Gang <chengang@...ndsoft.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.
>