| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YqgL0ZcU5dhhO5uL@FVFYT0MHHV2J.usts.net>
Date: Tue, 14 Jun 2022 12:17:21 +0800
From: Muchun Song <songmuchun@...edance.com>
To: Mike Kravetz <mike.kravetz@...cle.com>
Cc: Oscar Salvador <osalvador@...e.de>, david@...hat.com,
akpm@...ux-foundation.org, corbet@....net, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 6/6] mm: hugetlb_vmemmap: improve hugetlb_vmemmap code
readability
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:
> > > > -static __init int hugetlb_vmemmap_sysctls_init(void)
> > > > +static int __init hugetlb_vmemmap_init(void)
> > > > {
> > > > + const struct hstate *h;
> > > > + bool optimizable = false;
> > > > +
> > > > /*
> > > > - * If "struct page" crosses page boundaries, the vmemmap pages cannot
> > > > - * be optimized.
> > > > + * There are only (RESERVE_VMEMMAP_SIZE / sizeof(struct page)) struct
> > > > + * page structs that can be used when HVO is enabled.
> > > > */
> > > > - if (is_power_of_2(sizeof(struct page)))
> > > > - register_sysctl_init("vm", hugetlb_vmemmap_sysctls);
> > > > + BUILD_BUG_ON(__NR_USED_SUBPAGE >= RESERVE_VMEMMAP_SIZE / sizeof(struct page));
> > >
> > > I need to take another look, but from the first glance there is something
> > > here that caught my eye.
> > >
> >
> > Thanks for taking a look. This is introduced in commit f41f2ed43ca5.
> >
> > > > +
> > > > + for_each_hstate(h) {
> > > > + char buf[16];
> > > > + unsigned int size = 0;
> > > > +
> > > > + if (hugetlb_vmemmap_optimizable(h))
> > > > + size = hugetlb_vmemmap_size(h) - RESERVE_VMEMMAP_SIZE;
> > > > + optimizable = size ? true : optimizable;
> > >
> > > This feels weird, just use false instead of optimizable.
> > >
> >
> > This is a loop, we shoud keep "optimizable" as "true" as long as there is one
> > hstate is optimizable. How about:
> >
> > if (size)
> > optimizable = true;
> >
> > > > + string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf,
> > > > + sizeof(buf));
> > > > + pr_info("%d KiB vmemmap can be optimized for a %s page\n",
> > > > + size / SZ_1K, buf);
> > >
> > > I do not have a strong opinion but I wonder whether this brings a lot.
> > >
> >
> > I thought the users can know what size HugeTLB is optimizable via
> > this log. E.g. On aarch64, 64KB HugeTLB cannot be optimizable.
> > I do not have a strong opinion as well, if anyone think it is
> > unnecessary, I'll drop it in next version.
>
> I do not have a strong opinion. I think it adds a little information. For me,
> the new logging of number of pages vmemmap optimized at boot seems a bit
> redundant. Here is a BEFORE/AFTER comparison.
>
Well, I'll drop the "new logging".
> 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
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.
Thanks.
> Not insisting on these changes. Just wanted to share the ideas.
>
>
> Overall, the code improvements look good.
> --
> Mike Kravetz
>
Powered by blists - more mailing lists