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.