2022-09-08 08:36:45

by Liu Shixin

Subject: [PATCH] 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 ++++----
mm/swap_cgroup.c | 2 +-
3 files changed, 9 insertions(+), 8 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);
diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
index 5a9442979a18..107dd0e94b01 100644
--- a/mm/swap_cgroup.c
+++ b/mm/swap_cgroup.c
@@ -194,7 +194,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
return 0;
nomem:
pr_info("couldn't allocate enough memory for swap_cgroup\n");
- pr_info("swap_cgroup can be disabled by swapaccount=0 boot option\n");
+ pr_info("swap_cgroup can be disabled by swapaccount=[oO][Ff]/N/n/0 boot option\n");
return -ENOMEM;
}

--
2.25.1

2022-09-08 22:34:46

by Andrew Morton

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

On Thu, 8 Sep 2022 16:34:52 +0800 Liu Shixin <[email protected]> wrote:

> --- a/mm/swap_cgroup.c
> +++ b/mm/swap_cgroup.c
> @@ -194,7 +194,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
> return 0;
> nomem:
> pr_info("couldn't allocate enough memory for swap_cgroup\n");
> - pr_info("swap_cgroup can be disabled by swapaccount=0 boot option\n");
> + pr_info("swap_cgroup can be disabled by swapaccount=[oO][Ff]/N/n/0 boot option\n");

I'm not sure this really needed changing. "=0" was OK and the message
now looks rather silly. But whatever.

2022-09-09 08:31:27

by Liu Shixin

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

On 2022/9/9 15:37, Michal Hocko wrote:
> On Thu 08-09-22 15:02:48, Andrew Morton wrote:
>> On Thu, 8 Sep 2022 16:34:52 +0800 Liu Shixin <[email protected]> wrote:
>>
>>> --- a/mm/swap_cgroup.c
>>> +++ b/mm/swap_cgroup.c
>>> @@ -194,7 +194,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
>>> return 0;
>>> nomem:
>>> pr_info("couldn't allocate enough memory for swap_cgroup\n");
>>> - pr_info("swap_cgroup can be disabled by swapaccount=0 boot option\n");
>>> + pr_info("swap_cgroup can be disabled by swapaccount=[oO][Ff]/N/n/0 boot option\n");
>> I'm not sure this really needed changing. "=0" was OK and the message
>> now looks rather silly.
> Agreed. While swapaccount=0 is clear the extended form, even if correct,
> might just confuse some admins.
Thanks, the previous info is more appropriate, I will update the patch.

2022-09-09 08:37:25

by Michal Hocko

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

On Thu 08-09-22 15:02:48, Andrew Morton wrote:
> On Thu, 8 Sep 2022 16:34:52 +0800 Liu Shixin <[email protected]> wrote:
>
> > --- a/mm/swap_cgroup.c
> > +++ b/mm/swap_cgroup.c
> > @@ -194,7 +194,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
> > return 0;
> > nomem:
> > pr_info("couldn't allocate enough memory for swap_cgroup\n");
> > - pr_info("swap_cgroup can be disabled by swapaccount=0 boot option\n");
> > + pr_info("swap_cgroup can be disabled by swapaccount=[oO][Ff]/N/n/0 boot option\n");
>
> I'm not sure this really needed changing. "=0" was OK and the message
> now looks rather silly.

Agreed. While swapaccount=0 is clear the extended form, even if correct,
might just confuse some admins.

--
Michal Hocko
SUSE Labs