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