[PATCH 0/3] mm: add more kernel parameters to control mTHP

[PATCH 0/3] mm: add more kernel parameters to control mTHP

[Maíra Canal]

This series introduces three patches related to the kernel parameters
controlling mTHP. The first patch is a straightforward documentation update,
correcting the format of the kernel parameter ``thp_anon=``.

The second and third patches focus on controlling THP support for shmem
via the kernel command line. The second patch introduces a parameter to
control the global default huge page allocation policy for the internal
shmem mount. The third patch implements a parameter similar to ``thp_anon=``,
but for shmem.

The goal of these changes is to simplify the configuration of systems that
rely on mTHP support for shmem. For instance, a platform with a GPU that
benefits from huge pages may want to enable huge pages for shmem. Having
these kernel parameters streamlines the configuration process and ensures
consistency across setups.

Regarding the third patch, I’m open to suggestions on how to reduce code
duplication between ``thp_anon=`` and ``thp_shmem=``. While I duplicated
the ``get_order_from_str()`` function, I realize this isn’t ideal and
would appreciate advice on where best to place the function.

Let me know your thoughts.

[1] https://lore.kernel.org/linux-mm/20240820105244.62703-1-21cnbao@gmail.com/

Best Regards,
- Maíra

Maíra Canal (3):
  mm: fix the format of the kernel parameter ``thp_anon=``
  mm: shmem: control THP support through the kernel command line
  mm: shmem: override mTHP shmem default with a kernel parameter

 .../admin-guide/kernel-parameters.txt         |  19 ++-
 Documentation/admin-guide/mm/transhuge.rst    |  25 ++-
 mm/shmem.c                                    | 147 +++++++++++++++++-
 3 files changed, 186 insertions(+), 5 deletions(-)

-- 
2.46.2

[PATCH 1/3] mm: fix the format of the kernel parameter ``thp_anon=``

[Maíra Canal]

If we add ``thp_anon=32,64KB:always`` to the kernel command line, we
will see the following error:

[    0.000000] huge_memory: thp_anon=32,64K:always: error parsing string, ignoring setting

