linux-kernel - Re: [PATCH RESEND v2 2/4] mm: hugetlb: replace hugetlb_free_vmemmap_enabled with a static

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 18 Sep 2021 19:47:38 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Barry Song <21cnbao@...il.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oscar Salvador <osalvador@...e.de>,
        Michal Hocko <mhocko@...e.com>,
        Barry Song <song.bao.hua@...ilicon.com>,
        David Hildenbrand <david@...hat.com>,
        Chen Huang <chenhuang5@...wei.com>,
        "Bodeddula, Balasubramaniam" <bodeddub@...zon.com>,
        Jonathan Corbet <corbet@....net>,
        Matthew Wilcox <willy@...radead.org>,
        Xiongchun duan <duanxiongchun@...edance.com>,
        fam.zheng@...edance.com, Muchun Song <smuchun@...il.com>,
        Qi Zheng <zhengqi.arch@...edance.com>,
        linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH RESEND v2 2/4] mm: hugetlb: replace hugetlb_free_vmemmap_enabled
 with a static_key

On Sat, Sep 18, 2021 at 7:15 PM Barry Song <21cnbao@...il.com> wrote:
>
> On Sat, Sep 18, 2021 at 10:31 PM Muchun Song <songmuchun@...edance.com> wrote:
> >
> > On Sat, Sep 18, 2021 at 12:55 PM Barry Song <21cnbao@...il.com> wrote:
> > >
> > > On Sat, Sep 18, 2021 at 12:08 AM Muchun Song <songmuchun@...edance.com> wrote:
> > > >
> > > > The page_head_if_fake() is used throughout memory management and the
> > > > conditional check requires checking a global variable, although the
> > > > overhead of this check may be small, it increases when the memory
> > > > cache comes under pressure. Also, the global variable will not be
> > > > modified after system boot, so it is very appropriate to use static
> > > > key machanism.
> > > >
> > > > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > > > ---
> > > >  include/linux/hugetlb.h    |  6 +++++-
> > > >  include/linux/page-flags.h |  6 ++++--
> > > >  mm/hugetlb_vmemmap.c       | 10 +++++-----
> > > >  3 files changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > > > index f7ca1a3870ea..ee3ddf3d12cf 100644
> > > > --- a/include/linux/hugetlb.h
> > > > +++ b/include/linux/hugetlb.h
> > > > @@ -1057,7 +1057,11 @@ static inline void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr
> > > >  #endif /* CONFIG_HUGETLB_PAGE */
> > > >
> > > >  #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> > > > -extern bool hugetlb_free_vmemmap_enabled;
> > > > +DECLARE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
> > > > +                        hugetlb_free_vmemmap_enabled_key);
> > > > +#define hugetlb_free_vmemmap_enabled                                    \
> > > > +       static_key_enabled(&hugetlb_free_vmemmap_enabled_key)
> > > > +
> > > >  #else
> > > >  #define hugetlb_free_vmemmap_enabled   false
> > > >  #endif
> > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > > > index 7b1a918ebd43..d68d2cf30d76 100644
> > > > --- a/include/linux/page-flags.h
> > > > +++ b/include/linux/page-flags.h
> > > > @@ -185,7 +185,8 @@ enum pageflags {
> > > >  #ifndef __GENERATING_BOUNDS_H
> > > >
> > > >  #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> > > > -extern bool hugetlb_free_vmemmap_enabled;
> > > > +DECLARE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
> > > > +                        hugetlb_free_vmemmap_enabled_key);
> > > >
> > > >  /*
> > > >   * If the feature of freeing some vmemmap pages associated with each HugeTLB
> > > > @@ -204,7 +205,8 @@ extern bool hugetlb_free_vmemmap_enabled;
> > > >   */
> > > >  static __always_inline const struct page *page_head_if_fake(const struct page *page)
> > > >  {
> > > > -       if (!hugetlb_free_vmemmap_enabled)
> > > > +       if (!static_branch_maybe(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
> > > > +                                &hugetlb_free_vmemmap_enabled_key))
> > >
> > > A question bothering me is that we still have hugetlb_free_vmemmap_enabled
> > > defined as static_key_enabled(&hugetlb_free_vmemmap_enabled_key).
> > > but here you are using static_branch_maybe() with the CONFIG and refer the key
> > > directly.
> > > Do we only need one of them? Or something is wrong?
> > >
> >
> > Yeah, we only need one. But my consideration is that we
> > use static_branch_maybe() for performance sensitive places.
> > So I do not change hugetlb_free_vmemmap_enabled
> > to static_branch_maybe(), this can reduce some codes
> > that need to be updated when the static key is enabled.
> > Actually, the user of hugetlb_free_vmemmap_enabled
> > is not performance sensitive.
>
> not quite sure if an unified inline API will be better, e.g.
>
> #ifdef CONFIG_SCHED_SMT
> extern struct static_key_false sched_smt_present;
>
> static __always_inline bool sched_smt_active(void)
> {
>         return static_branch_likely(&sched_smt_present);
> }
> #else
> static inline bool sched_smt_active(void) { return false; }
> #endif

Alright, I can change hugetlb_free_vmemmap_enabled to
an inline function.

>
> but in your case, CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> is always true in your page_head_if_fake(). Why do we check it
> again?

That is CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON
not CONFIG_HUGETLB_PAGE_FREE_VMEMMAP.

Thanks