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

[Mike Kravetz <mike.kravetz@oracle.com>]

On 08/25/23 12:02, Xueshi Hu wrote:
> On 8/10/23 15:34, David Hildenbrand wrote:
> > On 10.08.23 02:17, Mike Kravetz wrote:
> > > On 08/08/23 17:13, Xueshi Hu wrote:
> > > > On 8/8/23 15:58, David Hildenbrand wrote:
> > > > > On 08.08.23 04:28, Xueshi Hu wrote:
> > > > > > On 8/7/23 23:15, David Hildenbrand wrote:
> > > > > > > On 06.08.23 09:48, Xueshi Hu wrote:
> > > 
> > > Sorry for jumping in late, I was away for a while.
> > > 
> > > Hu and myself discussed this previously in,
> > > https://lore.kernel.org/linux-mm/20230802182031.GA4762@monkey/T/#r1bdc8eeebafa08699fda5b15f247f3f966ddd090
> > > 
> > > The documentation around what is displayed with the hugetlb proc/sys
> > > interfaces is at best confusing and at worst wrong in places.
> > > 
> > > One source of confusion is use of term 'persistent hugetlb pages'.  The
> > > documentation does not define this term.  However, there is this
> > > definition in the code:
> > > #define persistent_huge_pages(h) (h->nr_huge_pages -
> > > h->surplus_huge_pages)
> > > 
> > > All of the write/update interfaces modify the number of persistent
> > > hugetlb
> > > pages as defined by the code (#define).  Only one read/show interface
> > > displays the number of persistent hugetlb pages as defined by the code
> > > (#define).  That is /proc/sys/vm/nr_hugepages (and sysctl).
> > 
> > Yes.
> > 
> > > 
> > > When thinking about this more, I am 'guessing' that when the
> > > documentation was
> > > originally written the term 'persistent hugetlb pages' did not refer
> > > to the
> > > #define in the code.  Rather, it was just the number of allocated
> > > hugetlb pages
> > > that 'persisted' until modified by the admin/user.
> > > 
> > > There is little doubt the documentation could/should be updated.
> > 
> > Absolutely.
> > 
> > > 
> > > The question is 'Should we change the /proc/sys/vm/nr_hugepages (and
> > > sysctl)
> > > interfaces to be consistent with all the other read/show interfaces?
> > > 
> > > The argument for changing is that consistency is good.  Why have one
> > > interface
> > > that is not like the others?
> > > 
> > > The reason for not changing is that this is the oldest interface.  The
> > > information/interfaces originally available in /proc were created in
> > > /sys.
> > > And, as mentioned in the documentation the /proc interfaces were kept
> > > for backward compatibility.  Unfortunately, the meaning of nr_hugepages
> > > was changed the /sys interfaces were created.  Sigh!!!
> > 
> > Indeed, they were designed to be different and to just leave the /proc
> > interface alone.
> > 
> > > 
> > > In the thread mentioned above, I was in agreement with Hu about changing
> > > /proc/sys/vm/nr_hugepages to be consistent with other read/show
> > > interfaces.
> > > Now, I am not sure.
> > 
> > My take would be to just leave /proc/sys/vm/nr_hugepages alone. Maybe
> > pr_warn_once() when the interface is used to guide people away from that
> > legacy interface + clarify the docs.
> > 
> > Your call. :)
> > 
> Considering /proc/sys/vm/nr_hugepages may be widely used, and it not total
> equivalent to the interfaces under /sys. What about just clarifying the
> docs?

I believe just updating the docs with clarification may be the best approach.
We need to say that /proc/sys/vm/nr_hugepages and sysctl (vm.nr_hugepages)
display the number of persistent hugetlb pages in the default pool.  And, we
should probably define 'persistent' as well.

With that, this patch should be dropped.  Without patch 1, patch 2 is not
necessary.  However, some cleanup (possible elimination) of max_huge_pages
could be done in the future.

Patch 3 is documentation updates which we agree need to be performed.  I can
assist here if you would like.

Patch 4 is most critical as it is a bug fix.  Perhaps this can/should be sent
separately.
-- 
Mike Kravetz