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