[PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[Sumanth Korikkar]

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.

Signed-off-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
---
 include/linux/shmem_fs.h | 9 +++++++++
 mm/shmem.c               | 6 ------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index a4c15db2f5e5..3fb18f7eb73e 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -110,8 +110,17 @@ extern struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
 extern void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end);
 int shmem_unuse(unsigned int type);
 
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 extern bool shmem_is_huge(struct inode *inode, pgoff_t index, bool shmem_huge_force,
 			  struct mm_struct *mm, unsigned long vm_flags);
+#else
+static __always_inline bool shmem_is_huge(struct inode *inode, pgoff_t index, bool shmem_huge_force,
+					  struct mm_struct *mm, unsigned long vm_flags)
+{
+	return false;
+}
+#endif
+
 #ifdef CONFIG_SHMEM
 extern unsigned long shmem_swap_usage(struct vm_area_struct *vma);
 #else
diff --git a/mm/shmem.c b/mm/shmem.c
index 0aad0d9a621b..94ab99b6b574 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -748,12 +748,6 @@ static long shmem_unused_huge_count(struct super_block *sb,
 
 #define shmem_huge SHMEM_HUGE_DENY
 
-bool shmem_is_huge(struct inode *inode, pgoff_t index, bool shmem_huge_force,
-		   struct mm_struct *mm, unsigned long vm_flags)
-{
-	return false;
-}
-
 static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
 		struct shrink_control *sc, unsigned long nr_to_split)
 {
-- 
2.40.1

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[Sumanth Korikkar]

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.

However, default kernel compiler options doesnt end up with the above
pattern right now.

I think, shmem_is_huge() for disabled transparent hugepages comes from:
Commit 5e6e5a12a44c ("huge tmpfs: shmem_is_huge(vma, inode, index)")

However, HPAGE_PMD_SIZE macros for !CONFIG_TRANSPARENT_HUGEPAGE
originates from:
Commit d8c37c480678 ("thp: add HPAGE_PMD_* definitions for
!CONFIG_TRANSPARENT_HUGEPAGE")

Thanks,
Sumanth

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[David Hildenbrand]

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?

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

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[Sumanth Korikkar]

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,
Sumanth

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[Peter Xu]

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

Re: [PATCH] mm/shmem: Inline shmem_is_huge() for disabled transparent hugepages

[David Hildenbrand]

On 10.04.24 18:51, Peter Xu wrote:
> On Wed, Apr 10, 2024 at 06:40:55PM +0200, David Hildenbrand wrote:
>> On 10.04.24 18:33, Peter Xu wrote:
>>> 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/
>>>
>>
>> No, I meant:
>>
>> https://lore.kernel.org/all/20240327152332.950956-4-peterx@redhat.com/
>>
>> which removes the "#define HPAGE_PMD_SIZE ({ BUILD_BUG(); 0; })" that we
>> seem to trigger here.
>>
>>
>> ... but it's been a long day, so maybe I'm all wrong :)
> 
> Ah..  So I thought it was one step further. :)
> 
> Then that shouldn't be the case; it didn't remove it but defined properly
> with HPAGE_PMD_SHIFT:
> 
> +#define HPAGE_PMD_SIZE	((1UL) << HPAGE_PMD_SHIFT)
> 
> Now we even have that properly defined for HUGETLB_PAGE, while prior to
> that we should hit this issue easier (even with !THP+HUGETLB_PAGE).

Ah, now I spot

#define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })

:) sorry for the noise!

To the original patch here

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb