[PATCH] memsw: handle swapaccount kernel parameter correctly

[PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

Hi Andrew,
I am sorry but the patch which added swapaccount parameter is not
correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
I didn't get the way how __setup parameters are handled correctly.
The patch bellow fixes that.

I am CCing stable as well because the patch got into .37 kernel.

---
>From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Wed, 26 Jan 2011 14:12:41 +0100
Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly

__setup based kernel command line parameters handled in
obsolete_checksetup provides the parameter value including = (more
precisely everything right after the parameter name) so we have to check
for =0 resp. =1 here. If no value is given then we get an empty string
rather then NULL.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index db76ef7..cea2be48 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
 static int __init enable_swap_account(char *s)
 {
 	/* consider enabled if no parameter or 1 is given */
-	if (!s || !strcmp(s, "1"))
+	if (!(*s) || !strcmp(s, "=1"))
 		really_do_swap_account = 1;
-	else if (!strcmp(s, "0"))
+	else if (!strcmp(s, "=0"))
 		really_do_swap_account = 0;
 	return 1;
 }
@@ -5023,7 +5023,7 @@ __setup("swapaccount", enable_swap_account);
 
 static int __init disable_swap_account(char *s)
 {
-	enable_swap_account("0");
+	enable_swap_account("=0");
 	return 1;
 }
 __setup("noswapaccount", disable_swap_account);
-- 
1.7.2.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

[PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

Hi Andrew,
I am sorry but the patch which added swapaccount parameter is not
correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
I didn't get the way how __setup parameters are handled correctly.
The patch bellow fixes that.

I am CCing stable as well because the patch got into .37 kernel.

---

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Andrew Morton]

On Wed, 26 Jan 2011 16:21:58 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> I am sorry but the patch which added swapaccount parameter is not
> correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> I didn't get the way how __setup parameters are handled correctly.
> The patch bellow fixes that.
> 
> I am CCing stable as well because the patch got into .37 kernel.
> 
> ---
> >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Wed, 26 Jan 2011 14:12:41 +0100
> Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> 
> __setup based kernel command line parameters handled in
> obsolete_checksetup provides the parameter value including = (more
> precisely everything right after the parameter name) so we have to check
> for =0 resp. =1 here. If no value is given then we get an empty string
> rather then NULL.

This doesn't provide a description of the bug which just got fixed.

>From reading the code I think the current behaviour is

"swapaccount": works OK
"noswapaccount": works OK
"swapaccount=0": doesn't do anything
"swapaccount=1": doesn't do anything

but I might be wrong about that.  Please send a changelog update to
clarify all this.

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Andrew Morton]

On Wed, 26 Jan 2011 16:21:58 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> I am sorry but the patch which added swapaccount parameter is not
> correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> I didn't get the way how __setup parameters are handled correctly.
> The patch bellow fixes that.
> 
> I am CCing stable as well because the patch got into .37 kernel.
> 
> ---
> >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Wed, 26 Jan 2011 14:12:41 +0100
> Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> 
> __setup based kernel command line parameters handled in
> obsolete_checksetup provides the parameter value including = (more
> precisely everything right after the parameter name) so we have to check
> for =0 resp. =1 here. If no value is given then we get an empty string
> rather then NULL.

This doesn't provide a description of the bug which just got fixed.

>From reading the code I think the current behaviour is

"swapaccount": works OK
"noswapaccount": works OK
"swapaccount=0": doesn't do anything
"swapaccount=1": doesn't do anything

but I might be wrong about that.  Please send a changelog update to
clarify all this.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

On Wed 26-01-11 14:06:18, Andrew Morton wrote:
> On Wed, 26 Jan 2011 16:21:58 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > I am sorry but the patch which added swapaccount parameter is not
> > correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> > I didn't get the way how __setup parameters are handled correctly.
> > The patch bellow fixes that.
> > 
> > I am CCing stable as well because the patch got into .37 kernel.
> > 
> > ---
> > >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Wed, 26 Jan 2011 14:12:41 +0100
> > Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> > 
> > __setup based kernel command line parameters handled in
> > obsolete_checksetup provides the parameter value including = (more
> > precisely everything right after the parameter name) so we have to check
> > for =0 resp. =1 here. If no value is given then we get an empty string
> > rather then NULL.
> 
> This doesn't provide a description of the bug which just got fixed.
> 
> From reading the code I think the current behaviour is
> 
> "swapaccount": works OK

