[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yqb89waW/jcsgRgo@FVFYT0MHHV2J.usts.net>
Date: Mon, 13 Jun 2022 17:01:43 +0800
From: Muchun Song <songmuchun@...edance.com>
To: Oscar Salvador <osalvador@...e.de>
Cc: mike.kravetz@...cle.com, 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 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.
Thanks.
Powered by blists - more mailing lists