linux-kernel - Re: [RESEND PATCH v3] arm64: enable THP

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] [day] [month] [year] [list]

Message-ID: <20220720091716.GA15752@willie-the-truck>
Date:   Wed, 20 Jul 2022 10:17:16 +0100
From:   Will Deacon <will@...nel.org>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     Barry Song <21cnbao@...il.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        LAK <linux-arm-kernel@...ts.infradead.org>,
        Linux-MM <linux-mm@...ck.org>,
        Steven Price <steven.price@....com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        郭健 <guojian@...o.com>,
        hanchuanhua <hanchuanhua@...o.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Hugh Dickins <hughd@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Minchan Kim <minchan@...nel.org>,
        Yang Shi <shy828301@...il.com>,
        Barry Song <v-songbaohua@...o.com>,
        张诗明(Simon Zhang) 
        <zhangshiming@...o.com>
Subject: Re: [RESEND PATCH v3] arm64: enable THP_SWAP for arm64

On Wed, Jul 20, 2022 at 08:06:42AM +0530, Anshuman Khandual wrote:
> On 7/20/22 07:16, Barry Song wrote:
> > On Tue, Jul 19, 2022 at 4:04 PM Anshuman Khandual
> > <anshuman.khandual@....com> wrote:
> >> On 7/19/22 09:29, Barry Song wrote:
> >>> On Tue, Jul 19, 2022 at 3:35 PM Anshuman Khandual
> >>> <anshuman.khandual@....com> wrote:
> >>>> On 7/19/22 08:58, Huang, Ying wrote:
> >>>>> Anshuman Khandual <anshuman.khandual@....com> writes:
> >>>>>>>> How about the following?
> >>>>>>>>
> >>>>>>>> static inline bool arch_wants_thp_swap(void)
> >>>>>>>> {
> >>>>>>>>      return IS_ENABLED(ARCH_WANTS_THP_SWAP);
> >>>>>>>> }
> >>>>>>>
> >>>>>>> This looks good. then i'll need to change arm64 to
> >>>>>>>
> >>>>>>>  +static inline bool arch_thp_swp_supported(void)
> >>>>>>>  +{
> >>>>>>>  +     return IS_ENABLED(ARCH_WANTS_THP_SWAP) &&  !system_supports_mte();
> >>>>>>>  +}
> >>>>>>
> >>>>>> Why ? CONFIG_THP_SWAP depends on ARCH_WANTS_THP_SWAP. In folio_alloc_swap(),
> >>>>>> IS_ENABLED(CONFIG_THP_SWAP) enabled, will also imply ARCH_WANTS_THP_SWAP too
> >>>>>> is enabled. Hence checking for ARCH_WANTS_THP_SWAP again does not make sense
> >>>>>> either in the generic fallback stub, or in arm64 platform override. Because
> >>>>>> without ARCH_WANTS_THP_SWAP enabled, arch_thp_swp_supported() should never
> >>>>>> be called in the first place.
> >>>>>
> >>>>> For the only caller now, the checking looks redundant.  But the original
> >>>>> proposed implementation as follows,
> >>>>>
> >>>>> static inline bool arch_thp_swp_supported(void)
> >>>>> {
> >>>>>      return true;
> >>>>> }
> >>>>>
> >>>>> will return true even on architectures that don't support/want THP swap.
> >>>>
> >>>> But the function will never be called on for those platforms.
> >>>>
> >>>>> That will confuse people too.
> >>>>
> >>>> I dont see how.
> >>>>
> >>>>>
> >>>>> And the "redundant" checking has no run time overhead, because compiler
> >>>>> will do the trick.
> >>>> I understand that, but dont think this indirection is necessary.
> >>>
> >>> Hi Anshuman, Hi Ying,
> >>> Thanks for the comments of both of you. Does the below look ok?
> >>>
> >>> generic,
> >>>
> >>>  static inline bool arch_wants_thp_swap(void)
> >>>   {
> >>>       return IS_ENABLED(CONFIG_THP_SWAP);
> >>>  }
> >>>
> >>> arm64,
> >>>
> >>> static inline bool arch_thp_swp_supported(void)
> >>> {
> >>>      return IS_ENABLED(CONFIG_THP_SWAP) &&  !system_supports_mte();
> >>> }
> >>>
> >>> caller,
> >>>
> >>> folio_alloc_swap(struct folio *folio)
> >>> {
> >>>
> >>>   if (folio_test_large(folio)) {
> >>>    - if (IS_ENABLED(CONFIG_THP_SWAP))
> >>>   + if (arch_thp_swp_supported())
> >>>         get_swap_pages(1, &entry, folio_nr_pages(folio));
> >>>        goto out;
> >>>   }
> >>
> >> Current proposal in this patch LGTM, I dont see any reason for these changes.
> > 
> > OK, thanks, Anshuman. Can I collect this as a Reviewed-by?
> 
> Yes please.
>
> Reviewed-by: Anshuman Khandual <anshuman.khandual@....com>

I've lost track of exactly what the outcome here is, so Barry, please can
you send a final version of the agreed-upon patch?

Thanks,

Will