This happens because the correct format isn't ``thp_anon=<size>,<size>[KMG]:<state>```,
as [KMG] must follow each number to especify its unit. So, the correct
format is ``thp_anon=<size>[KMG],<size>[KMG]:<state>```.

Therefore, adjust the documentation to reflect the correct format of the
parameter ``thp_anon=``.

Fixes: dd4d30d1cdbe ("mm: override mTHP "enabled" defaults at kernel cmdline")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 2 +-
 Documentation/admin-guide/mm/transhuge.rst      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..1666576acc0e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6688,7 +6688,7 @@
 			0: no polling (default)
 
 	thp_anon=	[KNL]
-			Format: <size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>
+			Format: <size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>
 			state is one of "always", "madvise", "never" or "inherit".
 			Control the default behavior of the system with respect
 			to anonymous transparent hugepages.
diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 203ba7aaf5fc..745055c3dc09 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -303,7 +303,7 @@ control by passing the parameter ``transparent_hugepage=always`` or
 kernel command line.
 
 Alternatively, each supported anonymous THP size can be controlled by
-passing ``thp_anon=<size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>``,
+passing ``thp_anon=<size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>``,
 where ``<size>`` is the THP size (must be a power of 2 of PAGE_SIZE and
 supported anonymous THP)  and ``<state>`` is one of ``always``, ``madvise``,
 ``never`` or ``inherit``.
-- 
2.46.2

Re: [PATCH 1/3] mm: fix the format of the kernel parameter ``thp_anon=``

[Barry Song]

On Mon, Oct 28, 2024 at 1:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> If we add ``thp_anon=32,64KB:always`` to the kernel command line, we
> will see the following error:
>
> [    0.000000] huge_memory: thp_anon=32,64K:always: error parsing string, ignoring setting
>
> This happens because the correct format isn't ``thp_anon=<size>,<size>[KMG]:<state>```,
> as [KMG] must follow each number to especify its unit. So, the correct
> format is ``thp_anon=<size>[KMG],<size>[KMG]:<state>```.

what if 32768,64K: always?

>
> Therefore, adjust the documentation to reflect the correct format of the
> parameter ``thp_anon=``.
>
> Fixes: dd4d30d1cdbe ("mm: override mTHP "enabled" defaults at kernel cmdline")
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 2 +-
>  Documentation/admin-guide/mm/transhuge.rst      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..1666576acc0e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6688,7 +6688,7 @@
>                         0: no polling (default)
>
>         thp_anon=       [KNL]
> -                       Format: <size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>
> +                       Format: <size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>
>                         state is one of "always", "madvise", "never" or "inherit".
>                         Control the default behavior of the system with respect
>                         to anonymous transparent hugepages.
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 203ba7aaf5fc..745055c3dc09 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -303,7 +303,7 @@ control by passing the parameter ``transparent_hugepage=always`` or
>  kernel command line.
>
>  Alternatively, each supported anonymous THP size can be controlled by
> -passing ``thp_anon=<size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>``,
> +passing ``thp_anon=<size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>``,
>  where ``<size>`` is the THP size (must be a power of 2 of PAGE_SIZE and
>  supported anonymous THP)  and ``<state>`` is one of ``always``, ``madvise``,
>  ``never`` or ``inherit``.
> --
> 2.46.2
>

Re: [PATCH 1/3] mm: fix the format of the kernel parameter ``thp_anon=``

[Maíra Canal]

Hi Barry,

On 27/10/24 16:52, Barry Song wrote:
> On Mon, Oct 28, 2024 at 1:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> If we add ``thp_anon=32,64KB:always`` to the kernel command line, we
>> will see the following error:
>>
>> [    0.000000] huge_memory: thp_anon=32,64K:always: error parsing string, ignoring setting
>>
>> This happens because the correct format isn't ``thp_anon=<size>,<size>[KMG]:<state>```,
>> as [KMG] must follow each number to especify its unit. So, the correct
>> format is ``thp_anon=<size>[KMG],<size>[KMG]:<state>```.
> 
> what if 32768,64K: always?

``32768,64K:always`` works. From the kernel parameters documentation, I
see that:

"Finally, the [KMG] suffix is commonly described after a number of
kernel parameter values. These ‘K’, ‘M’, and ‘G’ letters represent the
_binary_ multipliers ‘Kilo’, ‘Mega’, and ‘Giga’, equaling 2^10, 2^20,
and 2^30 bytes respectively. Such letter suffixes can also be entirely
omitted"

AFAIU this means that [KMG] can be omitted if we use bytes. But if we
don't use bytes, it cannot be omitted.

Best Regards,
- Maíra

> 
>>
>> Therefore, adjust the documentation to reflect the correct format of the
>> parameter ``thp_anon=``.
>>
>> Fixes: dd4d30d1cdbe ("mm: override mTHP "enabled" defaults at kernel cmdline")
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 2 +-
>>   Documentation/admin-guide/mm/transhuge.rst      | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 1518343bbe22..1666576acc0e 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -6688,7 +6688,7 @@
>>                          0: no polling (default)
>>
>>          thp_anon=       [KNL]
>> -                       Format: <size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>
>> +                       Format: <size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>
>>                          state is one of "always", "madvise", "never" or "inherit".
>>                          Control the default behavior of the system with respect
>>                          to anonymous transparent hugepages.
>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>> index 203ba7aaf5fc..745055c3dc09 100644
>> --- a/Documentation/admin-guide/mm/transhuge.rst
>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>> @@ -303,7 +303,7 @@ control by passing the parameter ``transparent_hugepage=always`` or
>>   kernel command line.
>>
>>   Alternatively, each supported anonymous THP size can be controlled by
>> -passing ``thp_anon=<size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>``,
>> +passing ``thp_anon=<size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>``,
>>   where ``<size>`` is the THP size (must be a power of 2 of PAGE_SIZE and
>>   supported anonymous THP)  and ``<state>`` is one of ``always``, ``madvise``,
>>   ``never`` or ``inherit``.
>> --
>> 2.46.2
>>

Re: [PATCH 1/3] mm: fix the format of the kernel parameter ``thp_anon=``

[Barry Song]

On Mon, Oct 28, 2024 at 9:36 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Barry,
>
> On 27/10/24 16:52, Barry Song wrote:
> > On Mon, Oct 28, 2024 at 1:58 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> If we add ``thp_anon=32,64KB:always`` to the kernel command line, we
> >> will see the following error:
> >>
> >> [    0.000000] huge_memory: thp_anon=32,64K:always: error parsing string, ignoring setting
> >>
> >> This happens because the correct format isn't ``thp_anon=<size>,<size>[KMG]:<state>```,
> >> as [KMG] must follow each number to especify its unit. So, the correct
> >> format is ``thp_anon=<size>[KMG],<size>[KMG]:<state>```.
> >
> > what if 32768,64K: always?
>
> ``32768,64K:always`` works. From the kernel parameters documentation, I
> see that:
>
> "Finally, the [KMG] suffix is commonly described after a number of
> kernel parameter values. These ‘K’, ‘M’, and ‘G’ letters represent the
> _binary_ multipliers ‘Kilo’, ‘Mega’, and ‘Giga’, equaling 2^10, 2^20,
> and 2^30 bytes respectively. Such letter suffixes can also be entirely
> omitted"
>
> AFAIU this means that [KMG] can be omitted if we use bytes. But if we
> don't use bytes, it cannot be omitted.

Thanks! Could we change the subject of this commit to "fix the doc" without
mentioning format fixes? we are obviously only fixing the doc. With that,
please feel free to add:

Acked-by: Barry Song <baohua@kernel.org>

>
> Best Regards,
> - Maíra
>
> >
> >>
> >> Therefore, adjust the documentation to reflect the correct format of the
> >> parameter ``thp_anon=``.
> >>
> >> Fixes: dd4d30d1cdbe ("mm: override mTHP "enabled" defaults at kernel cmdline")
> >> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >> ---
> >>   Documentation/admin-guide/kernel-parameters.txt | 2 +-
> >>   Documentation/admin-guide/mm/transhuge.rst      | 2 +-
> >>   2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 1518343bbe22..1666576acc0e 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -6688,7 +6688,7 @@
> >>                          0: no polling (default)
> >>
> >>          thp_anon=       [KNL]
> >> -                       Format: <size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>
> >> +                       Format: <size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>
> >>                          state is one of "always", "madvise", "never" or "inherit".
> >>                          Control the default behavior of the system with respect
> >>                          to anonymous transparent hugepages.
> >> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >> index 203ba7aaf5fc..745055c3dc09 100644
> >> --- a/Documentation/admin-guide/mm/transhuge.rst
> >> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >> @@ -303,7 +303,7 @@ control by passing the parameter ``transparent_hugepage=always`` or
> >>   kernel command line.
> >>
> >>   Alternatively, each supported anonymous THP size can be controlled by
> >> -passing ``thp_anon=<size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>``,
> >> +passing ``thp_anon=<size>[KMG],<size>[KMG]:<state>;<size>[KMG]-<size>[KMG]:<state>``,
> >>   where ``<size>`` is the THP size (must be a power of 2 of PAGE_SIZE and
> >>   supported anonymous THP)  and ``<state>`` is one of ``always``, ``madvise``,
> >>   ``never`` or ``inherit``.
> >> --
> >> 2.46.2
> >>
>

Thanks
barry

Re: [PATCH 1/3] mm: fix the format of the kernel parameter ``thp_anon=``

[David Hildenbrand]

On 27.10.24 18:36, Maíra Canal wrote:
> If we add ``thp_anon=32,64KB:always`` to the kernel command line, we
> will see the following error:

^ did you mean "64K" instead of "64KB" ?

> 
> [    0.000000] huge_memory: thp_anon=32,64K:always: error parsing string, ignoring setting
> 
> This happens because the correct format isn't ``thp_anon=<size>,<size>[KMG]:<state>```,
> as [KMG] must follow each number to especify its unit. So, the correct
> format is ``thp_anon=<size>[KMG],<size>[KMG]:<state>```.
> 
> Therefore, adjust the documentation to reflect the correct format of the
> parameter ``thp_anon=``.
> 
> Fixes: dd4d30d1cdbe ("mm: override mTHP "enabled" defaults at kernel cmdline")

As Barry says, this is a doc fix and we should make that clearer in the 
subject. With that:

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

-- 
Cheers,

David / dhildenb

[PATCH 2/3] mm: shmem: control THP support through the kernel command line

[Maíra Canal]

Add a new kernel command line to control the hugepage allocation policy
for the internal shmem mount, ``transparent_hugepage_shmem``. The
parameter is similar to ``transparent_hugepage`` and has the following
format:

transparent_hugepage_shmem=<policy>

where ``<policy>`` is one of the seven valid policies available for
shmem.

By configuring the default hugepage allocation policy for the internal
shmem mount, applications that use shmem, such as the DRM GEM objects,
can take advantage of mTHP before it's been configured through sysfs.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 .../admin-guide/kernel-parameters.txt         |  7 ++++
 Documentation/admin-guide/mm/transhuge.rst    |  6 +++
 mm/shmem.c                                    | 38 ++++++++++++++++++-
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1666576acc0e..acabb04d0dd4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6926,6 +6926,13 @@
 			See Documentation/admin-guide/mm/transhuge.rst
 			for more details.
 
+	transparent_hugepage_shmem= [KNL]
+			Format: [always|within_size|advise|never|deny|force]
+			Can be used to control the hugepage allocation policy for
+			the internal shmem mount.
+			See Documentation/admin-guide/mm/transhuge.rst
+			for more details.
+
 	trusted.source=	[KEYS]
 			Format: <string>
 			This parameter identifies the trust source as a backend
diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 745055c3dc09..9b5b02c4d1ab 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -326,6 +326,12 @@ PMD_ORDER THP policy will be overridden. If the policy for PMD_ORDER
 is not defined within a valid ``thp_anon``, its policy will default to
 ``never``.
 
+Similarly to ``transparent_hugepage``, you can control the hugepage
+allocation policy for the internal shmem mount by using the kernel parameter
+``transparent_hugepage_shmem=<policy>``, where ``<policy>`` is one of the
+seven valid policies for shmem (``always``, ``within_size``, ``advise``,
+``never``, ``deny``, and ``force``).
+
 Hugepages in tmpfs/shmem
 ========================
 
diff --git a/mm/shmem.c b/mm/shmem.c
index 44282a296c33..24cdeafd8260 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -582,7 +582,6 @@ static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index,
 	}
 }
 
-#if defined(CONFIG_SYSFS)
 static int shmem_parse_huge(const char *str)
 {
 	if (!strcmp(str, "never"))
@@ -599,7 +598,6 @@ static int shmem_parse_huge(const char *str)
 		return SHMEM_HUGE_FORCE;
 	return -EINVAL;
 }
-#endif
 
 #if defined(CONFIG_SYSFS) || defined(CONFIG_TMPFS)
 static const char *shmem_format_huge(int huge)
@@ -5174,6 +5172,42 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
 	__ATTR(shmem_enabled, 0644, thpsize_shmem_enabled_show, thpsize_shmem_enabled_store);
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE && CONFIG_SYSFS */
 
+#if defined(CONFIG_TRANSPARENT_HUGEPAGE)
+
+static int __init setup_transparent_hugepage_shmem(char *str)
+{
+	int huge, ret = 0;
+
+	if (!str)
+		goto out;
+
+	huge = shmem_parse_huge(str);
+	if (huge == -EINVAL)
+		goto out;
+
+	if (!has_transparent_hugepage() &&
+			huge != SHMEM_HUGE_NEVER && huge != SHMEM_HUGE_DENY) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Do not override huge allocation policy with non-PMD sized mTHP */
+	if (huge == SHMEM_HUGE_FORCE &&
+	    huge_shmem_orders_inherit != BIT(HPAGE_PMD_ORDER)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	shmem_huge = huge;
+	return 1;
+out:
+	pr_warn("transparent_hugepage_shmem= cannot parse, ignored\n");
+	return ret;
+}
+__setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
+
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+
 #else /* !CONFIG_SHMEM */
 
 /*
-- 
2.46.2

Re: [PATCH 2/3] mm: shmem: control THP support through the kernel command line

[Baolin Wang]

On 2024/10/28 01:36, Maíra Canal wrote:
> Add a new kernel command line to control the hugepage allocation policy
> for the internal shmem mount, ``transparent_hugepage_shmem``. The
> parameter is similar to ``transparent_hugepage`` and has the following
> format:
> 
> transparent_hugepage_shmem=<policy>
> 
> where ``<policy>`` is one of the seven valid policies available for
> shmem.
> 
> By configuring the default hugepage allocation policy for the internal
> shmem mount, applications that use shmem, such as the DRM GEM objects,
> can take advantage of mTHP before it's been configured through sysfs.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   .../admin-guide/kernel-parameters.txt         |  7 ++++
>   Documentation/admin-guide/mm/transhuge.rst    |  6 +++
>   mm/shmem.c                                    | 38 ++++++++++++++++++-
>   3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1666576acc0e..acabb04d0dd4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6926,6 +6926,13 @@
>   			See Documentation/admin-guide/mm/transhuge.rst
>   			for more details.
>   
> +	transparent_hugepage_shmem= [KNL]
> +			Format: [always|within_size|advise|never|deny|force]
> +			Can be used to control the hugepage allocation policy for
> +			the internal shmem mount.
> +			See Documentation/admin-guide/mm/transhuge.rst
> +			for more details.
> +
>   	trusted.source=	[KEYS]
>   			Format: <string>
>   			This parameter identifies the trust source as a backend
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 745055c3dc09..9b5b02c4d1ab 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -326,6 +326,12 @@ PMD_ORDER THP policy will be overridden. If the policy for PMD_ORDER
>   is not defined within a valid ``thp_anon``, its policy will default to
>   ``never``.
>   
> +Similarly to ``transparent_hugepage``, you can control the hugepage
> +allocation policy for the internal shmem mount by using the kernel parameter
> +``transparent_hugepage_shmem=<policy>``, where ``<policy>`` is one of the
> +seven valid policies for shmem (``always``, ``within_size``, ``advise``,
> +``never``, ``deny``, and ``force``).
> +
>   Hugepages in tmpfs/shmem
>   ========================
>   
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 44282a296c33..24cdeafd8260 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -582,7 +582,6 @@ static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index,
>   	}
>   }
>   
> -#if defined(CONFIG_SYSFS)
>   static int shmem_parse_huge(const char *str)
>   {
>   	if (!strcmp(str, "never"))
> @@ -599,7 +598,6 @@ static int shmem_parse_huge(const char *str)
>   		return SHMEM_HUGE_FORCE;
>   	return -EINVAL;
>   }
> -#endif
>   
>   #if defined(CONFIG_SYSFS) || defined(CONFIG_TMPFS)
>   static const char *shmem_format_huge(int huge)
> @@ -5174,6 +5172,42 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
>   	__ATTR(shmem_enabled, 0644, thpsize_shmem_enabled_show, thpsize_shmem_enabled_store);
>   #endif /* CONFIG_TRANSPARENT_HUGEPAGE && CONFIG_SYSFS */
>   
> +#if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> +
> +static int __init setup_transparent_hugepage_shmem(char *str)
> +{
> +	int huge, ret = 0;
> +
> +	if (!str)
> +		goto out;
> +
> +	huge = shmem_parse_huge(str);
> +	if (huge == -EINVAL)
> +		goto out;
> +
> +	if (!has_transparent_hugepage() &&
> +			huge != SHMEM_HUGE_NEVER && huge != SHMEM_HUGE_DENY) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Do not override huge allocation policy with non-PMD sized mTHP */
> +	if (huge == SHMEM_HUGE_FORCE &&
> +	    huge_shmem_orders_inherit != BIT(HPAGE_PMD_ORDER)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	shmem_huge = huge;

The code is similar to shmem_enabled_store(). Could you factor out the 
common parts into a helper function and reuse them?

> +	return 1;
> +out:
> +	pr_warn("transparent_hugepage_shmem= cannot parse, ignored\n");
> +	return ret;
> +}
> +__setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
> +
> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> +
>   #else /* !CONFIG_SHMEM */
>   
>   /*

[PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Maíra Canal]

Add the ``thp_shmem=`` kernel command line to allow specifying the
default policy of each supported shmem hugepage size. The kernel parameter
accepts the following format:

thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>

For example,

thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size

By configuring the default policy of several shmem huge pages, the user
can take advantage of mTHP before it's been configured through sysfs.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 .../admin-guide/kernel-parameters.txt         |  10 ++
 Documentation/admin-guide/mm/transhuge.rst    |  17 +++
 mm/shmem.c                                    | 109 +++++++++++++++++-
 3 files changed, 135 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index acabb04d0dd4..595fa096e28b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6700,6 +6700,16 @@
 			Force threading of all interrupt handlers except those
 			marked explicitly IRQF_NO_THREAD.
 
+	shmem_anon=	[KNL]
+			Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
+			Control the default policy of each hugepage size for the
+			internal shmem mount. <policy> is one of policies available
+			for the shmem mount ("always", "inherit", "never", "within_size",
+			and "advise").
+			It can be used multiple times for multiple shmem THP sizes.
+			See Documentation/admin-guide/mm/transhuge.rst for more
+			details.
+
 	topology=	[S390,EARLY]
 			Format: {off | on}
 			Specify if the kernel should make use of the cpu
diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 9b5b02c4d1ab..47e7fc30e22d 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
 seven valid policies for shmem (``always``, ``within_size``, ``advise``,
 ``never``, ``deny``, and ``force``).
 
+In the same manner as ``thp_anon`` controls each supported anonymous THP
+size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
+has the same format as ``thp_anon``, but also supports the policy
+``within_size``.
+
+``thp_shmem=`` may be specified multiple times to configure all THP sizes
+as required. If ``thp_shmem=`` is specified at least once, any shmem THP
+sizes not explicitly configured on the command line are implicitly set to
+``never``.
+
+``transparent_hugepage_shmem`` setting only affects the global toggle. If
+``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
+``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
+user, the PMD_ORDER hugepage policy will be overridden. If the policy for
+PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
+default to ``never``.
+
 Hugepages in tmpfs/shmem
 ========================
 
diff --git a/mm/shmem.c b/mm/shmem.c
index 24cdeafd8260..0a7a7d04f725 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always __read_mostly;
 static unsigned long huge_shmem_orders_madvise __read_mostly;
 static unsigned long huge_shmem_orders_inherit __read_mostly;
 static unsigned long huge_shmem_orders_within_size __read_mostly;
+static bool shmem_orders_configured __initdata;
 #endif
 
 #ifdef CONFIG_TMPFS
@@ -5013,7 +5014,8 @@ void __init shmem_init(void)
 	 * Default to setting PMD-sized THP to inherit the global setting and
 	 * disable all other multi-size THPs.
 	 */
-	huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
+	if (!shmem_orders_configured)
+		huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
 #endif
 	return;
 
@@ -5174,6 +5176,26 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
 
 #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
 
+static inline int get_order_from_str(const char *size_str)
+{
+	unsigned long size;
+	char *endptr;
+	int order;
+
+	size = memparse(size_str, &endptr);
+
+	if (!is_power_of_2(size))
+		goto err;
+	order = get_order(size);
+	if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT)
+		goto err;
+
+	return order;
+err:
+	pr_err("invalid size %s in thp_shmem boot parameter\n", size_str);
+	return -EINVAL;
+}
+
 static int __init setup_transparent_hugepage_shmem(char *str)
 {
 	int huge, ret = 0;
@@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
 }
 __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
 
+static char str_dup[PAGE_SIZE] __initdata;
+static int __init setup_thp_shmem(char *str)
+{
+	char *token, *range, *policy, *subtoken;
+	unsigned long always, inherit, madvise, within_size;
+	char *start_size, *end_size;
+	int start, end, nr;
+	char *p;
+
+	if (!str || strlen(str) + 1 > PAGE_SIZE)
+		goto err;
+	strcpy(str_dup, str);
+
+	always = huge_shmem_orders_always;
+	inherit = huge_shmem_orders_inherit;
+	madvise = huge_shmem_orders_madvise;
+	within_size = huge_shmem_orders_within_size;
+	p = str_dup;
+	while ((token = strsep(&p, ";")) != NULL) {
+		range = strsep(&token, ":");
+		policy = token;
+
+		if (!policy)
+			goto err;
+
+		while ((subtoken = strsep(&range, ",")) != NULL) {
+			if (strchr(subtoken, '-')) {
+				start_size = strsep(&subtoken, "-");
+				end_size = subtoken;
+
+				start = get_order_from_str(start_size);
+				end = get_order_from_str(end_size);
+			} else {
+				start = end = get_order_from_str(subtoken);
+			}
+
+			if (start < 0 || end < 0 || start > end)
+				goto err;
+
+			nr = end - start + 1;
+			if (!strcmp(policy, "always")) {
+				bitmap_set(&always, start, nr);
+				bitmap_clear(&inherit, start, nr);
+				bitmap_clear(&madvise, start, nr);
+				bitmap_clear(&within_size, start, nr);
+			} else if (!strcmp(policy, "advise")) {
+				bitmap_set(&madvise, start, nr);
+				bitmap_clear(&inherit, start, nr);
+				bitmap_clear(&always, start, nr);
+				bitmap_clear(&within_size, start, nr);
+			} else if (!strcmp(policy, "inherit")) {
+				bitmap_set(&inherit, start, nr);
+				bitmap_clear(&madvise, start, nr);
+				bitmap_clear(&always, start, nr);
+				bitmap_clear(&within_size, start, nr);
+			} else if (!strcmp(policy, "within_size")) {
+				bitmap_set(&within_size, start, nr);
+				bitmap_clear(&inherit, start, nr);
+				bitmap_clear(&madvise, start, nr);
+				bitmap_clear(&always, start, nr);
+			} else if (!strcmp(policy, "never")) {
+				bitmap_clear(&inherit, start, nr);
+				bitmap_clear(&madvise, start, nr);
+				bitmap_clear(&always, start, nr);
+				bitmap_clear(&within_size, start, nr);
+			} else {
+				pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
+				goto err;
+			}
+		}
+	}
+
+	huge_shmem_orders_always = always;
+	huge_shmem_orders_madvise = madvise;
+	huge_shmem_orders_inherit = inherit;
+	huge_shmem_orders_within_size = within_size;
+	shmem_orders_configured = true;
+	return 1;
+
+err:
+	pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
+	return 0;
+}
+__setup("thp_shmem=", setup_thp_shmem);
+
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 #else /* !CONFIG_SHMEM */
-- 
2.46.2

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Barry Song]

On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> Add the ``thp_shmem=`` kernel command line to allow specifying the
> default policy of each supported shmem hugepage size. The kernel parameter
> accepts the following format:
>
> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>
> For example,
>
> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
>
> By configuring the default policy of several shmem huge pages, the user
> can take advantage of mTHP before it's been configured through sysfs.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  10 ++
>  Documentation/admin-guide/mm/transhuge.rst    |  17 +++
>  mm/shmem.c                                    | 109 +++++++++++++++++-
>  3 files changed, 135 insertions(+), 1 deletion(-)
>

Hi Maíra,

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index acabb04d0dd4..595fa096e28b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6700,6 +6700,16 @@
>                         Force threading of all interrupt handlers except those
>                         marked explicitly IRQF_NO_THREAD.
>
> +       shmem_anon=     [KNL]
> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> +                       Control the default policy of each hugepage size for the
> +                       internal shmem mount. <policy> is one of policies available
> +                       for the shmem mount ("always", "inherit", "never", "within_size",
> +                       and "advise").
> +                       It can be used multiple times for multiple shmem THP sizes.
> +                       See Documentation/admin-guide/mm/transhuge.rst for more
> +                       details.

I'm not sure this is the right name. How about "thp_shmem"?

> +
>         topology=       [S390,EARLY]
>                         Format: {off | on}
>                         Specify if the kernel should make use of the cpu
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 9b5b02c4d1ab..47e7fc30e22d 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
>  seven valid policies for shmem (``always``, ``within_size``, ``advise``,
>  ``never``, ``deny``, and ``force``).
>
> +In the same manner as ``thp_anon`` controls each supported anonymous THP
> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
> +has the same format as ``thp_anon``, but also supports the policy
> +``within_size``.
> +
> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
> +sizes not explicitly configured on the command line are implicitly set to
> +``never``.
> +
> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
> +default to ``never``.
> +
>  Hugepages in tmpfs/shmem
>  ========================
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 24cdeafd8260..0a7a7d04f725 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always __read_mostly;
>  static unsigned long huge_shmem_orders_madvise __read_mostly;
>  static unsigned long huge_shmem_orders_inherit __read_mostly;
>  static unsigned long huge_shmem_orders_within_size __read_mostly;
> +static bool shmem_orders_configured __initdata;
>  #endif
>
>  #ifdef CONFIG_TMPFS
> @@ -5013,7 +5014,8 @@ void __init shmem_init(void)
>          * Default to setting PMD-sized THP to inherit the global setting and
>          * disable all other multi-size THPs.
>          */
> -       huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
> +       if (!shmem_orders_configured)
> +               huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
>  #endif
>         return;
>
> @@ -5174,6 +5176,26 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
>
>  #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
>
> +static inline int get_order_from_str(const char *size_str)
> +{
> +       unsigned long size;
> +       char *endptr;
> +       int order;
> +
> +       size = memparse(size_str, &endptr);
> +
> +       if (!is_power_of_2(size))
> +               goto err;
> +       order = get_order(size);
> +       if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT)
> +               goto err;
> +
> +       return order;
> +err:
> +       pr_err("invalid size %s in thp_shmem boot parameter\n", size_str);
> +       return -EINVAL;
> +}
> +
>  static int __init setup_transparent_hugepage_shmem(char *str)
>  {
>         int huge, ret = 0;
> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
>  }
>  __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
>
> +static char str_dup[PAGE_SIZE] __initdata;
> +static int __init setup_thp_shmem(char *str)
> +{
> +       char *token, *range, *policy, *subtoken;
> +       unsigned long always, inherit, madvise, within_size;
> +       char *start_size, *end_size;
> +       int start, end, nr;
> +       char *p;
> +
> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
> +               goto err;
> +       strcpy(str_dup, str);
> +
> +       always = huge_shmem_orders_always;
> +       inherit = huge_shmem_orders_inherit;
> +       madvise = huge_shmem_orders_madvise;
> +       within_size = huge_shmem_orders_within_size;
> +       p = str_dup;
> +       while ((token = strsep(&p, ";")) != NULL) {
> +               range = strsep(&token, ":");
> +               policy = token;
> +
> +               if (!policy)
> +                       goto err;
> +
> +               while ((subtoken = strsep(&range, ",")) != NULL) {
> +                       if (strchr(subtoken, '-')) {
> +                               start_size = strsep(&subtoken, "-");
> +                               end_size = subtoken;
> +
> +                               start = get_order_from_str(start_size);
> +                               end = get_order_from_str(end_size);
> +                       } else {
> +                               start = end = get_order_from_str(subtoken);
> +                       }
> +
> +                       if (start < 0 || end < 0 || start > end)
> +                               goto err;
> +
> +                       nr = end - start + 1;
> +                       if (!strcmp(policy, "always")) {
> +                               bitmap_set(&always, start, nr);
> +                               bitmap_clear(&inherit, start, nr);
> +                               bitmap_clear(&madvise, start, nr);
> +                               bitmap_clear(&within_size, start, nr);
> +                       } else if (!strcmp(policy, "advise")) {
> +                               bitmap_set(&madvise, start, nr);
> +                               bitmap_clear(&inherit, start, nr);
> +                               bitmap_clear(&always, start, nr);
> +                               bitmap_clear(&within_size, start, nr);
> +                       } else if (!strcmp(policy, "inherit")) {
> +                               bitmap_set(&inherit, start, nr);
> +                               bitmap_clear(&madvise, start, nr);
> +                               bitmap_clear(&always, start, nr);
> +                               bitmap_clear(&within_size, start, nr);
> +                       } else if (!strcmp(policy, "within_size")) {
> +                               bitmap_set(&within_size, start, nr);
> +                               bitmap_clear(&inherit, start, nr);
> +                               bitmap_clear(&madvise, start, nr);
> +                               bitmap_clear(&always, start, nr);
> +                       } else if (!strcmp(policy, "never")) {
> +                               bitmap_clear(&inherit, start, nr);
> +                               bitmap_clear(&madvise, start, nr);
> +                               bitmap_clear(&always, start, nr);
> +                               bitmap_clear(&within_size, start, nr);
> +                       } else {
> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
> +                               goto err;
> +                       }
> +               }
> +       }
> +
> +       huge_shmem_orders_always = always;
> +       huge_shmem_orders_madvise = madvise;
> +       huge_shmem_orders_inherit = inherit;
> +       huge_shmem_orders_within_size = within_size;
> +       shmem_orders_configured = true;
> +       return 1;
> +
> +err:
> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
> +       return 0;
> +}

Can we share source code with thp_anon since there's a lot of duplication?

> +__setup("thp_shmem=", setup_thp_shmem);
> +
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>
>  #else /* !CONFIG_SHMEM */
> --
> 2.46.2
>

Thanks
barry

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Lance Yang]

Hi Maíra,

On Mon, Oct 28, 2024 at 5:54 AM Barry Song <21cnbao@gmail.com> wrote:
>
> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
> >
> > Add the ``thp_shmem=`` kernel command line to allow specifying the
> > default policy of each supported shmem hugepage size. The kernel parameter
> > accepts the following format:
> >
> > thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> >
> > For example,
> >
> > thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
> >
> > By configuring the default policy of several shmem huge pages, the user
> > can take advantage of mTHP before it's been configured through sysfs.
> >
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  10 ++
> >  Documentation/admin-guide/mm/transhuge.rst    |  17 +++
> >  mm/shmem.c                                    | 109 +++++++++++++++++-
> >  3 files changed, 135 insertions(+), 1 deletion(-)
> >
>
> Hi Maíra,
>
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index acabb04d0dd4..595fa096e28b 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6700,6 +6700,16 @@
> >                         Force threading of all interrupt handlers except those
> >                         marked explicitly IRQF_NO_THREAD.
> >
> > +       shmem_anon=     [KNL]
> > +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> > +                       Control the default policy of each hugepage size for the
> > +                       internal shmem mount. <policy> is one of policies available
> > +                       for the shmem mount ("always", "inherit", "never", "within_size",
> > +                       and "advise").
> > +                       It can be used multiple times for multiple shmem THP sizes.
> > +                       See Documentation/admin-guide/mm/transhuge.rst for more
> > +                       details.
>
> I'm not sure this is the right name. How about "thp_shmem"?

+1

IHMO, it seems like 'thp_shmem' would be better, as it appears to fit well
with 'thp_anon' in naming style ;)

Thanks,
Lance

>
> > +
> >         topology=       [S390,EARLY]
> >                         Format: {off | on}
> >                         Specify if the kernel should make use of the cpu
> > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> > index 9b5b02c4d1ab..47e7fc30e22d 100644
> > --- a/Documentation/admin-guide/mm/transhuge.rst
> > +++ b/Documentation/admin-guide/mm/transhuge.rst
> > @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
> >  seven valid policies for shmem (``always``, ``within_size``, ``advise``,
> >  ``never``, ``deny``, and ``force``).
> >
> > +In the same manner as ``thp_anon`` controls each supported anonymous THP
> > +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
> > +has the same format as ``thp_anon``, but also supports the policy
> > +``within_size``.
> > +
> > +``thp_shmem=`` may be specified multiple times to configure all THP sizes
> > +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
> > +sizes not explicitly configured on the command line are implicitly set to
> > +``never``.
> > +
> > +``transparent_hugepage_shmem`` setting only affects the global toggle. If
> > +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
> > +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
> > +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
> > +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
> > +default to ``never``.
> > +
> >  Hugepages in tmpfs/shmem
> >  ========================
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 24cdeafd8260..0a7a7d04f725 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always __read_mostly;
> >  static unsigned long huge_shmem_orders_madvise __read_mostly;
> >  static unsigned long huge_shmem_orders_inherit __read_mostly;
> >  static unsigned long huge_shmem_orders_within_size __read_mostly;
> > +static bool shmem_orders_configured __initdata;
> >  #endif
> >
> >  #ifdef CONFIG_TMPFS
> > @@ -5013,7 +5014,8 @@ void __init shmem_init(void)
> >          * Default to setting PMD-sized THP to inherit the global setting and
> >          * disable all other multi-size THPs.
> >          */
> > -       huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
> > +       if (!shmem_orders_configured)
> > +               huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
> >  #endif
> >         return;
> >
> > @@ -5174,6 +5176,26 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
> >
> >  #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> >
> > +static inline int get_order_from_str(const char *size_str)
> > +{
> > +       unsigned long size;
> > +       char *endptr;
> > +       int order;
> > +
> > +       size = memparse(size_str, &endptr);
> > +
> > +       if (!is_power_of_2(size))
> > +               goto err;
> > +       order = get_order(size);
> > +       if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT)
> > +               goto err;
> > +
> > +       return order;
> > +err:
> > +       pr_err("invalid size %s in thp_shmem boot parameter\n", size_str);
> > +       return -EINVAL;
> > +}
> > +
> >  static int __init setup_transparent_hugepage_shmem(char *str)
> >  {
> >         int huge, ret = 0;
> > @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
> >  }
> >  __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
> >
> > +static char str_dup[PAGE_SIZE] __initdata;
> > +static int __init setup_thp_shmem(char *str)
> > +{
> > +       char *token, *range, *policy, *subtoken;
> > +       unsigned long always, inherit, madvise, within_size;
> > +       char *start_size, *end_size;
> > +       int start, end, nr;
> > +       char *p;
> > +
> > +       if (!str || strlen(str) + 1 > PAGE_SIZE)
> > +               goto err;
> > +       strcpy(str_dup, str);
> > +
> > +       always = huge_shmem_orders_always;
> > +       inherit = huge_shmem_orders_inherit;
> > +       madvise = huge_shmem_orders_madvise;
> > +       within_size = huge_shmem_orders_within_size;
> > +       p = str_dup;
> > +       while ((token = strsep(&p, ";")) != NULL) {
> > +               range = strsep(&token, ":");
> > +               policy = token;
> > +
> > +               if (!policy)
> > +                       goto err;
> > +
> > +               while ((subtoken = strsep(&range, ",")) != NULL) {
> > +                       if (strchr(subtoken, '-')) {
> > +                               start_size = strsep(&subtoken, "-");
> > +                               end_size = subtoken;
> > +
> > +                               start = get_order_from_str(start_size);
> > +                               end = get_order_from_str(end_size);
> > +                       } else {
> > +                               start = end = get_order_from_str(subtoken);
> > +                       }
> > +
> > +                       if (start < 0 || end < 0 || start > end)
> > +                               goto err;
> > +
> > +                       nr = end - start + 1;
> > +                       if (!strcmp(policy, "always")) {
> > +                               bitmap_set(&always, start, nr);
> > +                               bitmap_clear(&inherit, start, nr);
> > +                               bitmap_clear(&madvise, start, nr);
> > +                               bitmap_clear(&within_size, start, nr);
> > +                       } else if (!strcmp(policy, "advise")) {
> > +                               bitmap_set(&madvise, start, nr);
> > +                               bitmap_clear(&inherit, start, nr);
> > +                               bitmap_clear(&always, start, nr);
> > +                               bitmap_clear(&within_size, start, nr);
> > +                       } else if (!strcmp(policy, "inherit")) {
> > +                               bitmap_set(&inherit, start, nr);
> > +                               bitmap_clear(&madvise, start, nr);
> > +                               bitmap_clear(&always, start, nr);
> > +                               bitmap_clear(&within_size, start, nr);
> > +                       } else if (!strcmp(policy, "within_size")) {
> > +                               bitmap_set(&within_size, start, nr);
> > +                               bitmap_clear(&inherit, start, nr);
> > +                               bitmap_clear(&madvise, start, nr);
> > +                               bitmap_clear(&always, start, nr);
> > +                       } else if (!strcmp(policy, "never")) {
> > +                               bitmap_clear(&inherit, start, nr);
> > +                               bitmap_clear(&madvise, start, nr);
> > +                               bitmap_clear(&always, start, nr);
> > +                               bitmap_clear(&within_size, start, nr);
> > +                       } else {
> > +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
> > +                               goto err;
> > +                       }
> > +               }
> > +       }
> > +
> > +       huge_shmem_orders_always = always;
> > +       huge_shmem_orders_madvise = madvise;
> > +       huge_shmem_orders_inherit = inherit;
> > +       huge_shmem_orders_within_size = within_size;
> > +       shmem_orders_configured = true;
> > +       return 1;
> > +
> > +err:
> > +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
> > +       return 0;
> > +}
>
> Can we share source code with thp_anon since there's a lot of duplication?


>
> > +__setup("thp_shmem=", setup_thp_shmem);
> > +
> >  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> >
> >  #else /* !CONFIG_SHMEM */
> > --
> > 2.46.2
> >
>
> Thanks
> barry

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Maíra Canal]

Hi Barry,

On 27/10/24 18:54, Barry Song wrote:
> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Add the ``thp_shmem=`` kernel command line to allow specifying the
>> default policy of each supported shmem hugepage size. The kernel parameter
>> accepts the following format:
>>
>> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>>
>> For example,
>>
>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
>>
>> By configuring the default policy of several shmem huge pages, the user
>> can take advantage of mTHP before it's been configured through sysfs.
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   .../admin-guide/kernel-parameters.txt         |  10 ++
>>   Documentation/admin-guide/mm/transhuge.rst    |  17 +++
>>   mm/shmem.c                                    | 109 +++++++++++++++++-
>>   3 files changed, 135 insertions(+), 1 deletion(-)
>>
> 
> Hi Maíra,
> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index acabb04d0dd4..595fa096e28b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -6700,6 +6700,16 @@
>>                          Force threading of all interrupt handlers except those
>>                          marked explicitly IRQF_NO_THREAD.
>>
>> +       shmem_anon=     [KNL]
>> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>> +                       Control the default policy of each hugepage size for the
>> +                       internal shmem mount. <policy> is one of policies available
>> +                       for the shmem mount ("always", "inherit", "never", "within_size",
>> +                       and "advise").
>> +                       It can be used multiple times for multiple shmem THP sizes.
>> +                       See Documentation/admin-guide/mm/transhuge.rst for more
>> +                       details.
> 
> I'm not sure this is the right name. How about "thp_shmem"?

Oops, sorry about that.

> 
>> +
>>          topology=       [S390,EARLY]
>>                          Format: {off | on}
>>                          Specify if the kernel should make use of the cpu
>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>> index 9b5b02c4d1ab..47e7fc30e22d 100644
>> --- a/Documentation/admin-guide/mm/transhuge.rst
>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
>>   seven valid policies for shmem (``always``, ``within_size``, ``advise``,
>>   ``never``, ``deny``, and ``force``).
>>
>> +In the same manner as ``thp_anon`` controls each supported anonymous THP
>> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
>> +has the same format as ``thp_anon``, but also supports the policy
>> +``within_size``.
>> +
>> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
>> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
>> +sizes not explicitly configured on the command line are implicitly set to
>> +``never``.
>> +
>> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
>> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
>> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
>> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
>> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
>> +default to ``never``.
>> +
>>   Hugepages in tmpfs/shmem
>>   ========================
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 24cdeafd8260..0a7a7d04f725 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always __read_mostly;
>>   static unsigned long huge_shmem_orders_madvise __read_mostly;
>>   static unsigned long huge_shmem_orders_inherit __read_mostly;
>>   static unsigned long huge_shmem_orders_within_size __read_mostly;
>> +static bool shmem_orders_configured __initdata;
>>   #endif
>>
>>   #ifdef CONFIG_TMPFS
>> @@ -5013,7 +5014,8 @@ void __init shmem_init(void)
>>           * Default to setting PMD-sized THP to inherit the global setting and
>>           * disable all other multi-size THPs.
>>           */
>> -       huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
>> +       if (!shmem_orders_configured)
>> +               huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
>>   #endif
>>          return;
>>
>> @@ -5174,6 +5176,26 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
>>
>>   #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
>>
>> +static inline int get_order_from_str(const char *size_str)
>> +{
>> +       unsigned long size;
>> +       char *endptr;
>> +       int order;
>> +
>> +       size = memparse(size_str, &endptr);
>> +
>> +       if (!is_power_of_2(size))
>> +               goto err;
>> +       order = get_order(size);
>> +       if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT)
>> +               goto err;
>> +
>> +       return order;
>> +err:
>> +       pr_err("invalid size %s in thp_shmem boot parameter\n", size_str);
>> +       return -EINVAL;
>> +}
>> +
>>   static int __init setup_transparent_hugepage_shmem(char *str)
>>   {
>>          int huge, ret = 0;
>> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
>>   }
>>   __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
>>
>> +static char str_dup[PAGE_SIZE] __initdata;
>> +static int __init setup_thp_shmem(char *str)
>> +{
>> +       char *token, *range, *policy, *subtoken;
>> +       unsigned long always, inherit, madvise, within_size;
>> +       char *start_size, *end_size;
>> +       int start, end, nr;
>> +       char *p;
>> +
>> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
>> +               goto err;
>> +       strcpy(str_dup, str);
>> +
>> +       always = huge_shmem_orders_always;
>> +       inherit = huge_shmem_orders_inherit;
>> +       madvise = huge_shmem_orders_madvise;
>> +       within_size = huge_shmem_orders_within_size;
>> +       p = str_dup;
>> +       while ((token = strsep(&p, ";")) != NULL) {
>> +               range = strsep(&token, ":");
>> +               policy = token;
>> +
>> +               if (!policy)
>> +                       goto err;
>> +
>> +               while ((subtoken = strsep(&range, ",")) != NULL) {
>> +                       if (strchr(subtoken, '-')) {
>> +                               start_size = strsep(&subtoken, "-");
>> +                               end_size = subtoken;
>> +
>> +                               start = get_order_from_str(start_size);
>> +                               end = get_order_from_str(end_size);
>> +                       } else {
>> +                               start = end = get_order_from_str(subtoken);
>> +                       }
>> +
>> +                       if (start < 0 || end < 0 || start > end)
>> +                               goto err;
>> +
>> +                       nr = end - start + 1;
>> +                       if (!strcmp(policy, "always")) {
>> +                               bitmap_set(&always, start, nr);
>> +                               bitmap_clear(&inherit, start, nr);
>> +                               bitmap_clear(&madvise, start, nr);
>> +                               bitmap_clear(&within_size, start, nr);
>> +                       } else if (!strcmp(policy, "advise")) {
>> +                               bitmap_set(&madvise, start, nr);
>> +                               bitmap_clear(&inherit, start, nr);
>> +                               bitmap_clear(&always, start, nr);
>> +                               bitmap_clear(&within_size, start, nr);
>> +                       } else if (!strcmp(policy, "inherit")) {
>> +                               bitmap_set(&inherit, start, nr);
>> +                               bitmap_clear(&madvise, start, nr);
>> +                               bitmap_clear(&always, start, nr);
>> +                               bitmap_clear(&within_size, start, nr);
>> +                       } else if (!strcmp(policy, "within_size")) {
>> +                               bitmap_set(&within_size, start, nr);
>> +                               bitmap_clear(&inherit, start, nr);
>> +                               bitmap_clear(&madvise, start, nr);
>> +                               bitmap_clear(&always, start, nr);
>> +                       } else if (!strcmp(policy, "never")) {
>> +                               bitmap_clear(&inherit, start, nr);
>> +                               bitmap_clear(&madvise, start, nr);
>> +                               bitmap_clear(&always, start, nr);
>> +                               bitmap_clear(&within_size, start, nr);
>> +                       } else {
>> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
>> +                               goto err;
>> +                       }
>> +               }
>> +       }
>> +
>> +       huge_shmem_orders_always = always;
>> +       huge_shmem_orders_madvise = madvise;
>> +       huge_shmem_orders_inherit = inherit;
>> +       huge_shmem_orders_within_size = within_size;
>> +       shmem_orders_configured = true;
>> +       return 1;
>> +
>> +err:
>> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
>> +       return 0;
>> +}
> 
> Can we share source code with thp_anon since there's a lot of duplication?

I'm not a regular mm contributor and I'm most usually around drivers, so
I don't know exactly here I could add shared code. Should I add the
headers to "internal.h"?

Best Regards,
- Maíra

> 
>> +__setup("thp_shmem=", setup_thp_shmem);
>> +
>>   #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>
>>   #else /* !CONFIG_SHMEM */
>> --
>> 2.46.2
>>
> 
> Thanks
> barry

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Barry Song]

On Mon, Oct 28, 2024 at 6:10 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Barry,
>
> On 27/10/24 18:54, Barry Song wrote:
> > On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> Add the ``thp_shmem=`` kernel command line to allow specifying the
> >> default policy of each supported shmem hugepage size. The kernel parameter
> >> accepts the following format:
> >>
> >> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> >>
> >> For example,
> >>
> >> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
> >>
> >> By configuring the default policy of several shmem huge pages, the user
> >> can take advantage of mTHP before it's been configured through sysfs.
> >>
> >> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >> ---
> >>   .../admin-guide/kernel-parameters.txt         |  10 ++
> >>   Documentation/admin-guide/mm/transhuge.rst    |  17 +++
> >>   mm/shmem.c                                    | 109 +++++++++++++++++-
> >>   3 files changed, 135 insertions(+), 1 deletion(-)
> >>
> >
> > Hi Maíra,
> >
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index acabb04d0dd4..595fa096e28b 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -6700,6 +6700,16 @@
> >>                          Force threading of all interrupt handlers except those
> >>                          marked explicitly IRQF_NO_THREAD.
> >>
> >> +       shmem_anon=     [KNL]
> >> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> >> +                       Control the default policy of each hugepage size for the
> >> +                       internal shmem mount. <policy> is one of policies available
> >> +                       for the shmem mount ("always", "inherit", "never", "within_size",
> >> +                       and "advise").
> >> +                       It can be used multiple times for multiple shmem THP sizes.
> >> +                       See Documentation/admin-guide/mm/transhuge.rst for more
> >> +                       details.
> >
> > I'm not sure this is the right name. How about "thp_shmem"?
>
> Oops, sorry about that.
>
> >
> >> +
> >>          topology=       [S390,EARLY]
> >>                          Format: {off | on}
> >>                          Specify if the kernel should make use of the cpu
> >> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >> index 9b5b02c4d1ab..47e7fc30e22d 100644
> >> --- a/Documentation/admin-guide/mm/transhuge.rst
> >> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
> >>   seven valid policies for shmem (``always``, ``within_size``, ``advise``,
> >>   ``never``, ``deny``, and ``force``).
> >>
> >> +In the same manner as ``thp_anon`` controls each supported anonymous THP
> >> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
> >> +has the same format as ``thp_anon``, but also supports the policy
> >> +``within_size``.
> >> +
> >> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
> >> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
> >> +sizes not explicitly configured on the command line are implicitly set to
> >> +``never``.
> >> +
> >> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
> >> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
> >> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
> >> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
> >> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
> >> +default to ``never``.
> >> +
> >>   Hugepages in tmpfs/shmem
> >>   ========================
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 24cdeafd8260..0a7a7d04f725 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always __read_mostly;
> >>   static unsigned long huge_shmem_orders_madvise __read_mostly;
> >>   static unsigned long huge_shmem_orders_inherit __read_mostly;
> >>   static unsigned long huge_shmem_orders_within_size __read_mostly;
> >> +static bool shmem_orders_configured __initdata;
> >>   #endif
> >>
> >>   #ifdef CONFIG_TMPFS
> >> @@ -5013,7 +5014,8 @@ void __init shmem_init(void)
> >>           * Default to setting PMD-sized THP to inherit the global setting and
> >>           * disable all other multi-size THPs.
> >>           */
> >> -       huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
> >> +       if (!shmem_orders_configured)
> >> +               huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER);
> >>   #endif
> >>          return;
> >>
> >> @@ -5174,6 +5176,26 @@ struct kobj_attribute thpsize_shmem_enabled_attr =
> >>
> >>   #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> >>
> >> +static inline int get_order_from_str(const char *size_str)
> >> +{
> >> +       unsigned long size;
> >> +       char *endptr;
> >> +       int order;
> >> +
> >> +       size = memparse(size_str, &endptr);
> >> +
> >> +       if (!is_power_of_2(size))
> >> +               goto err;
> >> +       order = get_order(size);
> >> +       if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT)
> >> +               goto err;
> >> +
> >> +       return order;
> >> +err:
> >> +       pr_err("invalid size %s in thp_shmem boot parameter\n", size_str);
> >> +       return -EINVAL;
> >> +}
> >> +
> >>   static int __init setup_transparent_hugepage_shmem(char *str)
> >>   {
> >>          int huge, ret = 0;
> >> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
> >>   }
> >>   __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
> >>
> >> +static char str_dup[PAGE_SIZE] __initdata;
> >> +static int __init setup_thp_shmem(char *str)
> >> +{
> >> +       char *token, *range, *policy, *subtoken;
> >> +       unsigned long always, inherit, madvise, within_size;
> >> +       char *start_size, *end_size;
> >> +       int start, end, nr;
> >> +       char *p;
> >> +
> >> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
> >> +               goto err;
> >> +       strcpy(str_dup, str);
> >> +
> >> +       always = huge_shmem_orders_always;
> >> +       inherit = huge_shmem_orders_inherit;
> >> +       madvise = huge_shmem_orders_madvise;
> >> +       within_size = huge_shmem_orders_within_size;
> >> +       p = str_dup;
> >> +       while ((token = strsep(&p, ";")) != NULL) {
> >> +               range = strsep(&token, ":");
> >> +               policy = token;
> >> +
> >> +               if (!policy)
> >> +                       goto err;
> >> +
> >> +               while ((subtoken = strsep(&range, ",")) != NULL) {
> >> +                       if (strchr(subtoken, '-')) {
> >> +                               start_size = strsep(&subtoken, "-");
> >> +                               end_size = subtoken;
> >> +
> >> +                               start = get_order_from_str(start_size);
> >> +                               end = get_order_from_str(end_size);
> >> +                       } else {
> >> +                               start = end = get_order_from_str(subtoken);
> >> +                       }
> >> +
> >> +                       if (start < 0 || end < 0 || start > end)
> >> +                               goto err;
> >> +
> >> +                       nr = end - start + 1;
> >> +                       if (!strcmp(policy, "always")) {
> >> +                               bitmap_set(&always, start, nr);
> >> +                               bitmap_clear(&inherit, start, nr);
> >> +                               bitmap_clear(&madvise, start, nr);
> >> +                               bitmap_clear(&within_size, start, nr);
> >> +                       } else if (!strcmp(policy, "advise")) {
> >> +                               bitmap_set(&madvise, start, nr);
> >> +                               bitmap_clear(&inherit, start, nr);
> >> +                               bitmap_clear(&always, start, nr);
> >> +                               bitmap_clear(&within_size, start, nr);
> >> +                       } else if (!strcmp(policy, "inherit")) {
> >> +                               bitmap_set(&inherit, start, nr);
> >> +                               bitmap_clear(&madvise, start, nr);
> >> +                               bitmap_clear(&always, start, nr);
> >> +                               bitmap_clear(&within_size, start, nr);
> >> +                       } else if (!strcmp(policy, "within_size")) {
> >> +                               bitmap_set(&within_size, start, nr);
> >> +                               bitmap_clear(&inherit, start, nr);
> >> +                               bitmap_clear(&madvise, start, nr);
> >> +                               bitmap_clear(&always, start, nr);
> >> +                       } else if (!strcmp(policy, "never")) {
> >> +                               bitmap_clear(&inherit, start, nr);
> >> +                               bitmap_clear(&madvise, start, nr);
> >> +                               bitmap_clear(&always, start, nr);
> >> +                               bitmap_clear(&within_size, start, nr);
> >> +                       } else {
> >> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
> >> +                               goto err;
> >> +                       }
> >> +               }
> >> +       }
> >> +
> >> +       huge_shmem_orders_always = always;
> >> +       huge_shmem_orders_madvise = madvise;
> >> +       huge_shmem_orders_inherit = inherit;
> >> +       huge_shmem_orders_within_size = within_size;
> >> +       shmem_orders_configured = true;
> >> +       return 1;
> >> +
> >> +err:
> >> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
> >> +       return 0;
> >> +}
> >
> > Can we share source code with thp_anon since there's a lot of duplication?
>
> I'm not a regular mm contributor and I'm most usually around drivers, so
> I don't know exactly here I could add shared code. Should I add the
> headers to "internal.h"?

My comment isn't related to drivers or memory management. It's solely about
avoiding code duplication. For example, we could create a shared function to
handle both controls, reducing redundant code :-)

>
> Best Regards,
> - Maíra
>
> >
> >> +__setup("thp_shmem=", setup_thp_shmem);
> >> +
> >>   #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> >>
> >>   #else /* !CONFIG_SHMEM */
> >> --
> >> 2.46.2
> >>
> >
> > Thanks
> > barry
>

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Maíra Canal]

Hi Barry,

On 28/10/24 08:09, Barry Song wrote:
> On Mon, Oct 28, 2024 at 6:10 PM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Hi Barry,
>>
>> On 27/10/24 18:54, Barry Song wrote:
>>> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>>>>
>>>> Add the ``thp_shmem=`` kernel command line to allow specifying the
>>>> default policy of each supported shmem hugepage size. The kernel parameter
>>>> accepts the following format:
>>>>
>>>> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>>>>
>>>> For example,
>>>>
>>>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
>>>>
>>>> By configuring the default policy of several shmem huge pages, the user
>>>> can take advantage of mTHP before it's been configured through sysfs.
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>> ---
>>>>    .../admin-guide/kernel-parameters.txt         |  10 ++
>>>>    Documentation/admin-guide/mm/transhuge.rst    |  17 +++
>>>>    mm/shmem.c                                    | 109 +++++++++++++++++-
>>>>    3 files changed, 135 insertions(+), 1 deletion(-)
>>>>
>>>
>>> Hi Maíra,
>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index acabb04d0dd4..595fa096e28b 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -6700,6 +6700,16 @@
>>>>                           Force threading of all interrupt handlers except those
>>>>                           marked explicitly IRQF_NO_THREAD.
>>>>
>>>> +       shmem_anon=     [KNL]
>>>> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>>>> +                       Control the default policy of each hugepage size for the
>>>> +                       internal shmem mount. <policy> is one of policies available
>>>> +                       for the shmem mount ("always", "inherit", "never", "within_size",
>>>> +                       and "advise").
>>>> +                       It can be used multiple times for multiple shmem THP sizes.
>>>> +                       See Documentation/admin-guide/mm/transhuge.rst for more
>>>> +                       details.
>>>
>>> I'm not sure this is the right name. How about "thp_shmem"?
>>
>> Oops, sorry about that.
>>
>>>
>>>> +
>>>>           topology=       [S390,EARLY]
>>>>                           Format: {off | on}
>>>>                           Specify if the kernel should make use of the cpu
>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>>> index 9b5b02c4d1ab..47e7fc30e22d 100644
>>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>>> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
>>>>    seven valid policies for shmem (``always``, ``within_size``, ``advise``,
>>>>    ``never``, ``deny``, and ``force``).
>>>>
>>>> +In the same manner as ``thp_anon`` controls each supported anonymous THP
>>>> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
>>>> +has the same format as ``thp_anon``, but also supports the policy
>>>> +``within_size``.
>>>> +
>>>> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
>>>> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
>>>> +sizes not explicitly configured on the command line are implicitly set to
>>>> +``never``.
>>>> +
>>>> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
>>>> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
>>>> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
>>>> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
>>>> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
>>>> +default to ``never``.
>>>> +
>>>>    Hugepages in tmpfs/shmem
>>>>    ========================
>>>>
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 24cdeafd8260..0a7a7d04f725 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c

[...]

>>>>    static int __init setup_transparent_hugepage_shmem(char *str)
>>>>    {
>>>>           int huge, ret = 0;
>>>> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
>>>>    }
>>>>    __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
>>>>
>>>> +static char str_dup[PAGE_SIZE] __initdata;
>>>> +static int __init setup_thp_shmem(char *str)
>>>> +{
>>>> +       char *token, *range, *policy, *subtoken;
>>>> +       unsigned long always, inherit, madvise, within_size;
>>>> +       char *start_size, *end_size;
>>>> +       int start, end, nr;
>>>> +       char *p;
>>>> +
>>>> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
>>>> +               goto err;
>>>> +       strcpy(str_dup, str);
>>>> +
>>>> +       always = huge_shmem_orders_always;
>>>> +       inherit = huge_shmem_orders_inherit;
>>>> +       madvise = huge_shmem_orders_madvise;
>>>> +       within_size = huge_shmem_orders_within_size;
>>>> +       p = str_dup;
>>>> +       while ((token = strsep(&p, ";")) != NULL) {
>>>> +               range = strsep(&token, ":");
>>>> +               policy = token;
>>>> +
>>>> +               if (!policy)
>>>> +                       goto err;
>>>> +
>>>> +               while ((subtoken = strsep(&range, ",")) != NULL) {
>>>> +                       if (strchr(subtoken, '-')) {
>>>> +                               start_size = strsep(&subtoken, "-");
>>>> +                               end_size = subtoken;
>>>> +
>>>> +                               start = get_order_from_str(start_size);
>>>> +                               end = get_order_from_str(end_size);
>>>> +                       } else {
>>>> +                               start = end = get_order_from_str(subtoken);
>>>> +                       }
>>>> +
>>>> +                       if (start < 0 || end < 0 || start > end)
>>>> +                               goto err;
>>>> +
>>>> +                       nr = end - start + 1;
>>>> +                       if (!strcmp(policy, "always")) {
>>>> +                               bitmap_set(&always, start, nr);
>>>> +                               bitmap_clear(&inherit, start, nr);
>>>> +                               bitmap_clear(&madvise, start, nr);
>>>> +                               bitmap_clear(&within_size, start, nr);
>>>> +                       } else if (!strcmp(policy, "advise")) {
>>>> +                               bitmap_set(&madvise, start, nr);
>>>> +                               bitmap_clear(&inherit, start, nr);
>>>> +                               bitmap_clear(&always, start, nr);
>>>> +                               bitmap_clear(&within_size, start, nr);
>>>> +                       } else if (!strcmp(policy, "inherit")) {
>>>> +                               bitmap_set(&inherit, start, nr);
>>>> +                               bitmap_clear(&madvise, start, nr);
>>>> +                               bitmap_clear(&always, start, nr);
>>>> +                               bitmap_clear(&within_size, start, nr);
>>>> +                       } else if (!strcmp(policy, "within_size")) {
>>>> +                               bitmap_set(&within_size, start, nr);
>>>> +                               bitmap_clear(&inherit, start, nr);
>>>> +                               bitmap_clear(&madvise, start, nr);
>>>> +                               bitmap_clear(&always, start, nr);
>>>> +                       } else if (!strcmp(policy, "never")) {
>>>> +                               bitmap_clear(&inherit, start, nr);
>>>> +                               bitmap_clear(&madvise, start, nr);
>>>> +                               bitmap_clear(&always, start, nr);
>>>> +                               bitmap_clear(&within_size, start, nr);
>>>> +                       } else {
>>>> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
>>>> +                               goto err;
>>>> +                       }
>>>> +               }
>>>> +       }
>>>> +
>>>> +       huge_shmem_orders_always = always;
>>>> +       huge_shmem_orders_madvise = madvise;
>>>> +       huge_shmem_orders_inherit = inherit;
>>>> +       huge_shmem_orders_within_size = within_size;
>>>> +       shmem_orders_configured = true;
>>>> +       return 1;
>>>> +
>>>> +err:
>>>> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
>>>> +       return 0;
>>>> +}
>>>
>>> Can we share source code with thp_anon since there's a lot of duplication?
>>
>> I'm not a regular mm contributor and I'm most usually around drivers, so
>> I don't know exactly here I could add shared code. Should I add the
>> headers to "internal.h"?
> 
> My comment isn't related to drivers or memory management. It's solely about
> avoiding code duplication. For example, we could create a shared function to
> handle both controls, reducing redundant code :-)

Let me rephrase it.

I completely agree that we should avoid code duplication. I'm asking
where is the best place to add the headers of the shared functions.
"linux/shmem_fs.h" doesn't look appropriate to me, so I believe the
remaining options would be "linux/huge_mm.h" or "internal.h".

I would like to know your opinion about those two options.

Best Regards,
- Maíra

> 
>>
>> Best Regards,
>> - Maíra
>>
>>>
>>>> +__setup("thp_shmem=", setup_thp_shmem);
>>>> +
>>>>    #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>
>>>>    #else /* !CONFIG_SHMEM */
>>>> --
>>>> 2.46.2
>>>>
>>>
>>> Thanks
>>> barry
>>

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Barry Song]

On Mon, Oct 28, 2024 at 7:34 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Barry,
>
> On 28/10/24 08:09, Barry Song wrote:
> > On Mon, Oct 28, 2024 at 6:10 PM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> Hi Barry,
> >>
> >> On 27/10/24 18:54, Barry Song wrote:
> >>> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>>>
> >>>> Add the ``thp_shmem=`` kernel command line to allow specifying the
> >>>> default policy of each supported shmem hugepage size. The kernel parameter
> >>>> accepts the following format:
> >>>>
> >>>> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> >>>>
> >>>> For example,
> >>>>
> >>>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
> >>>>
> >>>> By configuring the default policy of several shmem huge pages, the user
> >>>> can take advantage of mTHP before it's been configured through sysfs.
> >>>>
> >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >>>> ---
> >>>>    .../admin-guide/kernel-parameters.txt         |  10 ++
> >>>>    Documentation/admin-guide/mm/transhuge.rst    |  17 +++
> >>>>    mm/shmem.c                                    | 109 +++++++++++++++++-
> >>>>    3 files changed, 135 insertions(+), 1 deletion(-)
> >>>>
> >>>
> >>> Hi Maíra,
> >>>
> >>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >>>> index acabb04d0dd4..595fa096e28b 100644
> >>>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>>> @@ -6700,6 +6700,16 @@
> >>>>                           Force threading of all interrupt handlers except those
> >>>>                           marked explicitly IRQF_NO_THREAD.
> >>>>
> >>>> +       shmem_anon=     [KNL]
> >>>> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
> >>>> +                       Control the default policy of each hugepage size for the
> >>>> +                       internal shmem mount. <policy> is one of policies available
> >>>> +                       for the shmem mount ("always", "inherit", "never", "within_size",
> >>>> +                       and "advise").
> >>>> +                       It can be used multiple times for multiple shmem THP sizes.
> >>>> +                       See Documentation/admin-guide/mm/transhuge.rst for more
> >>>> +                       details.
> >>>
> >>> I'm not sure this is the right name. How about "thp_shmem"?
> >>
> >> Oops, sorry about that.
> >>
> >>>
> >>>> +
> >>>>           topology=       [S390,EARLY]
> >>>>                           Format: {off | on}
> >>>>                           Specify if the kernel should make use of the cpu
> >>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >>>> index 9b5b02c4d1ab..47e7fc30e22d 100644
> >>>> --- a/Documentation/admin-guide/mm/transhuge.rst
> >>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >>>> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
> >>>>    seven valid policies for shmem (``always``, ``within_size``, ``advise``,
> >>>>    ``never``, ``deny``, and ``force``).
> >>>>
> >>>> +In the same manner as ``thp_anon`` controls each supported anonymous THP
> >>>> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
> >>>> +has the same format as ``thp_anon``, but also supports the policy
> >>>> +``within_size``.
> >>>> +
> >>>> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
> >>>> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
> >>>> +sizes not explicitly configured on the command line are implicitly set to
> >>>> +``never``.
> >>>> +
> >>>> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
> >>>> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
> >>>> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
> >>>> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
> >>>> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
> >>>> +default to ``never``.
> >>>> +
> >>>>    Hugepages in tmpfs/shmem
> >>>>    ========================
> >>>>
> >>>> diff --git a/mm/shmem.c b/mm/shmem.c
> >>>> index 24cdeafd8260..0a7a7d04f725 100644
> >>>> --- a/mm/shmem.c
> >>>> +++ b/mm/shmem.c
>
> [...]
>
> >>>>    static int __init setup_transparent_hugepage_shmem(char *str)
> >>>>    {
> >>>>           int huge, ret = 0;
> >>>> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
> >>>>    }
> >>>>    __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
> >>>>
> >>>> +static char str_dup[PAGE_SIZE] __initdata;
> >>>> +static int __init setup_thp_shmem(char *str)
> >>>> +{
> >>>> +       char *token, *range, *policy, *subtoken;
> >>>> +       unsigned long always, inherit, madvise, within_size;
> >>>> +       char *start_size, *end_size;
> >>>> +       int start, end, nr;
> >>>> +       char *p;
> >>>> +
> >>>> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
> >>>> +               goto err;
> >>>> +       strcpy(str_dup, str);
> >>>> +
> >>>> +       always = huge_shmem_orders_always;
> >>>> +       inherit = huge_shmem_orders_inherit;
> >>>> +       madvise = huge_shmem_orders_madvise;
> >>>> +       within_size = huge_shmem_orders_within_size;
> >>>> +       p = str_dup;
> >>>> +       while ((token = strsep(&p, ";")) != NULL) {
> >>>> +               range = strsep(&token, ":");
> >>>> +               policy = token;
> >>>> +
> >>>> +               if (!policy)
> >>>> +                       goto err;
> >>>> +
> >>>> +               while ((subtoken = strsep(&range, ",")) != NULL) {
> >>>> +                       if (strchr(subtoken, '-')) {
> >>>> +                               start_size = strsep(&subtoken, "-");
> >>>> +                               end_size = subtoken;
> >>>> +
> >>>> +                               start = get_order_from_str(start_size);
> >>>> +                               end = get_order_from_str(end_size);
> >>>> +                       } else {
> >>>> +                               start = end = get_order_from_str(subtoken);
> >>>> +                       }
> >>>> +
> >>>> +                       if (start < 0 || end < 0 || start > end)
> >>>> +                               goto err;
> >>>> +
> >>>> +                       nr = end - start + 1;
> >>>> +                       if (!strcmp(policy, "always")) {
> >>>> +                               bitmap_set(&always, start, nr);
> >>>> +                               bitmap_clear(&inherit, start, nr);
> >>>> +                               bitmap_clear(&madvise, start, nr);
> >>>> +                               bitmap_clear(&within_size, start, nr);
> >>>> +                       } else if (!strcmp(policy, "advise")) {
> >>>> +                               bitmap_set(&madvise, start, nr);
> >>>> +                               bitmap_clear(&inherit, start, nr);
> >>>> +                               bitmap_clear(&always, start, nr);
> >>>> +                               bitmap_clear(&within_size, start, nr);
> >>>> +                       } else if (!strcmp(policy, "inherit")) {
> >>>> +                               bitmap_set(&inherit, start, nr);
> >>>> +                               bitmap_clear(&madvise, start, nr);
> >>>> +                               bitmap_clear(&always, start, nr);
> >>>> +                               bitmap_clear(&within_size, start, nr);
> >>>> +                       } else if (!strcmp(policy, "within_size")) {
> >>>> +                               bitmap_set(&within_size, start, nr);
> >>>> +                               bitmap_clear(&inherit, start, nr);
> >>>> +                               bitmap_clear(&madvise, start, nr);
> >>>> +                               bitmap_clear(&always, start, nr);
> >>>> +                       } else if (!strcmp(policy, "never")) {
> >>>> +                               bitmap_clear(&inherit, start, nr);
> >>>> +                               bitmap_clear(&madvise, start, nr);
> >>>> +                               bitmap_clear(&always, start, nr);
> >>>> +                               bitmap_clear(&within_size, start, nr);
> >>>> +                       } else {
> >>>> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
> >>>> +                               goto err;
> >>>> +                       }
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       huge_shmem_orders_always = always;
> >>>> +       huge_shmem_orders_madvise = madvise;
> >>>> +       huge_shmem_orders_inherit = inherit;
> >>>> +       huge_shmem_orders_within_size = within_size;
> >>>> +       shmem_orders_configured = true;
> >>>> +       return 1;
> >>>> +
> >>>> +err:
> >>>> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
> >>>> +       return 0;
> >>>> +}
> >>>
> >>> Can we share source code with thp_anon since there's a lot of duplication?
> >>
> >> I'm not a regular mm contributor and I'm most usually around drivers, so
> >> I don't know exactly here I could add shared code. Should I add the
> >> headers to "internal.h"?
> >
> > My comment isn't related to drivers or memory management. It's solely about
> > avoiding code duplication. For example, we could create a shared function to
> > handle both controls, reducing redundant code :-)
>
> Let me rephrase it.
>
> I completely agree that we should avoid code duplication. I'm asking
> where is the best place to add the headers of the shared functions.
> "linux/shmem_fs.h" doesn't look appropriate to me, so I believe the
> remaining options would be "linux/huge_mm.h" or "internal.h".

Both locations seem quite odd. I have a feeling that these boot command
elements are purely internal, yet internal.h contains something that is
actually 'external' to mm. The shared code isn't 'external' enough to belong
in internal.h.

I didn't realize that shmem has placed these controls in its own file;
I thought they
were also located in mm/huge_memory.c. Given the current situation, I would
prefer to keep the code as it is and tolerate the code duplication.

Unless we are going to place controls for shmem and other thp controls in
one place, I feel your code is better than having a shared function either in
internal.h or linux/huge_mm.h.

>
> I would like to know your opinion about those two options.
>
> Best Regards,
> - Maíra
>
> >
> >>
> >> Best Regards,
> >> - Maíra
> >>
> >>>
> >>>> +__setup("thp_shmem=", setup_thp_shmem);
> >>>> +
> >>>>    #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> >>>>
> >>>>    #else /* !CONFIG_SHMEM */
> >>>> --
> >>>> 2.46.2
> >>>>
> >>>

Thanks
barry

Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter

[Maíra Canal]

Hi Barry,

On 28/10/24 19:35, Barry Song wrote:
> On Mon, Oct 28, 2024 at 7:34 PM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Hi Barry,
>>
>> On 28/10/24 08:09, Barry Song wrote:
>>> On Mon, Oct 28, 2024 at 6:10 PM Maíra Canal <mcanal@igalia.com> wrote:
>>>>
>>>> Hi Barry,
>>>>
>>>> On 27/10/24 18:54, Barry Song wrote:
>>>>> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal <mcanal@igalia.com> wrote:
>>>>>>
>>>>>> Add the ``thp_shmem=`` kernel command line to allow specifying the
>>>>>> default policy of each supported shmem hugepage size. The kernel parameter
>>>>>> accepts the following format:
>>>>>>
>>>>>> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>>>>>>
>>>>>> For example,
>>>>>>
>>>>>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size
>>>>>>
>>>>>> By configuring the default policy of several shmem huge pages, the user
>>>>>> can take advantage of mTHP before it's been configured through sysfs.
>>>>>>
>>>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>>>> ---
>>>>>>     .../admin-guide/kernel-parameters.txt         |  10 ++
>>>>>>     Documentation/admin-guide/mm/transhuge.rst    |  17 +++
>>>>>>     mm/shmem.c                                    | 109 +++++++++++++++++-
>>>>>>     3 files changed, 135 insertions(+), 1 deletion(-)
>>>>>>
>>>>>
>>>>> Hi Maíra,
>>>>>
>>>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>>>> index acabb04d0dd4..595fa096e28b 100644
>>>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>>>> @@ -6700,6 +6700,16 @@
>>>>>>                            Force threading of all interrupt handlers except those
>>>>>>                            marked explicitly IRQF_NO_THREAD.
>>>>>>
>>>>>> +       shmem_anon=     [KNL]
>>>>>> +                       Format: <size>[KMG],<size>[KMG]:<policy>;<size>[KMG]-<size>[KMG]:<policy>
>>>>>> +                       Control the default policy of each hugepage size for the
>>>>>> +                       internal shmem mount. <policy> is one of policies available
>>>>>> +                       for the shmem mount ("always", "inherit", "never", "within_size",
>>>>>> +                       and "advise").
>>>>>> +                       It can be used multiple times for multiple shmem THP sizes.
>>>>>> +                       See Documentation/admin-guide/mm/transhuge.rst for more
>>>>>> +                       details.
>>>>>
>>>>> I'm not sure this is the right name. How about "thp_shmem"?
>>>>
>>>> Oops, sorry about that.
>>>>
>>>>>
>>>>>> +
>>>>>>            topology=       [S390,EARLY]
>>>>>>                            Format: {off | on}
>>>>>>                            Specify if the kernel should make use of the cpu
>>>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>>>>> index 9b5b02c4d1ab..47e7fc30e22d 100644
>>>>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>>>>> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter
>>>>>>     seven valid policies for shmem (``always``, ``within_size``, ``advise``,
>>>>>>     ``never``, ``deny``, and ``force``).
>>>>>>
>>>>>> +In the same manner as ``thp_anon`` controls each supported anonymous THP
>>>>>> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem``
>>>>>> +has the same format as ``thp_anon``, but also supports the policy
>>>>>> +``within_size``.
>>>>>> +
>>>>>> +``thp_shmem=`` may be specified multiple times to configure all THP sizes
>>>>>> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP
>>>>>> +sizes not explicitly configured on the command line are implicitly set to
>>>>>> +``never``.
>>>>>> +
>>>>>> +``transparent_hugepage_shmem`` setting only affects the global toggle. If
>>>>>> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to
>>>>>> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the
>>>>>> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for
>>>>>> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will
>>>>>> +default to ``never``.
>>>>>> +
>>>>>>     Hugepages in tmpfs/shmem
>>>>>>     ========================
>>>>>>
>>>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>>>> index 24cdeafd8260..0a7a7d04f725 100644
>>>>>> --- a/mm/shmem.c
>>>>>> +++ b/mm/shmem.c
>>
>> [...]
>>
>>>>>>     static int __init setup_transparent_hugepage_shmem(char *str)
>>>>>>     {
>>>>>>            int huge, ret = 0;
>>>>>> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str)
>>>>>>     }
>>>>>>     __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
>>>>>>
>>>>>> +static char str_dup[PAGE_SIZE] __initdata;
>>>>>> +static int __init setup_thp_shmem(char *str)
>>>>>> +{
>>>>>> +       char *token, *range, *policy, *subtoken;
>>>>>> +       unsigned long always, inherit, madvise, within_size;
>>>>>> +       char *start_size, *end_size;
>>>>>> +       int start, end, nr;
>>>>>> +       char *p;
>>>>>> +
>>>>>> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
>>>>>> +               goto err;
>>>>>> +       strcpy(str_dup, str);
>>>>>> +
>>>>>> +       always = huge_shmem_orders_always;
>>>>>> +       inherit = huge_shmem_orders_inherit;
>>>>>> +       madvise = huge_shmem_orders_madvise;
>>>>>> +       within_size = huge_shmem_orders_within_size;
>>>>>> +       p = str_dup;
>>>>>> +       while ((token = strsep(&p, ";")) != NULL) {
>>>>>> +               range = strsep(&token, ":");
>>>>>> +               policy = token;
>>>>>> +
>>>>>> +               if (!policy)
>>>>>> +                       goto err;
>>>>>> +
>>>>>> +               while ((subtoken = strsep(&range, ",")) != NULL) {
>>>>>> +                       if (strchr(subtoken, '-')) {
>>>>>> +                               start_size = strsep(&subtoken, "-");
>>>>>> +                               end_size = subtoken;
>>>>>> +
>>>>>> +                               start = get_order_from_str(start_size);
>>>>>> +                               end = get_order_from_str(end_size);
>>>>>> +                       } else {
>>>>>> +                               start = end = get_order_from_str(subtoken);
>>>>>> +                       }
>>>>>> +
>>>>>> +                       if (start < 0 || end < 0 || start > end)
>>>>>> +                               goto err;
>>>>>> +
>>>>>> +                       nr = end - start + 1;
>>>>>> +                       if (!strcmp(policy, "always")) {
>>>>>> +                               bitmap_set(&always, start, nr);
>>>>>> +                               bitmap_clear(&inherit, start, nr);
>>>>>> +                               bitmap_clear(&madvise, start, nr);
>>>>>> +                               bitmap_clear(&within_size, start, nr);
>>>>>> +                       } else if (!strcmp(policy, "advise")) {
>>>>>> +                               bitmap_set(&madvise, start, nr);
>>>>>> +                               bitmap_clear(&inherit, start, nr);
>>>>>> +                               bitmap_clear(&always, start, nr);
>>>>>> +                               bitmap_clear(&within_size, start, nr);
>>>>>> +                       } else if (!strcmp(policy, "inherit")) {
>>>>>> +                               bitmap_set(&inherit, start, nr);
>>>>>> +                               bitmap_clear(&madvise, start, nr);
>>>>>> +                               bitmap_clear(&always, start, nr);
>>>>>> +                               bitmap_clear(&within_size, start, nr);
>>>>>> +                       } else if (!strcmp(policy, "within_size")) {
>>>>>> +                               bitmap_set(&within_size, start, nr);
>>>>>> +                               bitmap_clear(&inherit, start, nr);
>>>>>> +                               bitmap_clear(&madvise, start, nr);
>>>>>> +                               bitmap_clear(&always, start, nr);
>>>>>> +                       } else if (!strcmp(policy, "never")) {
>>>>>> +                               bitmap_clear(&inherit, start, nr);
>>>>>> +                               bitmap_clear(&madvise, start, nr);
>>>>>> +                               bitmap_clear(&always, start, nr);
>>>>>> +                               bitmap_clear(&within_size, start, nr);
>>>>>> +                       } else {
>>>>>> +                               pr_err("invalid policy %s in thp_shmem boot parameter\n", policy);
>>>>>> +                               goto err;
>>>>>> +                       }
>>>>>> +               }
>>>>>> +       }
>>>>>> +
>>>>>> +       huge_shmem_orders_always = always;
>>>>>> +       huge_shmem_orders_madvise = madvise;
>>>>>> +       huge_shmem_orders_inherit = inherit;
>>>>>> +       huge_shmem_orders_within_size = within_size;
>>>>>> +       shmem_orders_configured = true;
>>>>>> +       return 1;
>>>>>> +
>>>>>> +err:
>>>>>> +       pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str);
>>>>>> +       return 0;
>>>>>> +}
>>>>>
>>>>> Can we share source code with thp_anon since there's a lot of duplication?
>>>>
>>>> I'm not a regular mm contributor and I'm most usually around drivers, so
>>>> I don't know exactly here I could add shared code. Should I add the
>>>> headers to "internal.h"?
>>>
>>> My comment isn't related to drivers or memory management. It's solely about
>>> avoiding code duplication. For example, we could create a shared function to
>>> handle both controls, reducing redundant code :-)
>>
>> Let me rephrase it.
>>
>> I completely agree that we should avoid code duplication. I'm asking
>> where is the best place to add the headers of the shared functions.
>> "linux/shmem_fs.h" doesn't look appropriate to me, so I believe the
>> remaining options would be "linux/huge_mm.h" or "internal.h".
> 
> Both locations seem quite odd. I have a feeling that these boot command
> elements are purely internal, yet internal.h contains something that is
> actually 'external' to mm. The shared code isn't 'external' enough to belong
> in internal.h.
> 
> I didn't realize that shmem has placed these controls in its own file;
> I thought they
> were also located in mm/huge_memory.c. Given the current situation, I would
> prefer to keep the code as it is and tolerate the code duplication.
> 
> Unless we are going to place controls for shmem and other thp controls in
> one place, I feel your code is better than having a shared function either in
> internal.h or linux/huge_mm.h.

Sorry, I only catch your e-mail after sending v2. If possible, please,
take a look on v2 [1] and let me know if you still prefer to duplicate
the code.

[1] 
https://lore.kernel.org/linux-mm/20241029002324.1062723-1-mcanal@igalia.com/T/

Best Regards,
- Maíra

> 
>>
>> I would like to know your opinion about those two options.
>>
>> Best Regards,
>> - Maíra
>>
>>>
>>>>
>>>> Best Regards,
>>>> - Maíra
>>>>
>>>>>
>>>>>> +__setup("thp_shmem=", setup_thp_shmem);
>>>>>> +
>>>>>>     #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>
>>>>>>     #else /* !CONFIG_SHMEM */
>>>>>> --
>>>>>> 2.46.2
>>>>>>
>>>>>
> 
> Thanks
> barry