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 hugepages, 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..b48d744d99b0 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.
 
+	thp_shmem=	[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 dfcc88ec6e34..c2299fa0b345 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
@@ -5027,7 +5028,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;
 
@@ -5180,6 +5182,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;
@@ -5195,6 +5217,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;
+	strscpy(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

On 30.10.24 13:58, Maíra Canal 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 hugepages, 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..b48d744d99b0 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.
>   
> +	thp_shmem=	[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 dfcc88ec6e34..c2299fa0b345 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
> @@ -5027,7 +5028,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;
>   
> @@ -5180,6 +5182,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;
> +}

Hm, mostly copy and paste. You could reuse existing get_order_from_str() 
simply by passing in the supported orders and moving error reporting to 
the caller.

static inline int get_order_from_str(const char *size_str,
		int valid_orders)
{
	...
	if (!is_power_of_2(size))
		return -EINVAL;
	order = get_order(size);
	if (BIT(order) & ~valid_orders)
		return -EINVAL;
	return order;
}

> +
>   static int __init setup_transparent_hugepage_shmem(char *str)
>   {
>   	int huge;
> @@ -5195,6 +5217,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;
> +	strscpy(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;
> +			}
> +		}
> +	}


Similarly, copy-paste. But not that easy to abstract :) So maybe we'll 
have to keep that as is for now.


-- 
Cheers,

David / dhildenb

Hi David,

On 31/10/24 09:37, David Hildenbrand wrote:
> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>> index dfcc88ec6e34..c2299fa0b345 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
>> @@ -5027,7 +5028,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;
>> @@ -5180,6 +5182,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;
>> +}
> 
> Hm, mostly copy and paste. You could reuse existing get_order_from_str() 
> simply by passing in the supported orders and moving error reporting to 
> the caller.
> 

Can I use functions from mm/huge_memory.c here?

> static inline int get_order_from_str(const char *size_str,
>          int valid_orders)
> {
>      ...
>      if (!is_power_of_2(size))
>          return -EINVAL;
>      order = get_order(size);
>      if (BIT(order) & ~valid_orders)
>          return -EINVAL;
>      return order;
> }
> 
>> +
>>   static int __init setup_transparent_hugepage_shmem(char *str)
>>   {
>>       int huge;
>> @@ -5195,6 +5217,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;
>> +    strscpy(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;
>> +            }
>> +        }
>> +    }
> 
> 
> Similarly, copy-paste. But not that easy to abstract :) So maybe we'll 
> have to keep that as is for now.

On v2 [1], I abstracted to reduce copy and paste, but me and Barry
agreed that adding this sort of header to linux/huge_mm.h was weird.

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

Best Regards,
- Maíra

> 
> 

On 31.10.24 13:51, Maíra Canal wrote:
> Hi David,
> 
> On 31/10/24 09:37, David Hildenbrand wrote:
>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>>> index dfcc88ec6e34..c2299fa0b345 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
>>> @@ -5027,7 +5028,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;
>>> @@ -5180,6 +5182,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;
>>> +}
>>
>> Hm, mostly copy and paste. You could reuse existing get_order_from_str()
>> simply by passing in the supported orders and moving error reporting to
>> the caller.
>>
> 
> Can I use functions from mm/huge_memory.c here?

Yes, that's the idea.

-- 
Cheers,

David / dhildenb

Hi David,

On 31/10/24 09:57, David Hildenbrand wrote:
> On 31.10.24 13:51, Maíra Canal wrote:
>> Hi David,
>>
>> On 31/10/24 09:37, David Hildenbrand wrote:
>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>>>> index dfcc88ec6e34..c2299fa0b345 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
>>>> @@ -5027,7 +5028,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;
>>>> @@ -5180,6 +5182,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;
>>>> +}
>>>
>>> Hm, mostly copy and paste. You could reuse existing get_order_from_str()
>>> simply by passing in the supported orders and moving error reporting to
>>> the caller.
>>>
>>
>> Can I use functions from mm/huge_memory.c here?
> 
> Yes, that's the idea.
> 

