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

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

[Maíra Canal]

This series introduces three patches related to the kernel parameters
controlling mTHP and a fourth patch replacing `strcpy()` for `strscpy()`
in the file `mm/huge_memory.c`.

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.

Let me know your thoughts.

v1 -> v2: https://lore.kernel.org/linux-mm/20241027175743.1056710-1-mcanal@igalia.com/T/

* [1/3] s/fix the format/fix the doc in the commit's subject (Barry Song & David Hildenbrand)
* [1/3] Add Barry's A-b to PATCH 1/3 (Barry Song)
* [1/3] s/64KB/64K (David Hildenbrand)
* [1/3] Add David's A-b to PATCH 1/3 (David Hildenbrand)
* [2/3] Create the function `shmem_valid_huge()` to reduce code-duplication (Baolin Wang)
* [3/4] New PATCH: generalize the function `setup_thp_anon()` and add it to common file
* [4/4] Fix typo in the documentation: s/shmem_anon/thp_shmem (Barry Song)
* [4/4] Reduce code-duplication (Barry Song)

v2 -> v3: https://lore.kernel.org/linux-mm/20241029002324.1062723-1-mcanal@igalia.com/T/

* [2/4] Apply Wang's suggestion (Baolin Wang)
* [3/4] Delete PATCH 3/4 from v2 and implement ``thp_shmem=`` just like in v1 (Barry Song)
* [4/4] New PATCH: "mm: huge_memory: Use strscpy() instead of strcpy()"

Best Regards,
- Maíra

Maíra Canal (4):
  mm: fix docs for 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
  mm: huge_memory: Use strscpy() instead of strcpy()

 .../admin-guide/kernel-parameters.txt         |  19 +-
 Documentation/admin-guide/mm/transhuge.rst    |  25 ++-
 mm/huge_memory.c                              |   4 +-
 mm/shmem.c                                    | 181 +++++++++++++++---
 4 files changed, 201 insertions(+), 28 deletions(-)

-- 
2.46.2

[PATCH v3 1/4] mm: fix docs for the kernel parameter ``thp_anon=``

[Maíra Canal]

If we add ``thp_anon=32,64K: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>
Acked-by: Barry Song <baohua@kernel.org>
Acked-by: David Hildenbrand <david@redhat.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

[PATCH v3 2/4] 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                                    | 72 +++++++++++++------
 3 files changed, 62 insertions(+), 23 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 275251abd596..dfcc88ec6e34 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -582,24 +582,39 @@ static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index,
 	}
 }
 
-#if defined(CONFIG_SYSFS)
 static int shmem_parse_huge(const char *str)
 {
+	int huge;
+
+	if (!str)
+		return -EINVAL;
+
 	if (!strcmp(str, "never"))
-		return SHMEM_HUGE_NEVER;
-	if (!strcmp(str, "always"))
-		return SHMEM_HUGE_ALWAYS;
-	if (!strcmp(str, "within_size"))
-		return SHMEM_HUGE_WITHIN_SIZE;
-	if (!strcmp(str, "advise"))
-		return SHMEM_HUGE_ADVISE;
-	if (!strcmp(str, "deny"))
-		return SHMEM_HUGE_DENY;
-	if (!strcmp(str, "force"))
-		return SHMEM_HUGE_FORCE;
-	return -EINVAL;
+		huge = SHMEM_HUGE_NEVER;
+	else if (!strcmp(str, "always"))
+		huge = SHMEM_HUGE_ALWAYS;
+	else if (!strcmp(str, "within_size"))
+		huge = SHMEM_HUGE_WITHIN_SIZE;
+	else if (!strcmp(str, "advise"))
+		huge = SHMEM_HUGE_ADVISE;
+	else if (!strcmp(str, "deny"))
+		huge = SHMEM_HUGE_DENY;
+	else if (!strcmp(str, "force"))
+		huge = SHMEM_HUGE_FORCE;
+	else
+		return -EINVAL;
+
+	if (!has_transparent_hugepage() &&
+	    huge != SHMEM_HUGE_NEVER && huge != SHMEM_HUGE_DENY)
+		return -EINVAL;
+
+	/* Do not override huge allocation policy with non-PMD sized mTHP */
+	if (huge == SHMEM_HUGE_FORCE &&
+	    huge_shmem_orders_inherit != BIT(HPAGE_PMD_ORDER))
+		return -EINVAL;
+
+	return huge;
 }