Not really because the original test was !s || s="1" but as I am writing
in the commit message we are getting an empty string rather than NULL in
no parameter value case..
So noswapaccount is actually the only thing that is working.

> "noswapaccount": works OK
> "swapaccount=0": doesn't do anything
> "swapaccount=1": doesn't do anything
> 
> but I might be wrong about that.  Please send a changelog update to
> clarify all this.

Sorry for not being specific enough. What about somthing like this:
---
>From 317dec3d13ef7f11e8f2699331bc32fcd6a8ea0e Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Wed, 26 Jan 2011 14:12:41 +0100
Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly

__setup based kernel command line parameters handlers which are handled in
obsolete_checksetup are provided with the parameter value including =
(more precisely everything right after the parameter name).

This means that the current implementation of swapaccount[=1|0] doesn't
work at all because if there is a value for the parameter then we are
testing for "0" resp. "1" but we are getting "=0" resp. "=1" and if
there is no parameter value we are getting an empty string rather than
NULL.

The original noswapccount parameter, which doesn't care about the value,
works correctly.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index db76ef7..cea2be48 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
 static int __init enable_swap_account(char *s)
 {
 	/* consider enabled if no parameter or 1 is given */
-	if (!s || !strcmp(s, "1"))
+	if (!(*s) || !strcmp(s, "=1"))
 		really_do_swap_account = 1;
-	else if (!strcmp(s, "0"))
+	else if (!strcmp(s, "=0"))
 		really_do_swap_account = 0;
 	return 1;
 }
@@ -5023,7 +5023,7 @@ __setup("swapaccount", enable_swap_account);
 
 static int __init disable_swap_account(char *s)
 {
-	enable_swap_account("0");
+	enable_swap_account("=0");
 	return 1;
 }
 __setup("noswapaccount", disable_swap_account);
-- 
1.7.2.3


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

On Wed 26-01-11 14:06:18, Andrew Morton wrote:
> On Wed, 26 Jan 2011 16:21:58 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > I am sorry but the patch which added swapaccount parameter is not
> > correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> > I didn't get the way how __setup parameters are handled correctly.
> > The patch bellow fixes that.
> > 
> > I am CCing stable as well because the patch got into .37 kernel.
> > 
> > ---
> > >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Wed, 26 Jan 2011 14:12:41 +0100
> > Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> > 
> > __setup based kernel command line parameters handled in
> > obsolete_checksetup provides the parameter value including = (more
> > precisely everything right after the parameter name) so we have to check
> > for =0 resp. =1 here. If no value is given then we get an empty string
> > rather then NULL.
> 
> This doesn't provide a description of the bug which just got fixed.
> 
> From reading the code I think the current behaviour is
> 
> "swapaccount": works OK

Not really because the original test was !s || s="1" but as I am writing
in the commit message we are getting an empty string rather than NULL in
no parameter value case..
So noswapaccount is actually the only thing that is working.

> "noswapaccount": works OK
> "swapaccount=0": doesn't do anything
> "swapaccount=1": doesn't do anything
> 
> but I might be wrong about that.  Please send a changelog update to
> clarify all this.

