> On 11/15/19 4:58 AM, David Hildenbrand wrote:
>> On 15.11.19 07:36, linmiaohe wrote:
>>> From: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>>> ---
>>> 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.
