[PATCH 0/3] mm/hugetlb: fix /sys and /proc fs dealing with persistent hugepages

[PATCH 0/3] mm/hugetlb: fix /sys and /proc fs dealing with persistent hugepages

[Xueshi Hu]

Hi,

The interfaces provided by /sys and /proc for handling huge pages do not 
adequately address the meaning of persistent. Luckily, this can be easily
resolved with minor modifications. Additionally, code relevant with 
hstate:max_huge_pages can be simplified.

Thanks,
Hu

Xueshi Hu (3):
  mm/hugetlb: fix the inconsistency of /proc/sys/vm/nr_huge_pages
  mm/hugeltb: clean up hstate::max_huge_pages
  mm/hugeltb: fix nodes huge page allocation when there are surplus
    pages

 fs/hugetlbfs/inode.c |  2 +-
 mm/hugetlb.c         | 30 +++++++++---------------------
 2 files changed, 10 insertions(+), 22 deletions(-)

-- 
2.40.1

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

[Xueshi Hu]

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.

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

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

[Mike Kravetz]

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?
-- 
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
> 
> 

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

[Xueshi Hu]

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
> >
> >

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

[Mike Kravetz]

On 08/01/23 20:22, Xueshi Hu wrote:
> 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.

Sorry, I am not sure I understand your concerns.

Currently, the interfaces to set/update the number of hugetlb pages use
the supplied count as the number of requested persistent pages.  I am
not suggesting any changes there (except the bug in node specific code
you discovered).  Rather, I am suggesting that we update the interfaces
which show the number of hugepages (nr_hugepages) to display the number
of persistent pages to be consistent with the set/update interfaces.

> 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.

Correct.  Although the documentation states it is the number of
persistent hugetlb pages.  meminfo also contains the number of surplus
huge pages.  So, it it possible that one could see

HugePages_Total: 0
HugePages_Surp:  100

Ideally, one would want to know the value for overcommit hugepages as
well.

The sysfs directories /sys/kernel/mm/hugepages/hugepages-*/ contain both
the surplus and overcommit counts.

node specific sysfs directories only contain surplus counts.

> 2. If nr_hugepages means persistent hugetlb pages, converting in-use huge
> pages to surplus huge pages is impossible.

I am not sure I understand.  When writing to nr_hugepages today, it does
mean persistent hugetlb pages.  Are you suggesting we change it to mean
total hugetlb pages when writing/updating?  I do not think that is the
case, as none of your proposed changes do this.

> 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?

In any case, I agree the document should be updated to match the code.
It should also define persistent hugetlb pages.

Thank you,
-- 
Mike Kravetz

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

[Xueshi Hu]

On Tue, Aug 01, 2023 at 11:49:42AM -0700, Mike Kravetz wrote:
> On 08/01/23 20:22, Xueshi Hu wrote:
> > 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.
> 
> Sorry, I am not sure I understand your concerns.
I'm wrong, just ignore what I've said.
> 
> Currently, the interfaces to set/update the number of hugetlb pages use
> the supplied count as the number of requested persistent pages.  I am
> not suggesting any changes there (except the bug in node specific code
> you discovered).  Rather, I am suggesting that we update the interfaces
> which show the number of hugepages (nr_hugepages) to display the number
> of persistent pages to be consistent with the set/update interfaces.
I agree with you.
> 
> > 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.
> 
> Correct.  Although the documentation states it is the number of
> persistent hugetlb pages.  meminfo also contains the number of surplus
> huge pages.  So, it it possible that one could see
> 
> HugePages_Total: 0
> HugePages_Surp:  100
It's easy to fix.
> 
> Ideally, one would want to know the value for overcommit hugepages as
> well.
It will be straightforward to achieve this.
> 
> The sysfs directories /sys/kernel/mm/hugepages/hugepages-*/ contain both
> the surplus and overcommit counts.
> 
> node specific sysfs directories only contain surplus counts.
Node specific sysfs directories don't contain resv_hugepages too.
After resolving this issue, I will attempt to assess the feasibility
about how to implement node-specific reservations and overcommitment.
> 
> > 2. If nr_hugepages means persistent hugetlb pages, converting in-use huge
> > pages to surplus huge pages is impossible.
> 
> I am not sure I understand.  When writing to nr_hugepages today, it does
> mean persistent hugetlb pages.  Are you suggesting we change it to mean
> total hugetlb pages when writing/updating?  I do not think that is the
> case, as none of your proposed changes do this.
Still, I'm wrong.
> 
> > 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?
> 
> In any case, I agree the document should be updated to match the code.
> It should also define persistent hugetlb pages.
Yes, I'll add it in the v2 patch.
> 
> Thank you,
> -- 
> Mike Kravetz

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

[Mike Kravetz]

