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

[Mike Kravetz <mike.kravetz@oracle.com>]

On 2/11/20 1:35 PM, Mina Almasry wrote:
> On Thu, Feb 6, 2020 at 2:31 PM Mike Kravetz <mike.kravetz@oracle.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