Unfortunately, it isn't possible without adding the function to a
header.

I deleted `get_order_from_str()` from mm/shmem.c just to test it:

mm/shmem.c:5230:13: error: call to undeclared function 
'get_order_from_str'; ISO C99 and later do not support implicit function 
declarations [-Wimplicit-function-declaration]
  5230 |                                 start = 
get_order_from_str(start_size);
       |                                         ^
mm/shmem.c:5233:19: error: call to undeclared function 
'get_order_from_str'; ISO C99 and later do not support implicit function 
declarations [-Wimplicit-function-declaration]
  5233 |                                 start = end = 
get_order_from_str(subtoken);
       |                                               ^
2 errors generated.
make[3]: *** [scripts/Makefile.build:229: mm/shmem.o] Error 1
make[2]: *** [scripts/Makefile.build:478: mm] Error 2
make[2]: *** Waiting for unfinished jobs....

Please, check the discussion I had with Barry about the pros and cons of
copy and paste and code refactor [1][2]. We ended up deciding to
duplicate the code.

[1] 
https://lore.kernel.org/linux-mm/CAGsJ_4wJ2xoNRLMNkmLL3x1t2YiqQJ1=saXa+HNxRUbSNivRCw@mail.gmail.com/
[2] 
https://lore.kernel.org/linux-mm/CAGsJ_4yp89one_hoB_87poU2wrpJh_0NVicRKW538eE6yo1kZw@mail.gmail.com/

Best Regards,
- Maíra

On 31.10.24 14:24, Maíra Canal wrote:
> Hi David,
> 
> On 31/10/24 09:57, David Hildenbrand wrote:
>> On 31.10.24 13:51, Maíra Canal wrote:
>>> Hi David,
>>>
>>> On 31/10/24 09:37, David Hildenbrand wrote:
>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>>>>> index dfcc88ec6e34..c2299fa0b345 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
>>>>> @@ -5027,7 +5028,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;
>>>>> @@ -5180,6 +5182,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;
>>>>> +}
>>>>
>>>> Hm, mostly copy and paste. You could reuse existing get_order_from_str()
>>>> simply by passing in the supported orders and moving error reporting to
>>>> the caller.
>>>>
>>>
>>> Can I use functions from mm/huge_memory.c here?
>>
>> Yes, that's the idea.
>>
> 
> Unfortunately, it isn't possible without adding the function to a
> header.

Well ... sure, what's the problem with that?

-- 
Cheers,

David / dhildenb

On 31/10/24 10:33, David Hildenbrand wrote:
> On 31.10.24 14:24, Maíra Canal wrote:
>> Hi David,
>>
>> On 31/10/24 09:57, David Hildenbrand wrote:
>>> On 31.10.24 13:51, Maíra Canal wrote:
>>>> Hi David,
>>>>
>>>> On 31/10/24 09:37, David Hildenbrand wrote:
>>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>>>>>> index dfcc88ec6e34..c2299fa0b345 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
>>>>>> @@ -5027,7 +5028,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;
>>>>>> @@ -5180,6 +5182,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;
>>>>>> +}
>>>>>
>>>>> Hm, mostly copy and paste. You could reuse existing 
>>>>> get_order_from_str()
>>>>> simply by passing in the supported orders and moving error 
>>>>> reporting to
>>>>> the caller.
>>>>>
>>>>
>>>> Can I use functions from mm/huge_memory.c here?
>>>
>>> Yes, that's the idea.
>>>
>>
>> Unfortunately, it isn't possible without adding the function to a
>> header.
> 
> Well ... sure, what's the problem with that?

David & Barry, how do you feel about adding `get_order_from_str()` to
lib/cmdline.c?

Best Regards,
- Maíra