Sorry for not being specific enough. What about somthing like this:
---

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 09:23:20 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Wed 26-01-11 14:06:18, Andrew Morton wrote:
> > On Wed, 26 Jan 2011 16:21:58 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> > 
> > > I am sorry but the patch which added swapaccount parameter is not
> > > correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> > > I didn't get the way how __setup parameters are handled correctly.
> > > The patch bellow fixes that.
> > > 
> > > I am CCing stable as well because the patch got into .37 kernel.
> > > 
> > > ---
> > > >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@suse.cz>
> > > Date: Wed, 26 Jan 2011 14:12:41 +0100
> > > Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> > > 
> > > __setup based kernel command line parameters handled in
> > > obsolete_checksetup provides the parameter value including = (more
> > > precisely everything right after the parameter name) so we have to check
> > > for =0 resp. =1 here. If no value is given then we get an empty string
> > > rather then NULL.
> > 
> > This doesn't provide a description of the bug which just got fixed.
> > 
> > From reading the code I think the current behaviour is
> > 
> > "swapaccount": works OK
> 
> Not really because the original test was !s || s="1" but as I am writing
> in the commit message we are getting an empty string rather than NULL in
> no parameter value case..
> So noswapaccount is actually the only thing that is working.
> 
> > "noswapaccount": works OK
> > "swapaccount=0": doesn't do anything
> > "swapaccount=1": doesn't do anything
> > 
> > but I might be wrong about that.  Please send a changelog update to
> > clarify all this.
> 
> Sorry for not being specific enough. What about somthing like this:
> ---
> From 317dec3d13ef7f11e8f2699331bc32fcd6a8ea0e Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Wed, 26 Jan 2011 14:12:41 +0100
> Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> 
> __setup based kernel command line parameters handlers which are handled in
> obsolete_checksetup are provided with the parameter value including =
> (more precisely everything right after the parameter name).
> 
> This means that the current implementation of swapaccount[=1|0] doesn't
> work at all because if there is a value for the parameter then we are
> testing for "0" resp. "1" but we are getting "=0" resp. "=1" and if
> there is no parameter value we are getting an empty string rather than
> NULL.
> 
> The original noswapccount parameter, which doesn't care about the value,
> works correctly.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/memcontrol.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index db76ef7..cea2be48 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
>  static int __init enable_swap_account(char *s)
>  {
>  	/* consider enabled if no parameter or 1 is given */
> -	if (!s || !strcmp(s, "1"))
> +	if (!(*s) || !strcmp(s, "=1"))
>  		really_do_swap_account = 1;
> -	else if (!strcmp(s, "0"))
> +	else if (!strcmp(s, "=0"))
>  		really_do_swap_account = 0;
>  	return 1;
>  }

Hmm, usual callser of __setup() includes '=' to parameter name, as

mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);

How about moving "=" to __setup() ?


Thanks,
-Kame

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 09:23:20 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Wed 26-01-11 14:06:18, Andrew Morton wrote:
> > On Wed, 26 Jan 2011 16:21:58 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> > 
> > > I am sorry but the patch which added swapaccount parameter is not
> > > correct (we have discussed it https://lkml.org/lkml/2010/11/16/103).
> > > I didn't get the way how __setup parameters are handled correctly.
> > > The patch bellow fixes that.
> > > 
> > > I am CCing stable as well because the patch got into .37 kernel.
> > > 
> > > ---
> > > >From 144c2e8aed27d82d48217896ee1f58dbaa7f1f84 Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@suse.cz>
> > > Date: Wed, 26 Jan 2011 14:12:41 +0100
> > > Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> > > 
> > > __setup based kernel command line parameters handled in
> > > obsolete_checksetup provides the parameter value including = (more
> > > precisely everything right after the parameter name) so we have to check
> > > for =0 resp. =1 here. If no value is given then we get an empty string
> > > rather then NULL.
> > 
> > This doesn't provide a description of the bug which just got fixed.
> > 
> > From reading the code I think the current behaviour is
> > 
> > "swapaccount": works OK
> 
> Not really because the original test was !s || s="1" but as I am writing
> in the commit message we are getting an empty string rather than NULL in
> no parameter value case..
> So noswapaccount is actually the only thing that is working.
> 
> > "noswapaccount": works OK
> > "swapaccount=0": doesn't do anything
> > "swapaccount=1": doesn't do anything
> > 
> > but I might be wrong about that.  Please send a changelog update to
> > clarify all this.
> 
> Sorry for not being specific enough. What about somthing like this:
> ---
> From 317dec3d13ef7f11e8f2699331bc32fcd6a8ea0e Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Wed, 26 Jan 2011 14:12:41 +0100
> Subject: [PATCH] memsw: handle swapaccount kernel parameter correctly
> 
> __setup based kernel command line parameters handlers which are handled in
> obsolete_checksetup are provided with the parameter value including =
> (more precisely everything right after the parameter name).
> 
> This means that the current implementation of swapaccount[=1|0] doesn't
> work at all because if there is a value for the parameter then we are
> testing for "0" resp. "1" but we are getting "=0" resp. "=1" and if
> there is no parameter value we are getting an empty string rather than
> NULL.
> 
> The original noswapccount parameter, which doesn't care about the value,
> works correctly.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/memcontrol.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index db76ef7..cea2be48 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
>  static int __init enable_swap_account(char *s)
>  {
>  	/* consider enabled if no parameter or 1 is given */
> -	if (!s || !strcmp(s, "1"))
> +	if (!(*s) || !strcmp(s, "=1"))
>  		really_do_swap_account = 1;
> -	else if (!strcmp(s, "0"))
> +	else if (!strcmp(s, "=0"))
>  		really_do_swap_account = 0;
>  	return 1;
>  }

Hmm, usual callser of __setup() includes '=' to parameter name, as

mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);

