linux-kernel - Re: [PATCH v10 2/8] hugetlb_cgroup: add interface for charge/uncharge hugetlb reservations

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:   Wed, 29 Jan 2020 16:41:51 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     David Rientjes <rientjes@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>
Cc:     shakeelb@...gle.com, shuah@...nel.org, gthelen@...gle.com,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org, cgroups@...r.kernel.org,
        aneesh.kumar@...ux.vnet.ibm.com
Subject: Re: [PATCH v10 2/8] hugetlb_cgroup: add interface for charge/uncharge
 hugetlb reservations

On 1/29/20 1:21 PM, David Rientjes wrote:
> On Tue, 14 Jan 2020, Mina Almasry wrote:
> 
>> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
>> index 063962f6dfc6a..eab8a70d5bcb5 100644
>> --- a/include/linux/hugetlb_cgroup.h
>> +++ b/include/linux/hugetlb_cgroup.h
>> @@ -20,29 +20,37 @@
>>  struct hugetlb_cgroup;
>>  /*
>>   * Minimum page order trackable by hugetlb cgroup.
>> - * At least 3 pages are necessary for all the tracking information.
>> + * At least 4 pages are necessary for all the tracking information.
>>   */
>>  #define HUGETLB_CGROUP_MIN_ORDER	2
> 
> I always struggle with a way to document and protect these types of 
> usages.  In this case, we are using the private filed of tail pages; in 
> thp code, we enumerate these usages separately in struct page: see "Tail 
> pages of compound page" comment in the union.  Using the private field is 
> fine to store a pointer to the hugetlb_cgroup, but I'm wondering if we can 
> document or protect against future patches not understanding this usage.  
> Otherwise it's implicit beyond this comment.
> 
> Maybe an expanded comment here is the only thing that is needed because 
> it's unique to struct hugetlb_cgroup that describes what struct page 
> represents for the second, third, and (now) fourth tail page.

I think that expanding the comment may be sufficient.  Let's at least
document what the private field of the of the tail pages are used for
WRT cgroups.
Second tail page (hpage[2]) usage cgroup
Third tail page (hpage[3]) reservation cgroup

BTW, we are just adding usage of the third tail page IIUC.  The comment
that 4 pages are necessary includes the head page.
-- 
Mike Kravetz