[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.

---

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:
---

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

--
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




--
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?
---

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.
---