linux-kernel - Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon

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>] [day] [month] [year] [list]

Date:   Mon, 18 Nov 2019 03:14:43 +0000
From:   linmiaohe <linmiaohe@...wei.com>
To:     John Hubbard <jhubbard@...dia.com>,
        David Hildenbrand <david@...hat.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "richardw.yang@...ux.intel.com" <richardw.yang@...ux.intel.com>,
        "sfr@...b.auug.org.au" <sfr@...b.auug.org.au>,
        "rppt@...ux.ibm.com" <rppt@...ux.ibm.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "steve.capper@....com" <steve.capper@....com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "aarcange@...hat.com" <aarcange@...hat.com>,
        "chenjianhong2@...wei.com" <chenjianhong2@...wei.com>,
        "walken@...gle.com" <walken@...gle.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "tiny.windzz@...il.com" <tiny.windzz@...il.com>
CC:     "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma


> On 11/15/19 4:58 AM, David Hildenbrand wrote:
>> On 15.11.19 07:36, linmiaohe wrote:
>>> From: Miaohe Lin <linmiaohe@...wei.com>
>> 
>> I'm pro removing unnecessary jump labels.
>
>Thank you, simpler code is good--all other things being equal. 
>The  tradeoff is, as Qian points out: code churn and risk in critical code paths.
>
>In this case, I'd claim it's OK to improve this one, because we can likely get it right by visual inspection, and the pre-existing code is quite poor. And being in the kernel does not necessarily give weak code a free pass to remain there and incur maintenance and annoyance costs until the end of time. :)
>
>However, please spend equal time when you write your commit descriptions, because that's also very important. Commit logs should also be clear!
>
>> 
>> Subject: "mm: get rid of jump labels in find_mergeable_anon_vma()"
>> 
>>>
>>> The odd jump label try_prev and none is not really need
>
>s/need/needed/
>
>> 
>> s/odd jump label/jump labels/
>> 
>> s/is/are/
>> 
>>> in func find_mergeable_anon_vma, eliminate them to improve 
>>> readability.
>>>
>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>>> ---
>>>  mm/mmap.c | 18 +++++++-----------
>>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index 4d4db76a07da..ab980d468a10 100644
>> 
>> Let me suggest the following instead:
>> 
>> /* Try next first */
>> near = vma->vm_next;
>> if (near) {
>> 	anon_vma = reusable_anon_vma(near, vma, near);
>> 	if (anon_vma)
>> 		return anon_vma;
>> }
>> /* Try prev next */
>> near = vma->vm_prev;
>> if (near) {
>> 	anon_vma = reusable_anon_vma(near, vma, near);
>> 	if (anon_vma)
>> 		return anon_vma;
>> }
>> 
>
>Actually, it can be further simplified, because you don't need that last "if" statement, because you're returning NULL after this.
>
>So just return anon_vma there. (And adjust the comment block at the end, so that it's clear that anon_vma might be null.)
>
>

Many Thanks for all of your precious advice. I will fix my patch and send a v2 patch. Thanks again.