Re: [PATCH 1/3] mm/hugetlb: fix the inconsistency of /proc/sys/vm/nr_huge_pages

[Xueshi Hu <xueshi.hu@smartx.com>]

On Tue, Aug 1, 2023 at 6:17 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 07/30/23 20:51, Xueshi Hu wrote:
> > When writing to /proc/sys/vm/nr_huge_pages, it indicates global number of
> > huge pages of the default hstate. But when reading from it, it indicates
> > the current number of "persistent" huge pages in the kernel's huge page
> > pool.
> >
> > There are currently four interfaces used to export the number of huge
> > pages:
> > - /proc/meminfo
> > - /proc/sys/vm/*hugepages*
> > - /sys/devices/system/node/node0/hugepages/hugepages-2048kB/*
> > - /sys/kernel/mm/hugepages/hugepages-2048kB/*
> >
> > But only the /proc/sys/vm/nr_huge_pages provides the 'persistent'
> > semantics when reading from it. This inconsistency is very subtle and can
> > be easily misunderstood.
>
> Thanks for looking into this.
>
> The hugetlb documentation (./admin-guide/mm/hugetlbpage.rst) mentions
> the term 'persistent hugetlb pages', but never provides a definition.
>
> We can get the definition from the code as:
> #define persistent_huge_pages(h) (h->nr_huge_pages - h->surplus_huge_pages)
>
> Further, the documentation says:
> "The ``/proc/meminfo`` file provides information about the total number of
>  persistent hugetlb pages in the kernel's huge page pool."
>
> "``/proc/sys/vm/nr_hugepages`` indicates the current number of "persistent"
>  huge pages in the kernel's huge page pool."
>
> "The administrator may shrink the pool of persistent huge pages for
>  the default huge page size by setting the ``nr_hugepages`` sysctl to a
>  smaller value."
>
> So, the documentation implies that these interfaces should display the
> number of persistent hugetlb pages.  As you have discovered, all but the
> sysctl interface (and /proc/sys/vm/nr_hugepages) displays the total
> number of hugetlb pages rather than the number of persistent hugetlb
> pages.
>
> If we wanted to match the documentation, it seems we should change all
> the "show" interfaces to display persistent huge pages.  However, I am a
> bit concerned about how this may impact end users.
>
> There are two types if inconsistencies in these interfaces.
> 1) As this patch points out, not all "show" interfaces provide the same
>    information.  sysctl (/proc/sys/vm/nr_hugepages) displays the number
>    of persistent hugetlb pages, while the others display the total number
>    of hugetlb pages.
> 2) The show/read interfaces generally provide the total number of
>    hugetlb pages, and the update/write interfaces update the number of
>    persistent hugetlb pages.
>
> Both of these situations can lead to user confusion.  My 'guess' is that
> this has not been a widespread issue as most hugetlb users do not
> configure overcommit/surplus hugetlb pages and thus total number of
> hugetlb pages is the same as number of persistent hugetlb pages.
>
> Right now, I would suggest making all these interfaces display/take the
> number of persistent hugetlb pages for consistency.  This also matches
> the documentation.
>
> Thoughts?
I am concerned that modifying it this way may result in an weaker control
over hugetlb pages. Administrator will no longer be able to increase
surplus pages through the nr_hugepages interface.

Since surplus pages depend on the state of programs in the entire
system, adjusting nr_hugepages may lead to an unexpected number of
hugetlbs allocated which may leads to oom.

About the definition of /proc/sys/vm/nr_huge_pages and meaning of
"persistent", the documentation is kind of ambiguous.

The documentation says:

"The ``/proc/meminfo`` file provides information about the total number of
persistent hugetlb pages in the kernel's huge page pool."

"Caveat: Shrinking the persistent huge page pool via ``nr_hugepages``
such that it becomes less than the number of huge pages in use will
convert the balance of the in-use huge pages to surplus huge pages."

"The ``/proc`` interfaces discussed above have been retained for backwards
compatibility."

The ambiguities are:
1. HugePages_Total in /proc/meminfo is actually the total number of
hugetlb pages.
2. If nr_hugepages means persistent hugetlb pages, converting in-use huge
pages to surplus huge pages is impossible.
3. As you know, backward compatibility is not retained.

Given that the document needs to be modified anyway, why not make the
interface more user-friendly?

Thanks,
Hu
> --
> Mike Kravetz
>
> >
> > Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
> > ---
> >  mm/hugetlb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index e327a5a7602c..76af189053f0 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -4606,7 +4606,7 @@ static int hugetlb_sysctl_handler_common(bool obey_mempolicy,
> >                        void *buffer, size_t *length, loff_t *ppos)
> >  {
> >       struct hstate *h = &default_hstate;
> > -     unsigned long tmp = h->max_huge_pages;
> > +     unsigned long tmp = h->nr_huge_pages;
> >       int ret;
> >
> >       if (!hugepages_supported())
> > --
> > 2.40.1
> >
> >