| Message ID | 20210208082450.15716-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hugetlb: use helper huge_page_size() to get hugepage size | expand |
On 2/8/21 12:24 AM, Miaohe Lin wrote: > We can use helper huge_page_size() to get the hugepage size directly to > simplify the code slightly. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/hugetlb.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 18628f8dbfb0..6cdb59d8f663 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3199,7 +3199,7 @@ void __init hugetlb_add_hstate(unsigned int order) > BUG_ON(order == 0); > h = &hstates[hugetlb_max_hstate++]; > h->order = order; > - h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1); > + h->mask = ~(huge_page_size(h) - 1); > for (i = 0; i < MAX_NUMNODES; ++i) > INIT_LIST_HEAD(&h->hugepage_freelists[i]); > INIT_LIST_HEAD(&h->hugepage_activelist); > @@ -3474,7 +3474,7 @@ void hugetlb_report_meminfo(struct seq_file *m) > for_each_hstate(h) { > unsigned long count = h->nr_huge_pages; > > - total += (PAGE_SIZE << huge_page_order(h)) * count; > + total += huge_page_size(h) * count; > > if (h == &default_hstate) > seq_printf(m, > @@ -3487,10 +3487,10 @@ void hugetlb_report_meminfo(struct seq_file *m) > h->free_huge_pages, > h->resv_huge_pages, > h->surplus_huge_pages, > - (PAGE_SIZE << huge_page_order(h)) / 1024); > + huge_page_size(h) / SZ_1K); > } > > - seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024); > + seq_printf(m, "Hugetlb: %8lu kB\n", total / SZ_1K); > } > > int hugetlb_report_node_meminfo(char *buf, int len, int nid) > @@ -3524,7 +3524,7 @@ void hugetlb_show_meminfo(void) > h->nr_huge_pages_node[nid], > h->free_huge_pages_node[nid], > h->surplus_huge_pages_node[nid], > - 1UL << (huge_page_order(h) + PAGE_SHIFT - 10)); > + huge_page_size(h) >> 10); Should we change this to huge_page_size(h) / SZ_1K); as in hugetlb_report_meminfo above? Or, is that one where it takes an additional instruction to do the divide as opposed to the shift? I would rather add the instruction and keep everything consistent.
Hi： On 2021/2/9 8:45, Mike Kravetz wrote: > On 2/8/21 12:24 AM, Miaohe Lin wrote: >> We can use helper huge_page_size() to get the hugepage size directly to >> simplify the code slightly. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/hugetlb.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 18628f8dbfb0..6cdb59d8f663 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -3199,7 +3199,7 @@ void __init hugetlb_add_hstate(unsigned int order) >> BUG_ON(order == 0); >> h = &hstates[hugetlb_max_hstate++]; >> h->order = order; >> - h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1); >> + h->mask = ~(huge_page_size(h) - 1); >> for (i = 0; i < MAX_NUMNODES; ++i) >> INIT_LIST_HEAD(&h->hugepage_freelists[i]); >> INIT_LIST_HEAD(&h->hugepage_activelist); >> @@ -3474,7 +3474,7 @@ void hugetlb_report_meminfo(struct seq_file *m) >> for_each_hstate(h) { >> unsigned long count = h->nr_huge_pages; >> >> - total += (PAGE_SIZE << huge_page_order(h)) * count; >> + total += huge_page_size(h) * count; >> >> if (h == &default_hstate) >> seq_printf(m, >> @@ -3487,10 +3487,10 @@ void hugetlb_report_meminfo(struct seq_file *m) >> h->free_huge_pages, >> h->resv_huge_pages, >> h->surplus_huge_pages, >> - (PAGE_SIZE << huge_page_order(h)) / 1024); >> + huge_page_size(h) / SZ_1K); >> } >> >> - seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024); >> + seq_printf(m, "Hugetlb: %8lu kB\n", total / SZ_1K); >> } >> >> int hugetlb_report_node_meminfo(char *buf, int len, int nid) >> @@ -3524,7 +3524,7 @@ void hugetlb_show_meminfo(void) >> h->nr_huge_pages_node[nid], >> h->free_huge_pages_node[nid], >> h->surplus_huge_pages_node[nid], >> - 1UL << (huge_page_order(h) + PAGE_SHIFT - 10)); >> + huge_page_size(h) >> 10); > > Should we change this to > > huge_page_size(h) / SZ_1K); > > as in hugetlb_report_meminfo above? Or, is that one where it takes an > additional instruction to do the divide as opposed to the shift? I would> rather add the instruction and keep everything consistent. > Yes, it takes an additional instruction to do the divide as opposed to the shift. So I did not change this. But it seems keeping everything consistent in a function is more important. So should I send a V2 to change this or Andrew would kindly handle this ? Many thanks.
On 2/8/21 5:24 PM, Miaohe Lin wrote: > Hi： > On 2021/2/9 8:45, Mike Kravetz wrote: >> On 2/8/21 12:24 AM, Miaohe Lin wrote: >>> We can use helper huge_page_size() to get the hugepage size directly to >>> simplify the code slightly. >>> >>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>> --- >>> mm/hugetlb.c | 14 ++++++-------- >>> 1 file changed, 6 insertions(+), 8 deletions(-) >>> >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index 18628f8dbfb0..6cdb59d8f663 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -3199,7 +3199,7 @@ void __init hugetlb_add_hstate(unsigned int order) >>> BUG_ON(order == 0); >>> h = &hstates[hugetlb_max_hstate++]; >>> h->order = order; >>> - h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1); >>> + h->mask = ~(huge_page_size(h) - 1); >>> for (i = 0; i < MAX_NUMNODES; ++i) >>> INIT_LIST_HEAD(&h->hugepage_freelists[i]); >>> INIT_LIST_HEAD(&h->hugepage_activelist); >>> @@ -3474,7 +3474,7 @@ void hugetlb_report_meminfo(struct seq_file *m) >>> for_each_hstate(h) { >>> unsigned long count = h->nr_huge_pages; >>> >>> - total += (PAGE_SIZE << huge_page_order(h)) * count; >>> + total += huge_page_size(h) * count; >>> >>> if (h == &default_hstate) >>> seq_printf(m, >>> @@ -3487,10 +3487,10 @@ void hugetlb_report_meminfo(struct seq_file *m) >>> h->free_huge_pages, >>> h->resv_huge_pages, >>> h->surplus_huge_pages, >>> - (PAGE_SIZE << huge_page_order(h)) / 1024); >>> + huge_page_size(h) / SZ_1K); >>> } >>> >>> - seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024); >>> + seq_printf(m, "Hugetlb: %8lu kB\n", total / SZ_1K); >>> } >>> >>> int hugetlb_report_node_meminfo(char *buf, int len, int nid) >>> @@ -3524,7 +3524,7 @@ void hugetlb_show_meminfo(void) >>> h->nr_huge_pages_node[nid], >>> h->free_huge_pages_node[nid], >>> h->surplus_huge_pages_node[nid], >>> - 1UL << (huge_page_order(h) + PAGE_SHIFT - 10)); >>> + huge_page_size(h) >> 10); >> >> Should we change this to >> >> huge_page_size(h) / SZ_1K); >>> as in hugetlb_report_meminfo above? Or, is that one where it takes an >> additional instruction to do the divide as opposed to the shift? I would> rather add the instruction and keep everything consistent. >> > > Yes, it takes an additional instruction to do the divide as opposed to the shift. So I did not > change this. But it seems keeping everything consistent in a function is more important. So should > I send a V2 to change this or Andrew would kindly handle this ? I would go ahead and put together a v2 and let Andrew decide how he wants to handle it. You can include, Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Thanks
On 2021/2/9 9:36, Mike Kravetz wrote: > On 2/8/21 5:24 PM, Miaohe Lin wrote: >> Hi： >> On 2021/2/9 8:45, Mike Kravetz wrote: >>> On 2/8/21 12:24 AM, Miaohe Lin wrote: >>>> We can use helper huge_page_size() to get the hugepage size directly to >>>> simplify the code slightly. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/hugetlb.c | 14 ++++++-------- >>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>> index 18628f8dbfb0..6cdb59d8f663 100644 >>>> --- a/mm/hugetlb.c >>>> +++ b/mm/hugetlb.c >>>> @@ -3199,7 +3199,7 @@ void __init hugetlb_add_hstate(unsigned int order) >>>> BUG_ON(order == 0); >>>> h = &hstates[hugetlb_max_hstate++]; >>>> h->order = order; >>>> - h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1); >>>> + h->mask = ~(huge_page_size(h) - 1); >>>> for (i = 0; i < MAX_NUMNODES; ++i) >>>> INIT_LIST_HEAD(&h->hugepage_freelists[i]); >>>> INIT_LIST_HEAD(&h->hugepage_activelist); >>>> @@ -3474,7 +3474,7 @@ void hugetlb_report_meminfo(struct seq_file *m) >>>> for_each_hstate(h) { >>>> unsigned long count = h->nr_huge_pages; >>>> >>>> - total += (PAGE_SIZE << huge_page_order(h)) * count; >>>> + total += huge_page_size(h) * count; >>>> >>>> if (h == &default_hstate) >>>> seq_printf(m, >>>> @@ -3487,10 +3487,10 @@ void hugetlb_report_meminfo(struct seq_file *m) >>>> h->free_huge_pages, >>>> h->resv_huge_pages, >>>> h->surplus_huge_pages, >>>> - (PAGE_SIZE << huge_page_order(h)) / 1024); >>>> + huge_page_size(h) / SZ_1K); >>>> } >>>> >>>> - seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024); >>>> + seq_printf(m, "Hugetlb: %8lu kB\n", total / SZ_1K); >>>> } >>>> >>>> int hugetlb_report_node_meminfo(char *buf, int len, int nid) >>>> @@ -3524,7 +3524,7 @@ void hugetlb_show_meminfo(void) >>>> h->nr_huge_pages_node[nid], >>>> h->free_huge_pages_node[nid], >>>> h->surplus_huge_pages_node[nid], >>>> - 1UL << (huge_page_order(h) + PAGE_SHIFT - 10)); >>>> + huge_page_size(h) >> 10); >>> >>> Should we change this to >>> >>> huge_page_size(h) / SZ_1K); >>>> as in hugetlb_report_meminfo above? Or, is that one where it takes an >>> additional instruction to do the divide as opposed to the shift? I would> rather add the instruction and keep everything consistent. >>> >> >> Yes, it takes an additional instruction to do the divide as opposed to the shift. So I did not >> change this. But it seems keeping everything consistent in a function is more important. So should >> I send a V2 to change this or Andrew would kindly handle this ? > > I would go ahead and put together a v2 and let Andrew decide how he wants > to handle it. You can include, > Will do. Thanks a lot. > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > Thanks >
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 18628f8dbfb0..6cdb59d8f663 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3199,7 +3199,7 @@ void __init hugetlb_add_hstate(unsigned int order) BUG_ON(order == 0); h = &hstates[hugetlb_max_hstate++]; h->order = order; - h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1); + h->mask = ~(huge_page_size(h) - 1); for (i = 0; i < MAX_NUMNODES; ++i) INIT_LIST_HEAD(&h->hugepage_freelists[i]); INIT_LIST_HEAD(&h->hugepage_activelist); @@ -3474,7 +3474,7 @@ void hugetlb_report_meminfo(struct seq_file *m) for_each_hstate(h) { unsigned long count = h->nr_huge_pages; - total += (PAGE_SIZE << huge_page_order(h)) * count; + total += huge_page_size(h) * count; if (h == &default_hstate) seq_printf(m, @@ -3487,10 +3487,10 @@ void hugetlb_report_meminfo(struct seq_file *m) h->free_huge_pages, h->resv_huge_pages, h->surplus_huge_pages, - (PAGE_SIZE << huge_page_order(h)) / 1024); + huge_page_size(h) / SZ_1K); } - seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024); + seq_printf(m, "Hugetlb: %8lu kB\n", total / SZ_1K); } int hugetlb_report_node_meminfo(char *buf, int len, int nid) @@ -3524,7 +3524,7 @@ void hugetlb_show_meminfo(void) h->nr_huge_pages_node[nid], h->free_huge_pages_node[nid], h->surplus_huge_pages_node[nid], - 1UL << (huge_page_order(h) + PAGE_SHIFT - 10)); + huge_page_size(h) >> 10); } void hugetlb_report_usage(struct seq_file *m, struct mm_struct *mm) @@ -3647,9 +3647,7 @@ static int hugetlb_vm_op_split(struct vm_area_struct *vma, unsigned long addr) static unsigned long hugetlb_vm_op_pagesize(struct vm_area_struct *vma) { - struct hstate *hstate = hstate_vma(vma); - - return 1UL << huge_page_shift(hstate); + return huge_page_size(hstate_vma(vma)); } /*
We can use helper huge_page_size() to get the hugepage size directly to simplify the code slightly. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/hugetlb.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)