linux-kernel - Re: [PATCH v2 0/4] Change the return value for page isolation functions

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

Date:   Wed, 15 Feb 2023 09:21:33 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     David Hildenbrand <david@...hat.com>, akpm@...ux-foundation.org
Cc:     torvalds@...ux-foundation.org, sj@...nel.org, hannes@...xchg.org,
        mhocko@...nel.org, roman.gushchin@...ux.dev, shakeelb@...gle.com,
        muchun.song@...ux.dev, naoya.horiguchi@....com,
        linmiaohe@...wei.com, osalvador@...e.de, mike.kravetz@...cle.com,
        willy@...radead.org, damon@...ts.linux.dev,
        cgroups@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] Change the return value for page isolation
 functions



On 2/15/2023 1:52 AM, David Hildenbrand wrote:
> On 14.02.23 14:59, Baolin Wang wrote:
>> Now the page isolation functions did not return a boolean to indicate
>> success or not, instead it will return a negative error when failed
>> to isolate a page. So below code used in most places seem a boolean
>> success/failure thing, which can confuse people whether the isolation
>> is successful.
>>
>> if (folio_isolate_lru(folio))
>>          continue;
>>
>> Moreover the page isolation functions only return 0 or -EBUSY, and
>> most users did not care about the negative error except for few users,
>> thus we can convert all page isolation functions to return a boolean
>> value, which can remove the confusion to make code more clear.
>>
>> No functional changes intended in this patch series.
>>
>> Changes from v1:
>>   - Convert all isolation functions to return bool.
> 
> Acked-by: David Hildenbrand <david@...hat.com>

Thanks.

> 
> Although it's controversial if
> 
> if (!ret)
>      ret = -EBUSY;
> else
>      ret = 0;
> 
> is really appealing to the readers eye :)
> 
> ret = ret ? 0 : -EBUSY;
> 
> It's still confusing.
> 
> would be better as
> 
> ret = isolated ? 0 : -EBUSY;
> 
> IOW, not reusing the "int ret" variable.

Yes, pretty clear. Will do in next version.