From: Zhaoyang Huang <zhaoyang.huang@unisoc.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.

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
 include/linux/highmem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 99c474de800d..3926f8414729 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -227,7 +227,7 @@ struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
 	struct folio *folio;
 
 	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
-	if (folio)
+	if (folio && !want_init_on_alloc(GFP_HIGHUSER_MOVABLE))
 		clear_user_highpage(&folio->page, vaddr);
 
 	return folio;
-- 
2.25.1

On Mon 11-09-23 18:49:06, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.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.

I would rephrase as follows:
Many architectures (alpha, arm64, ia64, m68k s390, x86) define their own
vma_alloc_zeroed_movable_folio implementations which use __GFP_ZERO for
the page allocation.

Those which rely on the default implementation, however, would currently
go through the initialization twice (oce in the page allocator and
second in vma_alloc_zeroed_movable_folio) if init_on_alloc is enabled
though. Fix this by checking want_init_on_alloc before calling
clear_user_highpage.

> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

With the changelog updates
Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  include/linux/highmem.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 99c474de800d..3926f8414729 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -227,7 +227,7 @@ struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
>  	struct folio *folio;
>  
>  	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> -	if (folio)
> +	if (folio && !want_init_on_alloc(GFP_HIGHUSER_MOVABLE))
>  		clear_user_highpage(&folio->page, vaddr);
>  
>  	return folio;
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

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@unisoc.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.

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.  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.

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@unisoc.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

On Mon 11-09-23 14:12:26, Michal Hocko wrote:
> On Mon 11-09-23 18:49:06, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.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.
> 
> I would rephrase as follows:
> Many architectures (alpha, arm64, ia64, m68k s390, x86) define their own
> vma_alloc_zeroed_movable_folio implementations which use __GFP_ZERO for
> the page allocation.
> 
> Those which rely on the default implementation, however, would currently
> go through the initialization twice (oce in the page allocator and
> second in vma_alloc_zeroed_movable_folio) if init_on_alloc is enabled
> though. Fix this by checking want_init_on_alloc before calling
> clear_user_highpage.

Btw. have you checked other places which could have a similar problem?
From a very quick look __do_huge_pmd_anonymous_page, hugetlb_no_page,
hugetlbfs_fallocate and shmem_mfill_atomic_pte all follow the same
pattern. They do allocate memory so they go through the initialization
in the allocator and then reinitialized.

-- 
Michal Hocko
SUSE Labs