[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZP8QEERuihh33Oki@dhcp22.suse.cz>
Date: Mon, 11 Sep 2023 15:03:12 +0200
From: Michal Hocko <mhocko@...e.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
Zhaoyang Huang <huangzhaoyang@...il.com>, ke.wang@...soc.com
Subject: Re: [PATCH] mm: remove redundant clear page when
CONFIG_INIT_ON_ALLOC_DEFAULT_ON configured
On Mon 11-09-23 13:47:03, Matthew Wilcox wrote:
> On Mon, Sep 11, 2023 at 02:12:25PM +0200, Michal Hocko wrote:
> > On Mon 11-09-23 18:49:06, zhaoyang.huang wrote:
> > > From: Zhaoyang Huang <zhaoyang.huang@...soc.com>
> > >
> > > There will be redundant clear page within vma_alloc_zeroed_movable_folio
> > > when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on. Remove it by judging related
> > > configs.
> >
> > Thanks for spotting this. I suspect this is a fix based on a code review
> > rather than a real performance issue, right? It is always good to
> > mention that. From a very quick look it seems that many architectures
> > just definte vma_alloc_zeroed_movable_folio to use __GFP_ZERO so they
> > are not affected by this. This means that only a subset of architectures
> > are really affected. This is an important information as well.
> > Finally I think it would be more appropriate to mention that the double
> > initialization is done when init_on_alloc is enabled rather than
> > referring to the above config option which only controls whether the
> > functionality is enabled by default.
>
> This may well be an unsaafe change to make. We're not just zeroing the
> page, we're calling clear_user_highpage() which tells the architecture
> which virtual address the page will be mapped at. It could be that
> skipping the zeroing ("because the page is already zero") isn't enough;
> there will be traces of the former contents of some page in the D-cache
> for this address.
I haven't realized this difference between clear_user_highpage and
kernel_init_pages which is used by the page allocator. Thanks for
pointing this out!
>
> Or it might just be an optimisation. The description of clear_user_page()
> isn't entirely clear; the port may be relying on clear_user_page()
> to have flushed the dcache aliases.
>
> At this point, I don't think this patch is worth the risk.
Agreed! Based on that I take my ack back.
> My mind is
> changable on this, but I think we'd need buy-in from ARM, SH and Xtensa
> (who directly define clear_user_highpage()) as well as Arc, csky, ia64,
> m68k, mips, nios2, parisc, powerpc, sparc who all seem to have non-trivial
> clear_user_page() implementations.
--
Michal Hocko
SUSE Labs
Powered by blists - more mailing lists