2022-09-09 08:17:17

by Liu Shixin

[permalink] [raw]
Subject: [PATCH v2] mm/memcontrol: use kstrtobool for swapaccount param parsing

Use kstrtobool which is more powerful to handle all kinds of parameters
like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".

Signed-off-by: Liu Shixin <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 7 ++++---
mm/memcontrol.c | 8 ++++----
2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2434c6729f1c..5f8531e39bc9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6037,10 +6037,11 @@
Execution Facility on pSeries.

swapaccount= [KNL]
- Format: [0|1]
+ Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 }
Enable accounting of swap in memory resource
- controller if no parameter or 1 is given or disable
- it if 0 is given (See Documentation/admin-guide/cgroup-v1/memory.rst)
+ controller if no parameter or [oO][Nn]/Y/y/1 is given
+ or disable it if [oO][Ff]/N/n/0 is given
+ (See Documentation/admin-guide/cgroup-v1/memory.rst)

swiotlb= [ARM,IA-64,PPC,MIPS,X86]
Format: { <int> [,<int>] | force | noforce }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0a1a8a846870..5511c0c120d9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7434,10 +7434,10 @@ bool mem_cgroup_swap_full(struct folio *folio)

static int __init setup_swap_account(char *s)
{
- if (!strcmp(s, "1"))
- cgroup_memory_noswap = false;
- else if (!strcmp(s, "0"))
- cgroup_memory_noswap = true;
+ bool res;
+
+ if (!kstrtobool(s, &res))
+ cgroup_memory_noswap = !res;
return 1;
}
__setup("swapaccount=", setup_swap_account);
--
2.25.1


2022-09-09 21:54:43

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v2] mm/memcontrol: use kstrtobool for swapaccount param parsing

On Fri, 9 Sep 2022 16:46:47 +0800 Liu Shixin <[email protected]> wrote:

> Use kstrtobool which is more powerful to handle all kinds of parameters
> like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".
>
> ...
>
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6037,10 +6037,11 @@
> Execution Facility on pSeries.
>
> swapaccount= [KNL]
> - Format: [0|1]
> + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 }
> Enable accounting of swap in memory resource
> - controller if no parameter or 1 is given or disable
> - it if 0 is given (See Documentation/admin-guide/cgroup-v1/memory.rst)
> + controller if no parameter or [oO][Nn]/Y/y/1 is given
> + or disable it if [oO][Ff]/N/n/0 is given
> + (See Documentation/admin-guide/cgroup-v1/memory.rst)
>
> swiotlb= [ARM,IA-64,PPC,MIPS,X86]
> Format: { <int> [,<int>] | force | noforce }

mhocko suggested dropping this change as well.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0a1a8a846870..5511c0c120d9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7434,10 +7434,10 @@ bool mem_cgroup_swap_full(struct folio *folio)
>
> static int __init setup_swap_account(char *s)
> {
> - if (!strcmp(s, "1"))
> - cgroup_memory_noswap = false;
> - else if (!strcmp(s, "0"))
> - cgroup_memory_noswap = true;
> + bool res;
> +
> + if (!kstrtobool(s, &res))
> + cgroup_memory_noswap = !res;
> return 1;
> }
> __setup("swapaccount=", setup_swap_account);

And I agree. See, the risk with this patch is that someone will
develop userspace code which relies upon the new behaviour. Then when
someone tries to use that code on an older kernel, whoops, it doesn't
work. In other words, we're encouraging development of
non-backward-compatible userspace code.

Leaving the documentation as it was (just "0|1") will help to prevent
that situation.

2022-09-13 06:56:27

by Liu Shixin

[permalink] [raw]
Subject: [PATCH v3] mm/memcontrol: use kstrtobool for swapaccount param parsing

Use kstrtobool which is more powerful to handle all kinds of parameters
like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".

Signed-off-by: Liu Shixin <[email protected]>
---
mm/memcontrol.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0a1a8a846870..5511c0c120d9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7434,10 +7434,10 @@ bool mem_cgroup_swap_full(struct folio *folio)

static int __init setup_swap_account(char *s)
{
- if (!strcmp(s, "1"))
- cgroup_memory_noswap = false;
- else if (!strcmp(s, "0"))
- cgroup_memory_noswap = true;
+ bool res;
+
+ if (!kstrtobool(s, &res))
+ cgroup_memory_noswap = !res;
return 1;
}
__setup("swapaccount=", setup_swap_account);
--
2.25.1

2022-09-13 11:14:14

by Muchun Song

[permalink] [raw]
Subject: Re: [PATCH v2] mm/memcontrol: use kstrtobool for swapaccount param parsing



> On Sep 9, 2022, at 16:46, Liu Shixin <[email protected]> wrote:
>
> Use kstrtobool which is more powerful to handle all kinds of parameters
> like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".
>
> Signed-off-by: Liu Shixin <[email protected]>

With Andrew and Michal’s suggestion.

Reviewed-by: Muchun Song <[email protected]>

Thanks.

2022-09-19 15:21:21

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH v3] mm/memcontrol: use kstrtobool for swapaccount param parsing

On Tue 13-09-22 15:13:58, Liu Shixin wrote:
> Use kstrtobool which is more powerful to handle all kinds of parameters
> like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".

the main usefulness is with the code reusability of library functions
rather than ad-hoc stuff.

> Signed-off-by: Liu Shixin <[email protected]>

Acked-by: Michal Hocko <[email protected]>

Thanks!
> ---
> mm/memcontrol.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0a1a8a846870..5511c0c120d9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7434,10 +7434,10 @@ bool mem_cgroup_swap_full(struct folio *folio)
>
> static int __init setup_swap_account(char *s)
> {
> - if (!strcmp(s, "1"))
> - cgroup_memory_noswap = false;
> - else if (!strcmp(s, "0"))
> - cgroup_memory_noswap = true;
> + bool res;
> +
> + if (!kstrtobool(s, &res))
> + cgroup_memory_noswap = !res;
> return 1;
> }
> __setup("swapaccount=", setup_swap_account);
> --
> 2.25.1

--
Michal Hocko
SUSE Labs