How about moving "=" to __setup() ?


Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

On Thu 27-01-11 18:03:30, KAMEZAWA Hiroyuki wrote:
> On Thu, 27 Jan 2011 09:23:20 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index db76ef7..cea2be48 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
> >  static int __init enable_swap_account(char *s)
> >  {
> >  	/* consider enabled if no parameter or 1 is given */
> > -	if (!s || !strcmp(s, "1"))
> > +	if (!(*s) || !strcmp(s, "=1"))
> >  		really_do_swap_account = 1;
> > -	else if (!strcmp(s, "0"))
> > +	else if (!strcmp(s, "=0"))
> >  		really_do_swap_account = 0;
> >  	return 1;
> >  }
> 
> Hmm, usual callser of __setup() includes '=' to parameter name, as
> 
> mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
> mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);
> 
> How about moving "=" to __setup() ?

I have considered that as well but then we couldn't use swapaccount
parameter without any value because the parameter parsing matches the
whole string. 
I found it better to have consistent [no]swapaccount with the =0|1
extension rather than keeping = in the setup like other users.

Sounds reasonable?
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[Michal Hocko]

On Thu 27-01-11 18:03:30, KAMEZAWA Hiroyuki wrote:
> On Thu, 27 Jan 2011 09:23:20 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index db76ef7..cea2be48 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
> >  static int __init enable_swap_account(char *s)
> >  {
> >  	/* consider enabled if no parameter or 1 is given */
> > -	if (!s || !strcmp(s, "1"))
> > +	if (!(*s) || !strcmp(s, "=1"))
> >  		really_do_swap_account = 1;
> > -	else if (!strcmp(s, "0"))
> > +	else if (!strcmp(s, "=0"))
> >  		really_do_swap_account = 0;
> >  	return 1;
> >  }
> 
> Hmm, usual callser of __setup() includes '=' to parameter name, as
> 
> mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
> mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);
> 
> How about moving "=" to __setup() ?

I have considered that as well but then we couldn't use swapaccount
parameter without any value because the parameter parsing matches the
whole string. 
I found it better to have consistent [no]swapaccount with the =0|1
extension rather than keeping = in the setup like other users.

