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

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

On 8/26/23 05:15, Mike Kravetz wrote:
> 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?
Thanks for your patient reply.
> 
> 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.
A modified version will sent later.
> 
> Patch 3 is documentation updates which we agree need to be performed.  I can
> assist here if you would like.
I would greatly appreciate your assistance.
> 
> Patch 4 is most critical as it is a bug fix.  Perhaps this can/should be sent
> separately.
Ok, I'll send it separately.

Thanks,
Hu