* [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. --- ^ 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. -- 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> ^ 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: --- ^ 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 -- 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> ^ 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 -- 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> ^ 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 -- 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> ^ 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? --- ^ 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. -- 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> ^ 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. --- ^ 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).