Re: [PATCH 6/6] mm: hugetlb_vmemmap: improve hugetlb_vmemmap code readability

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

On Tue, Jun 14, 2022 at 12:17:21PM +0800”, Muchun Song wrote:
> On Mon, Jun 13, 2022 at 05:22:30PM -0700, Mike Kravetz wrote:
> > On Mon, Jun 13, 2022 at 05:01:43PM +0800”, Muchun Song wrote:
> > > On Mon, Jun 13, 2022 at 10:33:48AM +0200, Oscar Salvador wrote:
> > > > On Mon, Jun 13, 2022 at 02:35:12PM +0800, Muchun Song wrote:
>  
> > BEFORE
> > ------
> > [    0.000000] Command line: BOOT_IMAGE=(hd0,msdos1)/vmlinuz-5.19.0-rc1-next-20220610+ root=UUID=49c13301-2555-44dc-847b-caabe1d62bdf ro console=tty0 console=ttyS0,115200 audit=0 transparent_hugepage=always hugetlb_free_vmemmap=on hugepages=512
> > ...
> > [    0.330930] HugeTLB: can optimize 4095 vmemmap pages for hugepages-1048576kB
> > [    0.350450] HugeTLB: can optimize 7 vmemmap pages for hugepages-2048kB
> > [    0.359282] HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
> > [    0.359285] HugeTLB registered 2.00 MiB page size, pre-allocated 512 pages
> > 
> > AFTER
> > -----
> > [    0.000000] Command line: BOOT_IMAGE=(hd0,msdos1)/vmlinuz-5.19.0-rc1-next-20220610+ root=UUID=49c13301-2555-44dc-847b-caabe1d62bdf ro console=tty0 console=ttyS0,115200 audit=0 transparent_hugepage=always hugetlb_free_vmemmap=on hugepages=512
> > ...
> > [    0.409068] HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
> > [    0.409071] HugeTLB registered 2.00 MiB page size, pre-allocated 512 pages
> > [    1.246107] HugeTLB: 16380 KiB vmemmap can be optimized for a 1.00 GiB page
> > [    1.246110] HugeTLB: 28 KiB vmemmap can be optimized for a 2.00 MiB page
> > [    1.246123] HugeTLB: 512 huge pages whose vmemmap are optimized at boot
> > 
> > When I read those messages, I am not sure if 'optimized' is the best
> > word to use.  I know that using alloc/free throughout the code was
> > confusing.  But, wouldn't it perhaps be more clear to the end user if
> > the messages read?
> 
> Well, I agree with you at least. "free" may be more friendly to the end
> users.  I'll change the word "optimized" to "freed".
> 
> > 
> > HugeTLB: 16380 KiB vmemmap can be freed for a 1.00 GiB page
> > 
> > Also, how about having report_hugepages() call a routine that prints the
> > vmemmmap savings.  Then output could then look something like:
> > 
> > HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
> > 	 16380 KiB vmemmap can be freed for a 1.00 GiB page
> > HugeTLB: registered 2.00 MiB page size, pre-allocated 512 pages
> > 	 28 KiB vmemmap can be free for a 2.00 MiB page
> >
> 
> Well, we eliminate the prefix "HugeTLB:" for memory saving log.
> Maybe it is not a good choice since it it not easy to grep the log
> (e.g. dmesg | grep "HugeTLB" will not show saving log).  If

Agree, we should not drop the HugeTLB prefix from the message.

> we combine both 2-line log into one line, the log becomes a bit long.
> So I'd like to not eliminate the prefix but gather this 2-line log
> into one place. I mean "HugeTLB: registered 1.00 GiB page size,
> pre-allocated 0 pages" is just followed by "HugeTLB: 28 KiB vmemmap
> can be freed for a 2.00 MiB page" without any log insertion in
> between. But I have no strong opinion do this, I'd likt to listen
> to your opinion before making decision to do those changes.

I think we are talking about the same thing.  Did you make a mistake when
saying amount of vmemmap freed for 2MB page followed 1GB hstate size?

My thought was that it would be good to log the vmemmap savings near the
message that notes the hstate/huge page size and any precallocation.
Just my opinion.  So messages would look like:

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 16380 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 2.00 MiB page size, pre-allocated 512 pages
HugeTLB: 28 KiB vmemmap can be free for a 2.00 MiB page

I really do not have strong opinions on this.  However, if we are going
to change the messages we should try to make them as useful and clear as
possible.
-- 
Mike Kravetz