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

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

[Liu Shixin]

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 <liushixin2@huawei.com>
---
 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

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

[Andrew Morton]

On Fri, 9 Sep 2022 16:46:47 +0800 Liu Shixin <liushixin2@huawei.com> 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.

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

[Liu Shixin]

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 <liushixin2@huawei.com>
---
 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

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

[Michal Hocko]

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 <liushixin2@huawei.com>

Acked-by: Michal Hocko <mhocko@suse.com>

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

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

[Muchun Song]

> On Sep 9, 2022, at 16:46, Liu Shixin <liushixin2@huawei.com> 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 <liushixin2@huawei.com>

With Andrew and Michal’s suggestion.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.