Re: [PATCH RFC v3 03/35] mm: page_alloc: Add an arch hook to filter MIGRATE_CMA allocations

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Mon, Jan 29, 2024 at 02:14:16PM +0530, Anshuman Khandual wrote:
> 
> 
> On 1/25/24 22:12, Alexandru Elisei wrote:
> > As an architecture might have specific requirements around the allocation
> > of CMA pages, add an arch hook that can disable allocations from
> > MIGRATE_CMA, if the allocation was otherwise allowed.
> > 
> > This will be used by arm64, which will put tag storage pages on the
> > MIGRATE_CMA list, and tag storage pages cannot be tagged. The filter will
> > be used to deny using MIGRATE_CMA for __GFP_TAGGED allocations.
> 
> Just wondering how allocation requests would be blocked for direct
> alloc_contig_range() requests ?

alloc_contig_range() does page allocation in __alloc_contig_migrate_range()
-> alloc_migration_target(); __alloc_contig_migrate_range() ignores the
gfp_mask parameter passed to alloc_contig_range() when building struct
migration_target_control, even though it's available in the struct
compact_control argument. That looks like a bug to me, as the decription
for the gfp_mask parameter says: "GFP mask to use during compaction".

Regardless, when tag storage page T1 is migrated to it can be used to
storage tags, it doesn't matter if it is replaced by another tag storage
page T2 or a regular page, as long as the replacement isn't also tagged. If
the replacement is also tagged, the code to reserve tag storage would
recurse and deadlock. See patch #16 ("KVM: arm64: Don't deny VM_PFNMAP VMAs
when kvm_has_mte()") [1] for the code.

Does that make sense?

[1] https://lore.kernel.org/linux-mm/20240125164256.4147-24-alexandru.elisei@arm.com/

> 
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  include/linux/pgtable.h | 7 +++++++
> >  mm/page_alloc.c         | 3 ++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> > index 6d98d5fdd697..c5ddec6b5305 100644
> > --- a/include/linux/pgtable.h
> > +++ b/include/linux/pgtable.h
> > @@ -905,6 +905,13 @@ static inline void arch_do_swap_page(struct mm_struct *mm,
> >  static inline void arch_free_pages_prepare(struct page *page, int order) { }
> >  #endif
> >  
> > +#ifndef __HAVE_ARCH_ALLOC_CMA
> 
> Same as last patch i.e __HAVE_ARCH_ALLOC_CMA could be avoided via
> a direct check on #ifndef arch_alloc_cma instead.

include/linux/pgtable.h uses __HAVE_ARCH_*, and I would rather keep it
consistent.

Thanks,
Alex

> 
> > +static inline bool arch_alloc_cma(gfp_t gfp)
> > +{
> > +	return true;
> > +}
> > +#endif
> > +
> >  #ifndef __HAVE_ARCH_UNMAP_ONE
> >  /*
> >   * Some architectures support metadata associated with a page. When a
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 27282a1c82fe..a96d47a6393e 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3157,7 +3157,8 @@ static inline unsigned int gfp_to_alloc_flags_cma(gfp_t gfp_mask,
> >  						  unsigned int alloc_flags)
> >  {
> >  #ifdef CONFIG_CMA
> > -	if (gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> > +	if (gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE &&
> > +	    arch_alloc_cma(gfp_mask))
> >  		alloc_flags |= ALLOC_CMA;
> >  #endif
> >  	return alloc_flags;