-#endif
 
 #if defined(CONFIG_SYSFS) || defined(CONFIG_TMPFS)
 static const char *shmem_format_huge(int huge)
@@ -5066,15 +5081,7 @@ static ssize_t shmem_enabled_store(struct kobject *kobj,
 
 	huge = shmem_parse_huge(tmp);
 	if (huge == -EINVAL)
-		return -EINVAL;
-	if (!has_transparent_hugepage() &&
-			huge != SHMEM_HUGE_NEVER && huge != SHMEM_HUGE_DENY)
-		return -EINVAL;
-
-	/* Do not override huge allocation policy with non-PMD sized mTHP */
-	if (huge == SHMEM_HUGE_FORCE &&
-	    huge_shmem_orders_inherit != BIT(HPAGE_PMD_ORDER))
-		return -EINVAL;
+		return huge;
 
 	shmem_huge = huge;
 	if (shmem_huge > SHMEM_HUGE_DENY)
@@ -5171,6 +5178,25 @@ 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;
+
+	huge = shmem_parse_huge(str);
+	if (huge == -EINVAL) {
+		pr_warn("transparent_hugepage_shmem= cannot parse, ignored\n");
+		return huge;
+	}
+
+	shmem_huge = huge;
+	return 1;
+}
+__setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem);
+
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+
 #else /* !CONFIG_SHMEM */
 
 /*
-- 
2.46.2

[PATCH v3 3/4] 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 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

[PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Maíra Canal]

Replace strcpy() with strscpy() in mm/huge_memory.c

strcpy() has been deprecated because it is generally unsafe, so help to
eliminate it from the kernel source.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 mm/huge_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index f92068864469..8f41a694433c 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
 
 	if (!str || strlen(str) + 1 > PAGE_SIZE)
 		goto err;
-	strcpy(str_dup, str);
+	strscpy(str_dup, str);
 
 	always = huge_anon_orders_always;
 	madvise = huge_anon_orders_madvise;
@@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
 
 		tok = strsep(&buf, ",");
 		if (tok) {
-			strcpy(file_path, tok);
+			strscpy(file_path, tok);
 		} else {
 			ret = -EINVAL;
 			goto out;
-- 
2.46.2

Re: [PATCH v3 0/4] mm: add more kernel parameters to control mTHP

[Andrew Morton]

On Wed, 30 Oct 2024 09:58:54 -0300 Maíra Canal <mcanal@igalia.com> wrote:

> 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 changelogs for patches 2 and 3 both say

: By configuring ..., applications that use shmem, such as the DRM GEM objects,
: can take advantage of mTHP before it's been configured through sysfs.

There isn't a lot of info here - please explain this timing issue in
more detail.

Because the question which leaps to mind is: shouldn't the
"applications that use shmem" be changed to "configure mTHP through
sysfs" *before* "using shmem"?  Seems pretty basic.


Also, please consider my question to be a critique of the changelogs. 
If the changelogs were complete, I wouldn't need to ask any questions!

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Barry Song]

On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> Replace strcpy() with strscpy() in mm/huge_memory.c
>
> strcpy() has been deprecated because it is generally unsafe, so help to
> eliminate it from the kernel source.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  mm/huge_memory.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index f92068864469..8f41a694433c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
>
>         if (!str || strlen(str) + 1 > PAGE_SIZE)
>                 goto err;
> -       strcpy(str_dup, str);
> +       strscpy(str_dup, str);

What is the difference between strcpy and strscpy without a size parameter?

we have already a check and goto err. strcpy() is entirely safe.
         if (!str || strlen(str) + 1 > PAGE_SIZE)
                 goto err;

My understanding is that we don't need this patch.

>
>         always = huge_anon_orders_always;
>         madvise = huge_anon_orders_madvise;
> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
>
>                 tok = strsep(&buf, ",");
>                 if (tok) {
> -                       strcpy(file_path, tok);
> +                       strscpy(file_path, tok);
>                 } else {
>                         ret = -EINVAL;
>                         goto out;
> --
> 2.46.2
>

Thanks
barry

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Lance Yang]

Hi Maíra,

Looks like a lot of kernel code is still using strcpy() ;p

On Wed, Oct 30, 2024 at 9:03 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Replace strcpy() with strscpy() in mm/huge_memory.c
>
> strcpy() has been deprecated because it is generally unsafe, so help to
> eliminate it from the kernel source.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Feel free to add:
Reviewed-by: Lance Yang <ioworker0@gmail.com>

Thanks,
Lance


> ---
>  mm/huge_memory.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index f92068864469..8f41a694433c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
>
>         if (!str || strlen(str) + 1 > PAGE_SIZE)
>                 goto err;
> -       strcpy(str_dup, str);
> +       strscpy(str_dup, str);
>
>         always = huge_anon_orders_always;
>         madvise = huge_anon_orders_madvise;
> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
>
>                 tok = strsep(&buf, ",");
>                 if (tok) {
> -                       strcpy(file_path, tok);
> +                       strscpy(file_path, tok);
>                 } else {
>                         ret = -EINVAL;
>                         goto out;
> --
> 2.46.2
>

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

[Baolin Wang]

On 2024/10/30 20:58, 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>

The changes look good to me. With the commit message update pointed out 
by Andrew, feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Maíra Canal]

Hi Barry,

On 30/10/24 20:07, Barry Song wrote:
> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Replace strcpy() with strscpy() in mm/huge_memory.c
>>
>> strcpy() has been deprecated because it is generally unsafe, so help to
>> eliminate it from the kernel source.
>>
>> Link: https://github.com/KSPP/linux/issues/88
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   mm/huge_memory.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index f92068864469..8f41a694433c 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
>>
>>          if (!str || strlen(str) + 1 > PAGE_SIZE)
>>                  goto err;
>> -       strcpy(str_dup, str);
>> +       strscpy(str_dup, str);
> 
> What is the difference between strcpy and strscpy without a size parameter?
> 
> we have already a check and goto err. strcpy() is entirely safe.
>           if (!str || strlen(str) + 1 > PAGE_SIZE)
>                   goto err;
> 
> My understanding is that we don't need this patch.

strcpy() is a deprecated interface [1]. From the GitHub issue I linked
in the commit description, Kees states: "A lot of kernel code is still
using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to
make its use mostly safe, it would be nice to eliminate the function
from the kernel entirely."

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy

Best Regards,
- Maíra

> 
>>
>>          always = huge_anon_orders_always;
>>          madvise = huge_anon_orders_madvise;
>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
>>
>>                  tok = strsep(&buf, ",");
>>                  if (tok) {
>> -                       strcpy(file_path, tok);
>> +                       strscpy(file_path, tok);
>>                  } else {
>>                          ret = -EINVAL;
>>                          goto out;
>> --
>> 2.46.2
>>
> 
> Thanks
> barry

Re: [PATCH v3 0/4] mm: add more kernel parameters to control mTHP

[Maíra Canal]

Hi Andrew,

On 30/10/24 19:50, Andrew Morton wrote:
> On Wed, 30 Oct 2024 09:58:54 -0300 Maíra Canal <mcanal@igalia.com> wrote:
> 
>> 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 changelogs for patches 2 and 3 both say
> 
> : By configuring ..., applications that use shmem, such as the DRM GEM objects,
> : can take advantage of mTHP before it's been configured through sysfs.
> 
> There isn't a lot of info here - please explain this timing issue in
> more detail.
> 
> Because the question which leaps to mind is: shouldn't the
> "applications that use shmem" be changed to "configure mTHP through
> sysfs" *before* "using shmem"?  Seems pretty basic.

Sorry about that, I'll try to improve the commit messages and add more 
details.

As mentioned in the example I gave ("DRM GEM objects"), my main use is
GEM objects backed by shmem. I'd like to use Huge Pages on the GPU and I
can only do that if I have contiguous memory to back my objects.

I can't think how I can change sysfs from a DRM driver.

Best Regards,
- Maíra

> 
> 
> Also, please consider my question to be a critique of the changelogs.
> If the changelogs were complete, I wouldn't need to ask any questions!

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Barry Song]

On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Barry,
>
> On 30/10/24 20:07, Barry Song wrote:
> > On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> Replace strcpy() with strscpy() in mm/huge_memory.c
> >>
> >> strcpy() has been deprecated because it is generally unsafe, so help to
> >> eliminate it from the kernel source.
> >>
> >> Link: https://github.com/KSPP/linux/issues/88
> >> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >> ---
> >>   mm/huge_memory.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index f92068864469..8f41a694433c 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
> >>
> >>          if (!str || strlen(str) + 1 > PAGE_SIZE)
> >>                  goto err;
> >> -       strcpy(str_dup, str);
> >> +       strscpy(str_dup, str);
> >
> > What is the difference between strcpy and strscpy without a size parameter?
> >
> > we have already a check and goto err. strcpy() is entirely safe.
> >           if (!str || strlen(str) + 1 > PAGE_SIZE)
> >                   goto err;
> >
> > My understanding is that we don't need this patch.
>
> strcpy() is a deprecated interface [1]. From the GitHub issue I linked
> in the commit description, Kees states: "A lot of kernel code is still
> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to
> make its use mostly safe, it would be nice to eliminate the function
> from the kernel entirely."

I don't see any value added here since strscpy() has no size parameter and
we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless
bootcmd longer than PAGE_SIZE.

But I have no objection to this patch if other people like it.

>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
>
> Best Regards,
> - Maíra
>
> >
> >>
> >>          always = huge_anon_orders_always;
> >>          madvise = huge_anon_orders_madvise;
> >> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
> >>
> >>                  tok = strsep(&buf, ",");
> >>                  if (tok) {
> >> -                       strcpy(file_path, tok);
> >> +                       strscpy(file_path, tok);
> >>                  } else {
> >>                          ret = -EINVAL;
> >>                          goto out;
> >> --
> >> 2.46.2
> >>
> >
Thanks
barry

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Maíra Canal]

+cc Kees Cook

Hi Barry,

On 31/10/24 09:01, Barry Song wrote:
> On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Hi Barry,
>>
>> On 30/10/24 20:07, Barry Song wrote:
>>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
>>>>
>>>> Replace strcpy() with strscpy() in mm/huge_memory.c
>>>>
>>>> strcpy() has been deprecated because it is generally unsafe, so help to
>>>> eliminate it from the kernel source.
>>>>
>>>> Link: https://github.com/KSPP/linux/issues/88
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>> ---
>>>>    mm/huge_memory.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index f92068864469..8f41a694433c 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
>>>>
>>>>           if (!str || strlen(str) + 1 > PAGE_SIZE)
>>>>                   goto err;
>>>> -       strcpy(str_dup, str);
>>>> +       strscpy(str_dup, str);
>>>
>>> What is the difference between strcpy and strscpy without a size parameter?
>>>
>>> we have already a check and goto err. strcpy() is entirely safe.
>>>            if (!str || strlen(str) + 1 > PAGE_SIZE)
>>>                    goto err;
>>>
>>> My understanding is that we don't need this patch.
>>
>> strcpy() is a deprecated interface [1]. From the GitHub issue I linked
>> in the commit description, Kees states: "A lot of kernel code is still
>> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to
>> make its use mostly safe, it would be nice to eliminate the function
>> from the kernel entirely."
> 
> I don't see any value added here since strscpy() has no size parameter and

As `str_dup` is a sized buffer, we don't need to specify the size
parameter.

> we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless
> bootcmd longer than PAGE_SIZE.

 From my point of view, this is an extra layer of safety. A developer
could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This
could pass by in a review and we would have a check that allows strings
bigger than the destination buffer in systems using 16KB pages.

But see that `split_huge_pages_write` uses `strcpy` without any checks.

I can remove the internal check from `setup_thp_anon` if you feel it
would be more suitable.

Best Regards,
- Maíra

 > > But I have no objection to this patch if other people like it.
> 
>>
>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
>>
>> Best Regards,
>> - Maíra
>>
>>>
>>>>
>>>>           always = huge_anon_orders_always;
>>>>           madvise = huge_anon_orders_madvise;
>>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
>>>>
>>>>                   tok = strsep(&buf, ",");
>>>>                   if (tok) {
>>>> -                       strcpy(file_path, tok);
>>>> +                       strscpy(file_path, tok);
>>>>                   } else {
>>>>                           ret = -EINVAL;
>>>>                           goto out;
>>>> --
>>>> 2.46.2
>>>>
>>>
> Thanks
> barry

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

[David Hildenbrand]

On 30.10.24 13:58, 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                                    | 72 +++++++++++++------
>   3 files changed, 62 insertions(+), 23 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.
> +

LGTM, and it's consistent with the parameter for anon.

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

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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

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

[Maíra Canal]

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

> 
> 

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

[David Hildenbrand]

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

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

[Maíra Canal]

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

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

[David Hildenbrand]

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

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

[Maíra Canal]

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

> 

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Barry Song]

On Fri, Nov 1, 2024 at 1:12 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> +cc Kees Cook
>
> Hi Barry,
>
> On 31/10/24 09:01, Barry Song wrote:
> > On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote:
> >>
> >> Hi Barry,
> >>
> >> On 30/10/24 20:07, Barry Song wrote:
> >>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
> >>>>
> >>>> Replace strcpy() with strscpy() in mm/huge_memory.c
> >>>>
> >>>> strcpy() has been deprecated because it is generally unsafe, so help to
> >>>> eliminate it from the kernel source.
> >>>>
> >>>> Link: https://github.com/KSPP/linux/issues/88
> >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >>>> ---
> >>>>    mm/huge_memory.c | 4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index f92068864469..8f41a694433c 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
> >>>>
> >>>>           if (!str || strlen(str) + 1 > PAGE_SIZE)
> >>>>                   goto err;
> >>>> -       strcpy(str_dup, str);
> >>>> +       strscpy(str_dup, str);
> >>>
> >>> What is the difference between strcpy and strscpy without a size parameter?
> >>>
> >>> we have already a check and goto err. strcpy() is entirely safe.
> >>>            if (!str || strlen(str) + 1 > PAGE_SIZE)
> >>>                    goto err;
> >>>
> >>> My understanding is that we don't need this patch.
> >>
> >> strcpy() is a deprecated interface [1]. From the GitHub issue I linked
> >> in the commit description, Kees states: "A lot of kernel code is still
> >> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to
> >> make its use mostly safe, it would be nice to eliminate the function
> >> from the kernel entirely."
> >
> > I don't see any value added here since strscpy() has no size parameter and
>
> As `str_dup` is a sized buffer, we don't need to specify the size
> parameter.
>
> > we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless
> > bootcmd longer than PAGE_SIZE.
>
>  From my point of view, this is an extra layer of safety. A developer
> could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This
> could pass by in a review and we would have a check that allows strings
> bigger than the destination buffer in systems using 16KB pages.
>
> But see that `split_huge_pages_write` uses `strcpy` without any checks.
>
> I can remove the internal check from `setup_thp_anon` if you feel it
> would be more suitable.

My point is that we shouldn't remove the strlen(str) + 1 > PAGE_SIZE
check, as it could lead to situations like this:

For thp_anon=AB, where len(A) = PAGE_SIZE and B is illegal, we would
mistakenly interpret the string as valid by ignoring B after trimming it.

Therefore, if you remove the check, you could instead do:

len = strscpy(str_dup, src);
if (len >= sizeof(str_dup))
      err;

I have no objection to replacing strcpy with strscpy, so feel free to go
ahead :-)

>
> Best Regards,
> - Maíra
>
>  > > But I have no objection to this patch if other people like it.
> >
> >>
> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
> >>
> >> Best Regards,
> >> - Maíra
> >>
> >>>
> >>>>
> >>>>           always = huge_anon_orders_always;
> >>>>           madvise = huge_anon_orders_madvise;
> >>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
> >>>>
> >>>>                   tok = strsep(&buf, ",");
> >>>>                   if (tok) {
> >>>> -                       strcpy(file_path, tok);
> >>>> +                       strscpy(file_path, tok);
> >>>>                   } else {
> >>>>                           ret = -EINVAL;
> >>>>                           goto out;
> >>>> --
> >>>> 2.46.2
> >>>>
> >>>
Thanks
barry

Re: [PATCH v3 4/4] mm: huge_memory: Use strscpy() instead of strcpy()

[Barry Song]

On Fri, Nov 1, 2024 at 9:27 AM Barry Song <21cnbao@gmail.com> wrote:
>
> On Fri, Nov 1, 2024 at 1:12 AM Maíra Canal <mcanal@igalia.com> wrote:
> >
> > +cc Kees Cook
> >
> > Hi Barry,
> >
> > On 31/10/24 09:01, Barry Song wrote:
> > > On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote:
> > >>
> > >> Hi Barry,
> > >>
> > >> On 30/10/24 20:07, Barry Song wrote:
> > >>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote:
> > >>>>
> > >>>> Replace strcpy() with strscpy() in mm/huge_memory.c
> > >>>>
> > >>>> strcpy() has been deprecated because it is generally unsafe, so help to
> > >>>> eliminate it from the kernel source.
> > >>>>
> > >>>> Link: https://github.com/KSPP/linux/issues/88
> > >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > >>>> ---
> > >>>>    mm/huge_memory.c | 4 ++--
> > >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > >>>> index f92068864469..8f41a694433c 100644
> > >>>> --- a/mm/huge_memory.c
> > >>>> +++ b/mm/huge_memory.c
> > >>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str)
> > >>>>
> > >>>>           if (!str || strlen(str) + 1 > PAGE_SIZE)
> > >>>>                   goto err;
> > >>>> -       strcpy(str_dup, str);
> > >>>> +       strscpy(str_dup, str);
> > >>>
> > >>> What is the difference between strcpy and strscpy without a size parameter?
> > >>>
> > >>> we have already a check and goto err. strcpy() is entirely safe.
> > >>>            if (!str || strlen(str) + 1 > PAGE_SIZE)
> > >>>                    goto err;
> > >>>
> > >>> My understanding is that we don't need this patch.
> > >>
> > >> strcpy() is a deprecated interface [1]. From the GitHub issue I linked
> > >> in the commit description, Kees states: "A lot of kernel code is still
> > >> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to
> > >> make its use mostly safe, it would be nice to eliminate the function
> > >> from the kernel entirely."
> > >
> > > I don't see any value added here since strscpy() has no size parameter and
> >
> > As `str_dup` is a sized buffer, we don't need to specify the size
> > parameter.
> >
> > > we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless
> > > bootcmd longer than PAGE_SIZE.
> >
> >  From my point of view, this is an extra layer of safety. A developer
> > could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This
> > could pass by in a review and we would have a check that allows strings
> > bigger than the destination buffer in systems using 16KB pages.
> >
> > But see that `split_huge_pages_write` uses `strcpy` without any checks.
> >
> > I can remove the internal check from `setup_thp_anon` if you feel it
> > would be more suitable.
>
> My point is that we shouldn't remove the strlen(str) + 1 > PAGE_SIZE
> check, as it could lead to situations like this:
>
> For thp_anon=AB, where len(A) = PAGE_SIZE and B is illegal, we would
> mistakenly interpret the string as valid by ignoring B after trimming it.
>
> Therefore, if you remove the check, you could instead do:
>
> len = strscpy(str_dup, src);
> if (len >= sizeof(str_dup))
>       err;

Perhaps -E2BIG ( or < 0) i'm not quite sure :-)

>
> I have no objection to replacing strcpy with strscpy, so feel free to go
> ahead :-)
>
> >
> > Best Regards,
> > - Maíra
> >
> >  > > But I have no objection to this patch if other people like it.
> > >
> > >>
> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
> > >>
> > >> Best Regards,
> > >> - Maíra
> > >>
> > >>>
> > >>>>
> > >>>>           always = huge_anon_orders_always;
> > >>>>           madvise = huge_anon_orders_madvise;
> > >>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
> > >>>>
> > >>>>                   tok = strsep(&buf, ",");
> > >>>>                   if (tok) {
> > >>>> -                       strcpy(file_path, tok);
> > >>>> +                       strscpy(file_path, tok);
> > >>>>                   } else {
> > >>>>                           ret = -EINVAL;
> > >>>>                           goto out;
> > >>>> --
> > >>>> 2.46.2
> > >>>>
> > >>>
> Thanks
> barry

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

[Barry Song]

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

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

[Barry Song]

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

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

[David Hildenbrand]

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

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

[Barry Song]

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

Re: [PATCH v3 0/4] mm: add more kernel parameters to control mTHP

[Andrew Morton]

On Thu, 31 Oct 2024 08:04:58 -0300 Maíra Canal <mcanal@igalia.com> wrote:

> > There isn't a lot of info here - please explain this timing issue in
> > more detail.
> > 
> > Because the question which leaps to mind is: shouldn't the
> > "applications that use shmem" be changed to "configure mTHP through
> > sysfs" *before* "using shmem"?  Seems pretty basic.
> 
> Sorry about that, I'll try to improve the commit messages and add more 
> details.
> 
> As mentioned in the example I gave ("DRM GEM objects"), my main use is
> GEM objects backed by shmem. I'd like to use Huge Pages on the GPU and I
> can only do that if I have contiguous memory to back my objects.
> 
> I can't think how I can change sysfs from a DRM driver.

So your term "applications" actually refers to in-kernel drivers?

Re: [PATCH v3 0/4] mm: add more kernel parameters to control mTHP

[Maíra Canal]

Hi Andrew,

On 31/10/24 22:12, Andrew Morton wrote:
> On Thu, 31 Oct 2024 08:04:58 -0300 Maíra Canal <mcanal@igalia.com> wrote:
> 
>>> There isn't a lot of info here - please explain this timing issue in
>>> more detail.
>>>
>>> Because the question which leaps to mind is: shouldn't the
>>> "applications that use shmem" be changed to "configure mTHP through
>>> sysfs" *before* "using shmem"?  Seems pretty basic.
>>
>> Sorry about that, I'll try to improve the commit messages and add more
>> details.
>>
>> As mentioned in the example I gave ("DRM GEM objects"), my main use is
>> GEM objects backed by shmem. I'd like to use Huge Pages on the GPU and I
>> can only do that if I have contiguous memory to back my objects.
>>
>> I can't think how I can change sysfs from a DRM driver.
> 
> So your term "applications" actually refers to in-kernel drivers?

Yes, I'll make it clearer in v4. Sorry for the vague commit message.

Best Regards,
- Maíra