| Message ID | 20230911104906.2058503-1-zhaoyang.huang@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: remove redundant clear page when CONFIG_INIT_ON_ALLOC_DEFAULT_ON configured | expand |
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
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.
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.
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;