linux-kernel - Re: [PATCH v11 6/9] hugetlb_cgroup: support noreserve mappings

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]

Message-ID: <6b097a9d-3b2c-94cb-21a8-7e7a55a8f635@oracle.com>
Date:   Tue, 11 Feb 2020 13:51:33 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Mina Almasry <almasrymina@...gle.com>
Cc:     shuah <shuah@...nel.org>, David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Greg Thelen <gthelen@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        open list <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org, cgroups@...r.kernel.org
Subject: Re: [PATCH v11 6/9] hugetlb_cgroup: support noreserve mappings

On 2/11/20 1:35 PM, Mina Almasry wrote:
> On Thu, Feb 6, 2020 at 2:31 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>> On 2/3/20 3:22 PM, Mina Almasry wrote:
>>> +++ b/mm/hugetlb.c
>>> @@ -1339,6 +1339,9 @@ static void __free_huge_page(struct page *page)
>>>       clear_page_huge_active(page);
>>>       hugetlb_cgroup_uncharge_page(hstate_index(h), pages_per_huge_page(h),
>>>                                    page, false);
>>> +     hugetlb_cgroup_uncharge_page(hstate_index(h), pages_per_huge_page(h),
>>> +                                  page, true);
>>> +
>>
>> When looking at the code without change markings, the two above lines
>> look so similar my first thought is there must be a mistake.
>>
>> A suggestion for better code readability:
>> - hugetlb_cgroup_uncharge_page could just take "struct hstate *h" and
>>   get both hstate_index(h) and pages_per_huge_page(h).
>> - Perhaps make hugetlb_cgroup_uncharge_page and
>>   hugetlb_cgroup_uncharge_page_rsvd be wrappers around a common routine.
>>   Then the above would look like:
>>
>>   hugetlb_cgroup_uncharge_page(h, page);
>>   hugetlb_cgroup_uncharge_page_rsvd(h, page);
>>
> 
> I did modify the interfaces to this, as it's much better for
> readability indeed. Unfortunately the patch the adds interfaces
> probably needs a re-review now as it's changed quite a bit, I did not
> carry your or David's Reviewed-by.

No worries.  Happy to review again.

-- 
Mike Kravetz