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

[Muchun Song <songmuchun@bytedance.com>]

On Tue, Jun 14, 2022 at 09:57:35AM -0700, Mike Kravetz wrote:
> 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 am sure we are on the same page. This looks good to me.

> 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.
> 

Make sense. I'll do it in v2.

Thanks.