On Tue, Nov 21, 2017 at 11:19:07AM -0800, Andrew Morton wrote:
> On Tue, 21 Nov 2017 15:15:55 +0000 Roman Gushchin <g...@fb.com> wrote:
>
> > > > +
> > > > + for_each_hstate(h) {
> > > > + unsigned long count = h->nr_huge_pages;
> > > > +
> > > > + total += (PAGE_SIZE << huge_page_order(h)) * count;
> > > > +
> > > > + if (h == &default_hstate)
> > >
> > > I'm not understanding this test. Are we assuming that default_hstate
> > > always refers to the highest-index hstate? If so why, and is that
> > > valid?
> >
> > As Mike and Michal pointed, default_hstate is defined as
> > #define default_hstate (hstates[default_hstate_idx]),
> > where default_hstate_idx can be altered by a boot argument.
> >
> > We're iterating over all states to calculate total and also
> > print some additional info for the default size. Having a single
> > loop guarantees consistency of these numbers.
> >
>
> OK, I misread the handling of `count' -> HugePages_Total.
>
> It seems unnecessarily obscure?
>
> for_each_hstate(h) {
> unsigned long count = h->nr_huge_pages;
>
> total += (PAGE_SIZE << huge_page_order(h)) * count;
>
> if (h == &default_hstate)
> seq_printf(m,
> "HugePages_Total: %5lu\n"
> "HugePages_Free: %5lu\n"
> "HugePages_Rsvd: %5lu\n"
> "HugePages_Surp: %5lu\n"
> "Hugepagesize: %8lu kB\n",
> count,
> h->free_huge_pages,
> h->resv_huge_pages,
> h->surplus_huge_pages,
> (PAGE_SIZE << huge_page_order(h)) / 1024);
> }
>
> seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024);
>
>
> Why not
>
> seq_printf(m,
> "HugePages_Total: %5lu\n"
> "HugePages_Free: %5lu\n"
> "HugePages_Rsvd: %5lu\n"
> "HugePages_Surp: %5lu\n"
> "Hugepagesize: %8lu kB\n",
> h->nr_huge_pages,
> h->free_huge_pages,
> h->resv_huge_pages,
> h->surplus_huge_pages,
> 1UL << (huge_page_order(h) + PAGE_SHIFT - 10));
>
> for_each_hstate(h)
> total += (PAGE_SIZE << huge_page_order(h)) * h->nr_huge_pages;
> seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024);
>
> ?
The idea was that the local variable guarantees the consistency
between Hugetlb and HugePages_Total numbers. Otherwise we have
to take hugetlb_lock.
What we can do, is to rename "count" into "nr_huge_pages", like:
for_each_hstate(h) {
unsigned long nr_huge_pages = h->nr_huge_pages;
total += (PAGE_SIZE << huge_page_order(h)) * nr_huge_pages;
if (h == &default_hstate)
seq_printf(m,
"HugePages_Total: %5lu\n"
"HugePages_Free: %5lu\n"
"HugePages_Rsvd: %5lu\n"
"HugePages_Surp: %5lu\n"
"Hugepagesize: %8lu kB\n",
nr_huge_pages,
h->free_huge_pages,
h->resv_huge_pages,
h->surplus_huge_pages,
(PAGE_SIZE << huge_page_order(h)) / 1024);
}
seq_printf(m, "Hugetlb: %8lu kB\n", total / 1024);
But maybe taking a lock is not a bad idea, because it will also
guarantee consistency between other numbers (like HugePages_Free) as well,
which is not true right now.
Thanks!