Sounds reasonable?
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 10:29:51 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 27-01-11 18:03:30, KAMEZAWA Hiroyuki wrote:
> > On Thu, 27 Jan 2011 09:23:20 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> [...]
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index db76ef7..cea2be48 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
> > >  static int __init enable_swap_account(char *s)
> > >  {
> > >  	/* consider enabled if no parameter or 1 is given */
> > > -	if (!s || !strcmp(s, "1"))
> > > +	if (!(*s) || !strcmp(s, "=1"))
> > >  		really_do_swap_account = 1;
> > > -	else if (!strcmp(s, "0"))
> > > +	else if (!strcmp(s, "=0"))
> > >  		really_do_swap_account = 0;
> > >  	return 1;
> > >  }
> > 
> > Hmm, usual callser of __setup() includes '=' to parameter name, as
> > 
> > mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
> > mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);
> > 
> > How about moving "=" to __setup() ?
> 
> I have considered that as well but then we couldn't use swapaccount
> parameter without any value because the parameter parsing matches the
> whole string. 
> I found it better to have consistent [no]swapaccount with the =0|1
> extension rather than keeping = in the setup like other users.
> 
> Sounds reasonable?

Hmm. ok for this time.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


Could you try to write a patch for feature-removal-schedule.txt
and tries to remove noswapaccount and do clean up all ?
(And add warning to noswapaccount will be removed.....in 2.6.40)


Thanks,
-Kame

