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
