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 <[email protected]>
---
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
--
2.39.3
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 <[email protected]>
> ---
> 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.