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

[Mike Kravetz]

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