LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] memsw: handle swapaccount kernel parameter correctly
@ 2011-01-26 15:21 Michal Hocko
  2011-01-26 22:06 ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2011-01-26 15:21 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, balbir, KAMEZAWA Hiroyuki,
	Daisuke Nishimura, stable

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: handle swapaccount kernel parameter correctly
  2011-01-26 15:21 [PATCH] memsw: handle swapaccount kernel parameter correctly Michal Hocko
@ 2011-01-26 22:06 ` Andrew Morton
  2011-01-27  8:23   ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2011-01-26 22:06 UTC (permalink / raw)
  To: Michal Hocko
  Cc: linux-mm, linux-kernel, balbir, KAMEZAWA Hiroyuki,
	Daisuke Nishimura, stable

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: handle swapaccount kernel parameter correctly
  2011-01-26 22:06 ` Andrew Morton
@ 2011-01-27  8:23   ` Michal Hocko
  2011-01-27  9:03     ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2011-01-27  8:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, balbir, KAMEZAWA Hiroyuki,
	Daisuke Nishimura, stable

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: handle swapaccount kernel parameter correctly
  2011-01-27  8:23   ` Michal Hocko
@ 2011-01-27  9:03     ` KAMEZAWA Hiroyuki
  2011-01-27  9:29       ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-01-27  9:03 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura, stable

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: handle swapaccount kernel parameter correctly
  2011-01-27  9:03     ` KAMEZAWA Hiroyuki
@ 2011-01-27  9:29       ` Michal Hocko
  2011-01-27  9:48         ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2011-01-27  9:29 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura, stable

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: handle swapaccount kernel parameter correctly
  2011-01-27  9:29       ` Michal Hocko
@ 2011-01-27  9:48         ` KAMEZAWA Hiroyuki
  2011-01-27 10:47           ` [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-01-27  9:48 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura, stable

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
  2011-01-27  9:48         ` KAMEZAWA Hiroyuki
@ 2011-01-27 10:47           ` Michal Hocko
  2011-01-27 23:37             ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2011-01-27 10:47 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
  2011-01-27 10:47           ` [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal Michal Hocko
@ 2011-01-27 23:37             ` KAMEZAWA Hiroyuki
  2011-01-28  8:03               ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-01-27 23:37 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal
  2011-01-27 23:37             ` KAMEZAWA Hiroyuki
@ 2011-01-28  8:03               ` Michal Hocko
  0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2011-01-28  8:03 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki
  Cc: Andrew Morton, linux-mm, linux-kernel, balbir, Daisuke Nishimura

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-01-28  8:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-26 15:21 [PATCH] memsw: handle swapaccount kernel parameter correctly Michal Hocko
2011-01-26 22:06 ` Andrew Morton
2011-01-27  8:23   ` Michal Hocko
2011-01-27  9:03     ` KAMEZAWA Hiroyuki
2011-01-27  9:29       ` Michal Hocko
2011-01-27  9:48         ` KAMEZAWA Hiroyuki
2011-01-27 10:47           ` [PATCH] memsw: Deprecate noswapaccount kernel parameter and schedule it for removal Michal Hocko
2011-01-27 23:37             ` KAMEZAWA Hiroyuki
2011-01-28  8:03               ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).