Re: [PATCH] memsw: handle swapaccount kernel parameter correctly

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 10:29:51 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 27-01-11 18:03:30, KAMEZAWA Hiroyuki wrote:
> > On Thu, 27 Jan 2011 09:23:20 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> [...]
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index db76ef7..cea2be48 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -5013,9 +5013,9 @@ struct cgroup_subsys mem_cgroup_subsys = {
> > >  static int __init enable_swap_account(char *s)
> > >  {
> > >  	/* consider enabled if no parameter or 1 is given */
> > > -	if (!s || !strcmp(s, "1"))
> > > +	if (!(*s) || !strcmp(s, "=1"))
> > >  		really_do_swap_account = 1;
> > > -	else if (!strcmp(s, "0"))
> > > +	else if (!strcmp(s, "=0"))
> > >  		really_do_swap_account = 0;
> > >  	return 1;
> > >  }
> > 
> > Hmm, usual callser of __setup() includes '=' to parameter name, as
> > 
> > mm/hugetlb.c:__setup("hugepages=", hugetlb_nrpages_setup);
> > mm/hugetlb.c:__setup("default_hugepagesz=", hugetlb_default_setup);
> > 
> > How about moving "=" to __setup() ?
> 
> I have considered that as well but then we couldn't use swapaccount
> parameter without any value because the parameter parsing matches the
> whole string. 
> I found it better to have consistent [no]swapaccount with the =0|1
> extension rather than keeping = in the setup like other users.
> 
> Sounds reasonable?

Hmm. ok for this time.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


Could you try to write a patch for feature-removal-schedule.txt
and tries to remove noswapaccount and do clean up all ?
(And add warning to noswapaccount will be removed.....in 2.6.40)


Thanks,
-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[Michal Hocko]

On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> Could you try to write a patch for feature-removal-schedule.txt
> and tries to remove noswapaccount and do clean up all ?
> (And add warning to noswapaccount will be removed.....in 2.6.40)

Sure, no problem. What do you think about the following patch?
---
>From a597421909a3291886345565c73102117a52301e Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Thu, 27 Jan 2011 11:41:01 +0100
Subject: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

noswapaccount couldn't be used to control memsw for both on/off cases so
we have added swapaccount[=0|1] parameter. This way we can turn the
feature in two ways noswapaccount resp. swapaccount=0. We have kept the
original noswapaccount but I think we should remove it after some time
as it just makes more command line parameters without any advantages and
also the code to handle parameters is uglier if we want both parameters.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Requested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/feature-removal-schedule.txt |   16 ++++++++++++++++
 mm/memcontrol.c                            |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index b959659..b3f35e5 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -603,3 +603,19 @@ Why:	The adm9240, w83792d and w83793 hardware monitoring drivers have
 Who:	Jean Delvare <khali@linux-fr.org>
 
 ----------------------------
+
+What:	noswapaccount kernel command line parameter
+When:	2.6.40
+Why:	The original implementation of memsw feature enabled by
+	CONFIG_CGROUP_MEM_RES_CTLR_SWAP could be disabled by the noswapaccount
+	kernel parameter (introduced in 2.6.29-rc1). Later on, this decision
+	turned out to be not ideal because we cannot have the feature compiled
+	in and disabled by default and let only interested to enable it
+	(e.g. general distribution kernels might need it). Therefore we have
+	added swapaccount[=0|1] parameter (introduced in 2.6.37) which provides
+	the both possibilities. If we remove noswapaccount we will have
+	less command line parameters with the same functionality and we
+	can also cleanup the parameter handling a bit ().
+Who:	Michal Hocko <mhocko@suse.cz>
+
+----------------------------
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index cea2be48..0387287 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5023,6 +5023,7 @@ __setup("swapaccount", enable_swap_account);
 
 static int __init disable_swap_account(char *s)
 {
+	printk_once("noswapaccount is deprecated and will be removed in 2.6.40. Use swapaccount=0 instead\n");
 	enable_swap_account("=0");
 	return 1;
 }
-- 
1.7.2.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

[PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[Michal Hocko]

On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> Could you try to write a patch for feature-removal-schedule.txt
> and tries to remove noswapaccount and do clean up all ?
> (And add warning to noswapaccount will be removed.....in 2.6.40)

Sure, no problem. What do you think about the following patch?
---

Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 11:47:59 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> > Could you try to write a patch for feature-removal-schedule.txt
> > and tries to remove noswapaccount and do clean up all ?
> > (And add warning to noswapaccount will be removed.....in 2.6.40)
> 
> Sure, no problem. What do you think about the following patch?
> ---
> From a597421909a3291886345565c73102117a52301e Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 27 Jan 2011 11:41:01 +0100
> Subject: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
> 
> noswapaccount couldn't be used to control memsw for both on/off cases so
> we have added swapaccount[=0|1] parameter. This way we can turn the
> feature in two ways noswapaccount resp. swapaccount=0. We have kept the
> original noswapaccount but I think we should remove it after some time
> as it just makes more command line parameters without any advantages and
> also the code to handle parameters is uglier if we want both parameters.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Requested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Nice!. Thank you.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Maybe other discussion as 2.6.40 is too early or some may happen.
But Ack from me, at least.

Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 11:47:59 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> > Could you try to write a patch for feature-removal-schedule.txt
> > and tries to remove noswapaccount and do clean up all ?
> > (And add warning to noswapaccount will be removed.....in 2.6.40)
> 
> Sure, no problem. What do you think about the following patch?
> ---
> From a597421909a3291886345565c73102117a52301e Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 27 Jan 2011 11:41:01 +0100
> Subject: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
> 
> noswapaccount couldn't be used to control memsw for both on/off cases so
> we have added swapaccount[=0|1] parameter. This way we can turn the
> feature in two ways noswapaccount resp. swapaccount=0. We have kept the
> original noswapaccount but I think we should remove it after some time
> as it just makes more command line parameters without any advantages and
> also the code to handle parameters is uglier if we want both parameters.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Requested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Nice!. Thank you.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Maybe other discussion as 2.6.40 is too early or some may happen.
But Ack from me, at least.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[Michal Hocko]

On Fri 28-01-11 08:37:03, KAMEZAWA Hiroyuki wrote:
> On Thu, 27 Jan 2011 11:47:59 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> > > Could you try to write a patch for feature-removal-schedule.txt
> > > and tries to remove noswapaccount and do clean up all ?
> > > (And add warning to noswapaccount will be removed.....in 2.6.40)
> > 
> > Sure, no problem. What do you think about the following patch?
> > ---
> > From a597421909a3291886345565c73102117a52301e Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Thu, 27 Jan 2011 11:41:01 +0100
> > Subject: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
> > 
> > noswapaccount couldn't be used to control memsw for both on/off cases so
> > we have added swapaccount[=0|1] parameter. This way we can turn the
> > feature in two ways noswapaccount resp. swapaccount=0. We have kept the
> > original noswapaccount but I think we should remove it after some time
> > as it just makes more command line parameters without any advantages and
> > also the code to handle parameters is uglier if we want both parameters.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > Requested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Nice!. Thank you.
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Thanks

> 
> Maybe other discussion as 2.6.40 is too early or some may happen.
> But Ack from me, at least.

Sure.

Just for record here is the patch to remove and cleanup. I can split it
into 2 patches (one for removal and the other for the cleanup) but this
can be done later as well.
---

>From 212b3bae0d32a55a7422c8089a4cebfb9ca4a42f Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Fri, 28 Jan 2011 08:57:32 +0100
Subject: [PATCH] memsw: remove noswapaccount kernel parameter

noswapaccount parameter has been deprecated for some time without any
complains from users so we can remove it. swapaccount=0|1 can be used
instead.

As we are removing the parameter we can also cleanup swapaccount
because it doesn't have to accept an empty string anymore (to match
noswapaccount) and so we can push = into __setup macro rather than
checking "=1" resp. "=0" strings

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 Documentation/kernel-parameters.txt |    3 ---
 mm/memcontrol.c                     |   13 +++----------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index b72e071..a8fe114 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1746,9 +1746,6 @@ and is between 256 and 4096 characters. It is defined in the file
 
 	nosoftlockup	[KNL] Disable the soft-lockup detector.
 
-	noswapaccount	[KNL] Disable accounting of swap in memory resource
-			controller. (See Documentation/cgroups/memory.txt)
-
 	nosync		[HW,M68K] Disables sync negotiation for all devices.
 
 	notsc		[BUGS=X86-32] Disable Time Stamp Counter
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0387287..96c3471 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5013,19 +5013,12 @@ struct cgroup_subsys mem_cgroup_subsys = {
 static int __init enable_swap_account(char *s)
 {
 	/* consider enabled if no parameter or 1 is given */
-	if (!(*s) || !strcmp(s, "=1"))
+	if (!strcmp(s, "1"))
 		really_do_swap_account = 1;
-	else if (!strcmp(s, "=0"))
+	else if (!strcmp(s, "0"))
 		really_do_swap_account = 0;
 	return 1;
 }
-__setup("swapaccount", enable_swap_account);
+__setup("swapaccount=", enable_swap_account);
 
-static int __init disable_swap_account(char *s)
-{
-	printk_once("noswapaccount is deprecated and will be removed in 2.6.40. Use swapaccount=0 instead\n");
-	enable_swap_account("=0");
-	return 1;
-}
-__setup("noswapaccount", disable_swap_account);
 #endif
-- 
1.7.2.3


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal

[Michal Hocko]

On Fri 28-01-11 08:37:03, KAMEZAWA Hiroyuki wrote:
> On Thu, 27 Jan 2011 11:47:59 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Thu 27-01-11 18:48:27, KAMEZAWA Hiroyuki wrote:
> > > Could you try to write a patch for feature-removal-schedule.txt
> > > and tries to remove noswapaccount and do clean up all ?
> > > (And add warning to noswapaccount will be removed.....in 2.6.40)
> > 
> > Sure, no problem. What do you think about the following patch?
> > ---
> > From a597421909a3291886345565c73102117a52301e Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Thu, 27 Jan 2011 11:41:01 +0100
> > Subject: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
> > 
> > noswapaccount couldn't be used to control memsw for both on/off cases so
> > we have added swapaccount[=0|1] parameter. This way we can turn the
> > feature in two ways noswapaccount resp. swapaccount=0. We have kept the
> > original noswapaccount but I think we should remove it after some time
> > as it just makes more command line parameters without any advantages and
> > also the code to handle parameters is uglier if we want both parameters.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > Requested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Nice!. Thank you.
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Thanks

> 
> Maybe other discussion as 2.6.40 is too early or some may happen.
> But Ack from me, at least.

Sure.

Just for record here is the patch to remove and cleanup. I can split it
into 2 patches (one for removal and the other for the cleanup) but this
can be done later as well.
---