> 

On Fri, Nov 1, 2024 at 3:20 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> On 31/10/24 10:33, David Hildenbrand wrote:
> > On 31.10.24 14:24, Maíra Canal wrote:
> >> Hi David,
> >>
> >> On 31/10/24 09:57, David Hildenbrand wrote:
> >>> On 31.10.24 13:51, Maíra Canal wrote:
> >>>> Hi David,
> >>>>
> >>>> On 31/10/24 09:37, David Hildenbrand wrote:
> >>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
> >>>>>> index dfcc88ec6e34..c2299fa0b345 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
> >>>>>> @@ -5027,7 +5028,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;
> >>>>>> @@ -5180,6 +5182,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;
> >>>>>> +}
> >>>>>
> >>>>> Hm, mostly copy and paste. You could reuse existing
> >>>>> get_order_from_str()
> >>>>> simply by passing in the supported orders and moving error
> >>>>> reporting to
> >>>>> the caller.
> >>>>>
> >>>>
> >>>> Can I use functions from mm/huge_memory.c here?
> >>>
> >>> Yes, that's the idea.
> >>>
> >>
> >> Unfortunately, it isn't possible without adding the function to a
> >> header.
> >
> > Well ... sure, what's the problem with that?
>
> David & Barry, how do you feel about adding `get_order_from_str()` to
> lib/cmdline.c?

I'd vote to leave it as is. If, at some point, the controls for shared memory
and anonymous memory are moved to a file, that would be the right time
to call the same get_order_from_str() for both.

This is too trivial to warrant being an exposed API in huge_memory.h
or cmdline.

>
> Best Regards,
> - Maíra
>

Thanks
Barry

On 31.10.24 22:12, Barry Song wrote:
> On Fri, Nov 1, 2024 at 3:20 AM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> On 31/10/24 10:33, David Hildenbrand wrote:
>>> On 31.10.24 14:24, Maíra Canal wrote:
>>>> Hi David,
>>>>
>>>> On 31/10/24 09:57, David Hildenbrand wrote:
>>>>> On 31.10.24 13:51, Maíra Canal wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 31/10/24 09:37, David Hildenbrand wrote:
>>>>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
>>>>>>>> index dfcc88ec6e34..c2299fa0b345 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
>>>>>>>> @@ -5027,7 +5028,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;
>>>>>>>> @@ -5180,6 +5182,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;
>>>>>>>> +}
>>>>>>>
>>>>>>> Hm, mostly copy and paste. You could reuse existing
>>>>>>> get_order_from_str()
>>>>>>> simply by passing in the supported orders and moving error
>>>>>>> reporting to
>>>>>>> the caller.
>>>>>>>
>>>>>>
>>>>>> Can I use functions from mm/huge_memory.c here?
>>>>>
>>>>> Yes, that's the idea.
>>>>>
>>>>
>>>> Unfortunately, it isn't possible without adding the function to a
>>>> header.
>>>
>>> Well ... sure, what's the problem with that?
>>
>> David & Barry, how do you feel about adding `get_order_from_str()` to
>> lib/cmdline.c?
> 
> I'd vote to leave it as is. If, at some point, the controls for shared memory
> and anonymous memory are moved to a file, that would be the right time
> to call the same get_order_from_str() for both.
 > > This is too trivial to warrant being an exposed API in huge_memory.h
> or cmdline.

I ... don't quite agree. cmdline.c is probably a bit excessive and I 
wouldn't suggest that at this point.

This seems like a reasonable helper function to have as inline in 
mm/internal.h.

... unless I am missing something important and the obvious code 
duplication is warranted.

-- 
Cheers,

David / dhildenb

