Re: [PATCH 12/13] mm/treewide: Remove pXd_huge()

From: Peter Xu <peterx@redhat.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Matthew Wilcox <willy@infradead.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	"x86@kernel.org" <x86@kernel.org>,
	Mike Rapoport <rppt@kernel.org>,
	Muchun Song <muchun.song@linux.dev>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH 12/13] mm/treewide: Remove pXd_huge()
Date: Thu, 14 Mar 2024 10:08:50 -0400	[thread overview]
Message-ID: <ZfME8vjFhl8AdyFe@x1n> (raw)
In-Reply-To: <7e93ab99-c956-42d0-9afd-3c856f3ad951@csgroup.eu>

On Thu, Mar 14, 2024 at 08:56:59AM +0000, Christophe Leroy wrote:
> 
> 
> Le 13/03/2024 à 22:47, peterx@redhat.com a écrit :
> > From: Peter Xu <peterx@redhat.com>
> > 
> > This API is not used anymore, drop it for the whole tree.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   arch/arm/mm/Makefile                          |  1 -
> >   arch/arm/mm/hugetlbpage.c                     | 29 -------------------
> >   arch/arm64/mm/hugetlbpage.c                   | 10 -------
> >   arch/loongarch/mm/hugetlbpage.c               | 10 -------
> >   arch/mips/include/asm/pgtable-32.h            |  2 +-
> >   arch/mips/include/asm/pgtable-64.h            |  2 +-
> >   arch/mips/mm/hugetlbpage.c                    | 10 -------
> >   arch/parisc/mm/hugetlbpage.c                  | 11 -------
> >   .../include/asm/book3s/64/pgtable-4k.h        | 10 -------
> >   .../include/asm/book3s/64/pgtable-64k.h       | 25 ----------------
> >   arch/powerpc/include/asm/nohash/pgtable.h     | 10 -------
> >   arch/riscv/mm/hugetlbpage.c                   | 10 -------
> >   arch/s390/mm/hugetlbpage.c                    | 10 -------
> >   arch/sh/mm/hugetlbpage.c                      | 10 -------
> >   arch/sparc/mm/hugetlbpage.c                   | 10 -------
> >   arch/x86/mm/hugetlbpage.c                     | 16 ----------
> >   include/linux/hugetlb.h                       | 24 ---------------
> >   17 files changed, 2 insertions(+), 198 deletions(-)
> >   delete mode 100644 arch/arm/mm/hugetlbpage.c
> > 
> 
> > diff --git a/arch/mips/include/asm/pgtable-32.h b/arch/mips/include/asm/pgtable-32.h
> > index 0e196650f4f4..92b7591aac2a 100644
> > --- a/arch/mips/include/asm/pgtable-32.h
> > +++ b/arch/mips/include/asm/pgtable-32.h
> > @@ -129,7 +129,7 @@ static inline int pmd_none(pmd_t pmd)
> >   static inline int pmd_bad(pmd_t pmd)
> >   {
> >   #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
> > -	/* pmd_huge(pmd) but inline */
> > +	/* pmd_leaf(pmd) but inline */
> 
> Shouldn't this comment have been changed in patch 11 ?

IMHO it's fine to be here, as this is the patch to finally drop _huge().
Patch 11 only converts the callers to use _leaf()s.  So this comment is
still valid until this patch, because this patch removes that definition.

> 
> >   	if (unlikely(pmd_val(pmd) & _PAGE_HUGE))
> 
> Unlike pmd_huge() which is an outline function, pmd_leaf() is a macro so 
> it could be used here instead of open coping.

I worry it will break things as pmd_leaf() can sometimes be defined after
arch *pgtable.h headers.  So I avoided touching it except what I think I'm
confident.  I had a feeling it's inlined just because of a similar reason
for the old _huge().

> 
> >   		return 0;
> >   #endif
> > diff --git a/arch/mips/include/asm/pgtable-64.h b/arch/mips/include/asm/pgtable-64.h
> > index 20ca48c1b606..7c28510b3768 100644
> > --- a/arch/mips/include/asm/pgtable-64.h
> > +++ b/arch/mips/include/asm/pgtable-64.h
> > @@ -245,7 +245,7 @@ static inline int pmd_none(pmd_t pmd)
> >   static inline int pmd_bad(pmd_t pmd)
> >   {
> >   #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
> > -	/* pmd_huge(pmd) but inline */
> > +	/* pmd_leaf(pmd) but inline */
> 
> Same
> 
> >   	if (unlikely(pmd_val(pmd) & _PAGE_HUGE))
> 
> Same
> 
> >   		return 0;
> >   #endif
> 
> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable-64k.h b/arch/powerpc/include/asm/book3s/64/pgtable-64k.h
> > index 2fce3498b000..579a7153857f 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable-64k.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable-64k.h
> > @@ -4,31 +4,6 @@
> >   
> >   #ifndef __ASSEMBLY__
> >   #ifdef CONFIG_HUGETLB_PAGE
> > -/*
> > - * We have PGD_INDEX_SIZ = 12 and PTE_INDEX_SIZE = 8, so that we can have
> > - * 16GB hugepage pte in PGD and 16MB hugepage pte at PMD;
> > - *
> > - * Defined in such a way that we can optimize away code block at build time
> > - * if CONFIG_HUGETLB_PAGE=n.
> > - *
> > - * returns true for pmd migration entries, THP, devmap, hugetlb
> > - * But compile time dependent on CONFIG_HUGETLB_PAGE
> > - */
> 
> Should we keep this comment somewhere for documentation ?

The 2nd/3rd paragraphs are definitely obsolete, so should be dropped.

OTOH, I'm not sure how much that will help if e.g. I move that over to
pmd_leaf(): a check over cpu_to_be64(_PAGE_PTE) is an implementation as
simple as it could be to explain itself with even no comment to me..

I also don't fully digest why that 1st paragraph discusses PGD entries: for
example, there's no pgd_huge() defined.  It may not mean that the comment
is wrong, perhaps it means that I may lack some knowledge around this area
on Power..

Would you suggest how I should move paragraph 1 (and help to explain what
it is describing)?  Or maybe we can provide a separate patch for Power's
huge page sizes but posted separately (and very possibly I'm not the best
candidate then..).

> 
> > -static inline int pmd_huge(pmd_t pmd)
> > -{
> > -	/*
> > -	 * leaf pte for huge page
> > -	 */
> > -	return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
> > -}
> > -
> > -static inline int pud_huge(pud_t pud)
> > -{
> > -	/*
> > -	 * leaf pte for huge page
> > -	 */
> > -	return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE));
> > -}
> >   
> >   /*
> >    * With 64k page size, we have hugepage ptes in the pgd and pmd entries. We don't

-- 
Peter Xu