Received-SPF: pass (google.com: domain of linux-kernel+bounces-138981-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Apr 2024 12:33:46 -0400
From: Peter Xu <peterx@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Sumanth Korikkar <sumanthk@linux.ibm.com>, akpm@linux-foundation.org,
	linux-mm@kvack.org, hughd@google.com, hca@linux.ibm.com,
	gor@linux.ibm.com, agordeev@linux.ibm.com, iii@linux.ibm.com,
	linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled
 transparent hugepages
Message-ID: <Zha_auw6yBx0YRWQ@x1n>
References: <20240409155407.2322714-1-sumanthk@linux.ibm.com>
 <594dbec7-b560-44e5-a684-93dcb8ba85df@redhat.com>
 <Zhavr9NxvayDhU9X@li-2b55cdcc-350b-11b2-a85c-a78bff51fc11.ibm.com>
 <29b613a8-f0d5-4f5f-adbc-d64ed8908044@redhat.com>
 <Zha5V0QxODZR4qOW@li-2b55cdcc-350b-11b2-a85c-a78bff51fc11.ibm.com>
 <1ce2fe6c-b56a-4582-a5d8-babc8fccef52@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1ce2fe6c-b56a-4582-a5d8-babc8fccef52@redhat.com>

On Wed, Apr 10, 2024 at 06:12:34PM +0200, David Hildenbrand wrote:
> On 10.04.24 18:07, Sumanth Korikkar wrote:
> > On Wed, Apr 10, 2024 at 05:51:28PM +0200, David Hildenbrand wrote:
> > > On 10.04.24 17:26, Sumanth Korikkar wrote:
> > > > On Wed, Apr 10, 2024 at 02:34:35PM +0200, David Hildenbrand wrote:
> > > > > On 09.04.24 17:54, Sumanth Korikkar wrote:
> > > > > > In order to  minimize code size (CONFIG_CC_OPTIMIZE_FOR_SIZE=y),
> > > > > > compiler might choose to make a regular function call (out-of-line) for
> > > > > > shmem_is_huge() instead of inlining it. When transparent hugepages are
> > > > > > disabled (CONFIG_TRANSPARENT_HUGEPAGE=n), it can cause compilation
> > > > > > error.
> > > > > > 
> > > > > > mm/shmem.c: In function ‘shmem_getattr’:
> > > > > > ./include/linux/huge_mm.h:383:27: note: in expansion of macro ‘BUILD_BUG’
> > > > > >      383 | #define HPAGE_PMD_SIZE ({ BUILD_BUG(); 0; })
> > > > > >          |                           ^~~~~~~~~
> > > > > > mm/shmem.c:1148:33: note: in expansion of macro ‘HPAGE_PMD_SIZE’
> > > > > >     1148 |                 stat->blksize = HPAGE_PMD_SIZE;
> > > > > > 
> > > > > > To prevent the possible error, always inline shmem_is_huge() when
> > > > > > transparent hugepages are disabled.
> > > > > > 
> > > > > 
> > > > > Do you know which commit introduced that?
> > > > Hi David,
> > > > 
> > > > Currently with CONFIG_CC_OPTIMIZE_FOR_SIZE=y and expirementing with
> > > > -fPIC kernel compiler option, I could see this error on s390.
> > > 
> > > Got it. I assume on Linus' tree, not mm/unstable?
> > 
> > It's not yet upstream.
> > > 
> > > > 
> > > > However, default kernel compiler options doesnt end up with the above
> > > > pattern right now.
> > > 
> > > Okay, just asking if this is related to recent HPAGE_PMD_SIZE changes:
> > > 
> > > commit c1a1e497a3d5711dbf8fa6d7432d6b83ec18c26f
> > > Author: Peter Xu <peterx@redhat.com>
> > > Date:   Wed Mar 27 11:23:22 2024 -0400
> > > 
> > >      mm: make HPAGE_PXD_* macros even if !THP
> > > 
> > > Which is still in mm-unstable and not upstream.
> > 
> > Not related to this commit. I tried on master branch.
> 
> Thanks! Can you try with Peters patch? (ccing Peter)
> 
> If I am not wrong, that should also resolve the issue you are seeing.

David,

Do you mean this one?

https://lore.kernel.org/all/20240403013249.1418299-4-peterx@redhat.com/

That's indeed similar but that was for pud_pfn() not HPAGE_* stuff.

I just had a quick look, Sumanth's fix looks valid, and IIUC the goal is
also that we should keep these build checks around for the long term goal
(Jason definitely preferred that [1] too, which I agree).

I removed that build check there for pud_pfn just to avoid other build
fallouts for other archs as a temporary measure.  For this one if it's in
common code for a long time and if it's the single spot maybe it's nice to
have this patch as proposed, as it means it optimizes the if check too
besides fixing the build error.  After all referencing HPAGE_* with
!THP+!HUGETLB shouldn't happen logically.

[1] https://lore.kernel.org/r/20240404112404.GG1723999@nvidia.com

Thanks,

-- 
Peter Xu