| Message ID | d9c0ac9df6f6e6176b09fc1ae58146ec4f8d73f5.1714978902.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add mTHP support for anonymous shmem | expand |
On 06/05/2024 09:46, Baolin Wang wrote: > Move highest_order() and next_order() out of the CONFIG_TRANSPARENT_HUGEPAGE > macro, which can be common functions to be used. Sorry if I haven't kept up with the discussion, but why is this needed? I wouldn't expect a need to iterate over orders if THP is compile-time disabled because we will never try to allocate THP? > > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > include/linux/huge_mm.h | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index 017cee864080..e49b56c40a11 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -106,6 +106,17 @@ extern struct kobj_attribute shmem_enabled_attr; > #define HPAGE_PUD_MASK (~(HPAGE_PUD_SIZE - 1)) > #define HPAGE_PUD_SIZE ((1UL) << HPAGE_PUD_SHIFT) > > +static inline int highest_order(unsigned long orders) > +{ > + return fls_long(orders) - 1; > +} > + > +static inline int next_order(unsigned long *orders, int prev) > +{ > + *orders &= ~BIT(prev); > + return highest_order(*orders); > +} > + > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > extern unsigned long transparent_hugepage_flags; > @@ -138,17 +149,6 @@ static inline bool hugepage_flags_enabled(void) > huge_anon_orders_madvise; > } > > -static inline int highest_order(unsigned long orders) > -{ > - return fls_long(orders) - 1; > -} > - > -static inline int next_order(unsigned long *orders, int prev) > -{ > - *orders &= ~BIT(prev); > - return highest_order(*orders); > -} > - > /* > * Do the below checks: > * - For file vma, check if the linear page offset of vma is
On 2024/5/7 18:21, Ryan Roberts wrote: > On 06/05/2024 09:46, Baolin Wang wrote: >> Move highest_order() and next_order() out of the CONFIG_TRANSPARENT_HUGEPAGE >> macro, which can be common functions to be used. > > Sorry if I haven't kept up with the discussion, but why is this needed? I > wouldn't expect a need to iterate over orders if THP is compile-time disabled > because we will never try to allocate THP? Cause I don't want to add some dummy functions to avoid building errors if CONFIG_TRANSPARENT_HUGEPAGE is not enabled in patch 6. Another thought is that the pagecache can also allocate a large folio even when THP is not enabled, so these helpers may be used in the future (not sure though). Anyway, I also have no strong perference for this patch, below dummy functions can also work for me: diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index c15bebb2cf53..7aa802ee2ce5 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -586,6 +586,16 @@ static inline bool thp_migration_supported(void) { return false; } + +static inline int highest_order(unsigned long orders) +{ + return 0; +} + +static inline int next_order(unsigned long *orders, int prev) +{ + return 0; +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
On 08/05/2024 03:13, Baolin Wang wrote: > > > On 2024/5/7 18:21, Ryan Roberts wrote: >> On 06/05/2024 09:46, Baolin Wang wrote: >>> Move highest_order() and next_order() out of the CONFIG_TRANSPARENT_HUGEPAGE >>> macro, which can be common functions to be used. >> >> Sorry if I haven't kept up with the discussion, but why is this needed? I >> wouldn't expect a need to iterate over orders if THP is compile-time disabled >> because we will never try to allocate THP? > > Cause I don't want to add some dummy functions to avoid building errors if > CONFIG_TRANSPARENT_HUGEPAGE is not enabled in patch 6. Another thought is that > the pagecache can also allocate a large folio even when THP is not enabled, so > these helpers may be used in the future (not sure though). OK, I'll admit I haven't looked at the latter patches yet - I'd like to conclude on the interface and mapping/alignment strategy first. But it wasn't necessary to access these functions for the anon/private case without CONFIG_TRANSPARENT_HUGEPAGE, so I'm wondering why it's needed for shmem case. I would expect that they don't need to be defined at all. > > Anyway, I also have no strong perference for this patch, below dummy functions > can also work for me: > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index c15bebb2cf53..7aa802ee2ce5 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -586,6 +586,16 @@ static inline bool thp_migration_supported(void) > { > return false; > } > + > +static inline int highest_order(unsigned long orders) > +{ > + return 0; > +} > + > +static inline int next_order(unsigned long *orders, int prev) > +{ > + return 0; > +} > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
On 2024/5/8 17:06, Ryan Roberts wrote: > On 08/05/2024 03:13, Baolin Wang wrote: >> >> >> On 2024/5/7 18:21, Ryan Roberts wrote: >>> On 06/05/2024 09:46, Baolin Wang wrote: >>>> Move highest_order() and next_order() out of the CONFIG_TRANSPARENT_HUGEPAGE >>>> macro, which can be common functions to be used. >>> >>> Sorry if I haven't kept up with the discussion, but why is this needed? I >>> wouldn't expect a need to iterate over orders if THP is compile-time disabled >>> because we will never try to allocate THP? >> >> Cause I don't want to add some dummy functions to avoid building errors if >> CONFIG_TRANSPARENT_HUGEPAGE is not enabled in patch 6. Another thought is that >> the pagecache can also allocate a large folio even when THP is not enabled, so >> these helpers may be used in the future (not sure though). > > OK, I'll admit I haven't looked at the latter patches yet - I'd like to conclude > on the interface and mapping/alignment strategy first. > > But it wasn't necessary to access these functions for the anon/private case > without CONFIG_TRANSPARENT_HUGEPAGE, so I'm wondering why it's needed for shmem > case. I would expect that they don't need to be defined at all. Currently in the shmem_alloc_and_add_folio() function, the hugepage allocating is not guarded with '#ifdef CONFIG_TRANSPARENT_HUGEPAGE', but rather with 'IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)', which can lead to some building errors when CONFIG_TRANSPARENT_HUGEPAGE is not enabled. However, this is not a big issue, and I will make some adjustments to avoid defining dummy functions.
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index 017cee864080..e49b56c40a11 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -106,6 +106,17 @@ extern struct kobj_attribute shmem_enabled_attr; #define HPAGE_PUD_MASK (~(HPAGE_PUD_SIZE - 1)) #define HPAGE_PUD_SIZE ((1UL) << HPAGE_PUD_SHIFT) +static inline int highest_order(unsigned long orders) +{ + return fls_long(orders) - 1; +} + +static inline int next_order(unsigned long *orders, int prev) +{ + *orders &= ~BIT(prev); + return highest_order(*orders); +} + #ifdef CONFIG_TRANSPARENT_HUGEPAGE extern unsigned long transparent_hugepage_flags; @@ -138,17 +149,6 @@ static inline bool hugepage_flags_enabled(void) huge_anon_orders_madvise; } -static inline int highest_order(unsigned long orders) -{ - return fls_long(orders) - 1; -} - -static inline int next_order(unsigned long *orders, int prev) -{ - *orders &= ~BIT(prev); - return highest_order(*orders); -} - /* * Do the below checks: * - For file vma, check if the linear page offset of vma is
Move highest_order() and next_order() out of the CONFIG_TRANSPARENT_HUGEPAGE macro, which can be common functions to be used. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- include/linux/huge_mm.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)