On 08/02/23 15:31, Xueshi Hu wrote:
> On Tue, Aug 01, 2023 at 11:49:42AM -0700, Mike Kravetz wrote:
> > On 08/01/23 20:22, Xueshi Hu wrote:
> > > 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.
> > 
> > Sorry, I am not sure I understand your concerns.
> I'm wrong, just ignore what I've said.
> > 
> > Currently, the interfaces to set/update the number of hugetlb pages use
> > the supplied count as the number of requested persistent pages.  I am
> > not suggesting any changes there (except the bug in node specific code
> > you discovered).  Rather, I am suggesting that we update the interfaces
> > which show the number of hugepages (nr_hugepages) to display the number
> > of persistent pages to be consistent with the set/update interfaces.
> I agree with you.
> > 
> > > 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.
> > 
> > Correct.  Although the documentation states it is the number of
> > persistent hugetlb pages.  meminfo also contains the number of surplus
> > huge pages.  So, it it possible that one could see
> > 
> > HugePages_Total: 0
> > HugePages_Surp:  100
> It's easy to fix.
> > 
> > Ideally, one would want to know the value for overcommit hugepages as
> > well.
> It will be straightforward to achieve this.
> > 
> > The sysfs directories /sys/kernel/mm/hugepages/hugepages-*/ contain both
> > the surplus and overcommit counts.
> > 
> > node specific sysfs directories only contain surplus counts.
> Node specific sysfs directories don't contain resv_hugepages too.
> After resolving this issue, I will attempt to assess the feasibility
> about how to implement node-specific reservations and overcommitment.
> > 
> > > 2. If nr_hugepages means persistent hugetlb pages, converting in-use huge
> > > pages to surplus huge pages is impossible.
> > 
> > I am not sure I understand.  When writing to nr_hugepages today, it does
> > mean persistent hugetlb pages.  Are you suggesting we change it to mean
> > total hugetlb pages when writing/updating?  I do not think that is the
> > case, as none of your proposed changes do this.
> Still, I'm wrong.
> > 
> > > 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?
> > 
> > In any case, I agree the document should be updated to match the code.
> > It should also define persistent hugetlb pages.
> Yes, I'll add it in the v2 patch.

I did not trim previous conversations because I wanted to keep all context.

In summary, I do not believe we want to change the meaning of the value
used to write/update the number of hugetlb pages.  That currently means
the number of persistent pages(except in the case of the bug in node
specific interfaces).

All of the read/display interfaces provide the total number of hugetlb
pages (except sysctl addressed by this patch).

Moving forward, I do not think we want to change the write/update interfaces.
Doing so might cause unexpected different behavior for existing users.

The question is 'Should we change all read/display interfaces to provide
the number of persistent hugetlb pages?  Or, should we leave them
displaying total number of hugetlb pages?'

The Documentation says the interfaces should display the persistent value.
However, the documentation does not define persistent value and we have
instead been displaying the total value for a long time.  Upon further thought,
I think we should just continue displaying total hugetlb page count.  We
should also update the documentation to match the code.

I believe a good strategy is to not change existing interfaces unless there
is a bug or very good reason.  In this case, there is no good reason.

In summary, I think we should:
- Update the sysctl read interface to display total number of hugetlb
  pages as in this patch.
- Fix the bug in node specific write/update interface (patch 3).
- Update documentation to match code.
- Not absolutely necessary, but code cleanup as in patch 2 (or something
  similar) is OK.

Sorry for going back to agreeing with most of this patch series.  But, I
think it was good to discuss the details about the best way to move forward.
-- 
Mike Kravetz

[PATCH 2/3] mm/hugeltb: clean up hstate::max_huge_pages

[Xueshi Hu]

Presently, the sole use case of hstate::max_huge_pages is confined to
hugetlb_sysctl_handler_common() and hugetlbfs_size_to_hpages().
The former has been replaced with hstate::nr_huge_pages, while the latter
can be effortlessly substituted.

After hugeltb subsystem has been initialized, hstate::max_huge_pages
always equals to persistent_huge_pages(). It's a burden to maintain
the equation[1][2].

After this patch, hstate::max_huge_pages is only used in kernel command
line parameter parsing.

Renaming set_max_huge_pages() to set_nr_huge_pages() would enhance the
readability of the code.

[1]: Commit a43a83c79b4f ("mm/hugetlb: fix incorrect update of
max_huge_pages")
[2]: Commit c1470b33bb6e ("mm/hugetlb: fix incorrect hugepages count
during mem hotplug")

Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
---
 fs/hugetlbfs/inode.c |  2 +-
 mm/hugetlb.c         | 24 +++++-------------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 316c4cebd3f3..cd1a3e4bf8fb 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1375,7 +1375,7 @@ hugetlbfs_size_to_hpages(struct hstate *h, unsigned long long size_opt,
 
 	if (val_type == SIZE_PERCENT) {
 		size_opt <<= huge_page_shift(h);
-		size_opt *= h->max_huge_pages;
+		size_opt *= (h->nr_huge_pages - h->surplus_huge_pages);
 		do_div(size_opt, 100);
 	}
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 76af189053f0..56647235ab21 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2343,14 +2343,13 @@ int dissolve_free_huge_page(struct page *page)
 		}
 
 		remove_hugetlb_folio(h, folio, false);
-		h->max_huge_pages--;
 		spin_unlock_irq(&hugetlb_lock);
 
 		/*
 		 * Normally update_and_free_hugtlb_folio will allocate required vmemmmap
 		 * before freeing the page.  update_and_free_hugtlb_folio will fail to
 		 * free the page if it can not allocate required vmemmap.  We
-		 * need to adjust max_huge_pages if the page is not freed.
+		 * need to adjust nr_huge_pages if the page is not freed.
 		 * Attempt to allocate vmemmmap here so that we can take
 		 * appropriate action on failure.
 		 */
@@ -2360,7 +2359,6 @@ int dissolve_free_huge_page(struct page *page)
 		} else {
 			spin_lock_irq(&hugetlb_lock);
 			add_hugetlb_folio(h, folio, false);
-			h->max_huge_pages++;
 			spin_unlock_irq(&hugetlb_lock);
 		}
 
@@ -3274,8 +3272,6 @@ static void __init hugetlb_hstate_alloc_pages_onenode(struct hstate *h, int nid)
 	string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
 	pr_warn("HugeTLB: allocating %u of page size %s failed node%d.  Only allocated %lu hugepages.\n",
 		h->max_huge_pages_node[nid], buf, nid, i);
-	h->max_huge_pages -= (h->max_huge_pages_node[nid] - i);
-	h->max_huge_pages_node[nid] = i;
 }
 
 static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
@@ -3336,7 +3332,6 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
 		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
 		pr_warn("HugeTLB: allocating %lu of page size %s failed.  Only allocated %lu hugepages.\n",
 			h->max_huge_pages, buf, i);
-		h->max_huge_pages = i;
 	}
 	kfree(node_alloc_noretry);
 }
@@ -3460,7 +3455,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 }
 
 #define persistent_huge_pages(h) (h->nr_huge_pages - h->surplus_huge_pages)
-static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
+static int set_nr_huge_pages(struct hstate *h, unsigned long count, int nid,
 			      nodemask_t *nodes_allowed)
 {
 	unsigned long min_count, ret;
@@ -3601,7 +3596,6 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 			break;
 	}
 out:
-	h->max_huge_pages = persistent_huge_pages(h);
 	spin_unlock_irq(&hugetlb_lock);
 	mutex_unlock(&h->resize_lock);
 
@@ -3639,7 +3633,7 @@ static int demote_free_hugetlb_folio(struct hstate *h, struct folio *folio)
 	destroy_compound_hugetlb_folio_for_demote(folio, huge_page_order(h));
 
 	/*
-	 * Taking target hstate mutex synchronizes with set_max_huge_pages.
+	 * Taking target hstate mutex synchronizes with set_nr_huge_pages.
 	 * Without the mutex, pages added to target hstate could be marked
 	 * as surplus.
 	 *
@@ -3664,14 +3658,6 @@ static int demote_free_hugetlb_folio(struct hstate *h, struct folio *folio)
 
 	spin_lock_irq(&hugetlb_lock);
 
-	/*
-	 * Not absolutely necessary, but for consistency update max_huge_pages
-	 * based on pool changes for the demoted page.
-	 */
-	h->max_huge_pages--;
-	target_hstate->max_huge_pages +=
-		pages_per_huge_page(h) / pages_per_huge_page(target_hstate);
-
 	return rc;
 }
 
@@ -3770,13 +3756,13 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
 	} else {
 		/*
 		 * Node specific request.  count adjustment happens in
-		 * set_max_huge_pages() after acquiring hugetlb_lock.
+		 * set_nr_huge_pages() after acquiring hugetlb_lock.
 		 */
 		init_nodemask_of_node(&nodes_allowed, nid);
 		n_mask = &nodes_allowed;
 	}
 
-	err = set_max_huge_pages(h, count, nid, n_mask);
+	err = set_nr_huge_pages(h, count, nid, n_mask);
 
 	return err ? err : len;
 }
-- 
2.40.1

[PATCH 3/3] mm/hugeltb: fix nodes huge page allocation when there are surplus pages

[Xueshi Hu]