On Fri, Nov 1, 2024 at 10:39 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 31.10.24 22:12, Barry Song wrote:
> > On Fri, Nov 1, 2024 at 3:20 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> On 31/10/24 10:33, David Hildenbrand wrote:
> >>> On 31.10.24 14:24, Maíra Canal wrote:
> >>>> Hi David,
> >>>>
> >>>> On 31/10/24 09:57, David Hildenbrand wrote:
> >>>>> On 31.10.24 13:51, Maíra Canal wrote:
> >>>>>> Hi David,
> >>>>>>
> >>>>>> On 31/10/24 09:37, David Hildenbrand wrote:
> >>>>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
> >>>>>>>> index dfcc88ec6e34..c2299fa0b345 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
> >>>>>>>> @@ -5027,7 +5028,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;
> >>>>>>>> @@ -5180,6 +5182,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;
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> Hm, mostly copy and paste. You could reuse existing
> >>>>>>> get_order_from_str()
> >>>>>>> simply by passing in the supported orders and moving error
> >>>>>>> reporting to
> >>>>>>> the caller.
> >>>>>>>
> >>>>>>
> >>>>>> Can I use functions from mm/huge_memory.c here?
> >>>>>
> >>>>> Yes, that's the idea.
> >>>>>
> >>>>
> >>>> Unfortunately, it isn't possible without adding the function to a
> >>>> header.
> >>>
> >>> Well ... sure, what's the problem with that?
> >>
> >> David & Barry, how do you feel about adding `get_order_from_str()` to
> >> lib/cmdline.c?
> >
> > I'd vote to leave it as is. If, at some point, the controls for shared memory
> > and anonymous memory are moved to a file, that would be the right time
> > to call the same get_order_from_str() for both.
>  > > This is too trivial to warrant being an exposed API in huge_memory.h
> > or cmdline.
>
> I ... don't quite agree. cmdline.c is probably a bit excessive and I
> wouldn't suggest that at this point.
>
> This seems like a reasonable helper function to have as inline in
> mm/internal.h.

For get_order_from_str() specifically, I agree that it's fine to keep
it in internal.h,
as it could be considered a general need. However, anything larger than that
in parse bootcmd seems too trivial and not general enough to qualify as an API.

>
> ... unless I am missing something important and the obvious code
> duplication is warranted.
>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
barry

On Fri, Nov 1, 2024 at 3:20 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> On 31/10/24 10:33, David Hildenbrand wrote:
> > On 31.10.24 14:24, Maíra Canal wrote:
> >> Hi David,
> >>
> >> On 31/10/24 09:57, David Hildenbrand wrote:
> >>> On 31.10.24 13:51, Maíra Canal wrote:
> >>>> Hi David,
> >>>>
> >>>> On 31/10/24 09:37, David Hildenbrand wrote:
> >>>>> On 30.10.24 13:58, Maíra Canal 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 hugepages, 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/mm/shmem.c b/mm/shmem.c
> >>>>>> index dfcc88ec6e34..c2299fa0b345 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
> >>>>>> @@ -5027,7 +5028,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;
> >>>>>> @@ -5180,6 +5182,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;
> >>>>>> +}
> >>>>>
> >>>>> Hm, mostly copy and paste. You could reuse existing
> >>>>> get_order_from_str()
> >>>>> simply by passing in the supported orders and moving error
> >>>>> reporting to
> >>>>> the caller.
> >>>>>
> >>>>
> >>>> Can I use functions from mm/huge_memory.c here?
> >>>
> >>> Yes, that's the idea.
> >>>
> >>
> >> Unfortunately, it isn't possible without adding the function to a
> >> header.
> >
> > Well ... sure, what's the problem with that?
>
> David & Barry, how do you feel about adding `get_order_from_str()` to
> lib/cmdline.c?

I'd vote to leave it as is. If, at some point, the controls for shared memory
and anonymous memory are moved to a file, that would be the right time
to call the same get_order_from_str() for both.

This is too trivial to warrant being an exposed API in huge_memory.h
or cmdline.

>
> Best Regards,
> - Maíra

Thanks
barry