In set_nr_huge_pages(), local variable "count" is used to record
persistent_huge_pages(), but when it cames to nodes huge page allocation,
the semantics changes to nr_huge_pages. When there exists surplus huge
pages and using the interface under
/sys/devices/system/node/node*/hugepages to change huge page pool size,
this difference can result in the allocation of an unexpected number of
huge pages.

Steps to reproduce the bug:

Starting with:

				  Node 0          Node 1    Total
	HugePages_Total             0.00            0.00     0.00
	HugePages_Free              0.00            0.00     0.00
	HugePages_Surp              0.00            0.00     0.00

create 100 huge pages in Node 0 and consume it, then set Node 0 's
nr_hugepages to 0.

yields:

				  Node 0          Node 1    Total
	HugePages_Total           200.00            0.00   200.00
	HugePages_Free              0.00            0.00     0.00
	HugePages_Surp            200.00            0.00   200.00

write 100 to Node 1's nr_hugepages

		echo 100 > /sys/devices/system/node/node1/\
	hugepages/hugepages-2048kB/nr_hugepages

gets:

				  Node 0          Node 1    Total
	HugePages_Total           200.00          400.00   600.00
	HugePages_Free              0.00          400.00   400.00
	HugePages_Surp            200.00            0.00   200.00

Kernel is expected to create only 100 huge pages and it gives 200.

Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
---
 mm/hugetlb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 56647235ab21..8ed4fffdebda 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3490,7 +3490,9 @@ static int set_nr_huge_pages(struct hstate *h, unsigned long count, int nid,
 	if (nid != NUMA_NO_NODE) {
 		unsigned long old_count = count;
 
-		count += h->nr_huge_pages - h->nr_huge_pages_node[nid];
+		count += persistent_huge_pages(h) -
+			 (h->nr_huge_pages_node[nid] -
+			  h->surplus_huge_pages_node[nid]);
 		/*
 		 * User may have specified a large count value which caused the
 		 * above calculation to overflow.  In this case, they wanted
-- 
2.40.1

Re: [PATCH 3/3] mm/hugeltb: fix nodes huge page allocation when there are surplus pages

[Mike Kravetz]

On 07/30/23 20:51, Xueshi Hu wrote:
> In set_nr_huge_pages(), local variable "count" is used to record
> persistent_huge_pages(), but when it cames to nodes huge page allocation,
> the semantics changes to nr_huge_pages. When there exists surplus huge
> pages and using the interface under
> /sys/devices/system/node/node*/hugepages to change huge page pool size,
> this difference can result in the allocation of an unexpected number of
> huge pages.
> 
> Steps to reproduce the bug:
> 
> Starting with:
> 
> 				  Node 0          Node 1    Total
> 	HugePages_Total             0.00            0.00     0.00
> 	HugePages_Free              0.00            0.00     0.00
> 	HugePages_Surp              0.00            0.00     0.00
> 
> create 100 huge pages in Node 0 and consume it, then set Node 0 's
> nr_hugepages to 0.
> 
> yields:
> 
> 				  Node 0          Node 1    Total
> 	HugePages_Total           200.00            0.00   200.00
> 	HugePages_Free              0.00            0.00     0.00
> 	HugePages_Surp            200.00            0.00   200.00
> 
> write 100 to Node 1's nr_hugepages
> 
> 		echo 100 > /sys/devices/system/node/node1/\
> 	hugepages/hugepages-2048kB/nr_hugepages
> 
> gets:
> 
> 				  Node 0          Node 1    Total
> 	HugePages_Total           200.00          400.00   600.00
> 	HugePages_Free              0.00          400.00   400.00
> 	HugePages_Surp            200.00            0.00   200.00
> 
> Kernel is expected to create only 100 huge pages and it gives 200.
> 
> Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
> ---
>  mm/hugetlb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Good catch!

I added the code modified in this patch with commit fd875dca7c717.  However,
my commit moved the specific line that was the root case of the bug.  That
specific line was added with the commit 9a30523066cde that added hugetlb
node specific support way back in 2009 (2.6.32 timeframe).

Fix looks good, but waiting on resolution of max_huge_pages usage.
-- 
Mike Kravetz
  
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 56647235ab21..8ed4fffdebda 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3490,7 +3490,9 @@ static int set_nr_huge_pages(struct hstate *h, unsigned long count, int nid,
>  	if (nid != NUMA_NO_NODE) {
>  		unsigned long old_count = count;
>  
> -		count += h->nr_huge_pages - h->nr_huge_pages_node[nid];
> +		count += persistent_huge_pages(h) -
> +			 (h->nr_huge_pages_node[nid] -
> +			  h->surplus_huge_pages_node[nid]);
>  		/*
>  		 * User may have specified a large count value which caused the
>  		 * above calculation to overflow.  In this case, they wanted
> -- 
> 2.40.1
> 
>