Hi Andrew,
could you consider the following patch for the Linus tree, please?
The discussion took place in this email thread
http://lkml.org/lkml/2010/11/10/114.
The patch is based on top of 151f52f09c572 commit in the Linus tree.
Please let me know if there I should route this patch through somebody
else.
Thanks!
---
>From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Wed, 10 Nov 2010 13:30:04 +0100
Subject: [PATCH] Make swap accounting default behavior configurable
Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
configuration option and then it is turned on by default. There is
a boot option (noswapaccount) which can disable this feature.
This makes it hard for distributors to enable the configuration option
as this feature leads to a bigger memory consumption and this is a no-go
for general purpose distribution kernel. On the other hand swap
accounting may be very usuful for some workloads.
This patch adds a new configuration option which controls the default
behavior (CGROUP_MEM_RES_CTLR_SWAP_ENABLED). If the option is selected
then the feature is turned on by default.
It also adds a new boot parameter swapaccount which (contrary to
noswapaccount) enables the feature. (I would consider swapaccount=yes|no
semantic with removed noswapaccount parameter much better but this
parameter is kind of API which might be in use and unexpected breakage
is no-go.)
The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled as well)
Signed-off-by: Michal Hocko <[email protected]>
Acked-by: Daisuke Nishimura <[email protected]>
---
Documentation/kernel-parameters.txt | 3 +++
init/Kconfig | 13 +++++++++++++
mm/memcontrol.c | 15 ++++++++++++++-
3 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ed45e98..14eafa5 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
improve throughput, but will also increase the
amount of memory reserved for use by the client.
+ swapaccount [KNL] Enable accounting of swap in memory resource
+ controller. (See Documentation/cgroups/memory.txt)
+
swiotlb= [IA-64] Number of I/O TLB slabs
switches= [HW,M68k]
diff --git a/init/Kconfig b/init/Kconfig
index 88c1046..c972899 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -613,6 +613,19 @@ config CGROUP_MEM_RES_CTLR_SWAP
if boot option "noswapaccount" is set, swap will not be accounted.
Now, memory usage of swap_cgroup is 2 bytes per entry. If swap page
size is 4096bytes, 512k per 1Gbytes of swap.
+config CGROUP_MEM_RES_CTLR_SWAP_ENABLED
+ bool "Memory Resource Controller Swap Extension enabled by default"
+ depends on CGROUP_MEM_RES_CTLR_SWAP
+ default y
+ help
+ Memory Resource Controller Swap Extension comes with its price in
+ a bigger memory consumption. General purpose distribution kernels
+ which want to enable the feautre but keep it disabled by default
+ and let the user enable it by swapaccount boot command line
+ parameter should have this option unselected.
+ For those who want to have the feature enabled by default should
+ select this option (if, for some reason, they need to disable it
+ then noswapaccount does the trick).
menuconfig CGROUP_SCHED
bool "Group CPU scheduler"
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9a99cfa..4f479fe 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -61,7 +61,14 @@ struct mem_cgroup *root_mem_cgroup __read_mostly;
#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
/* Turned on only when memory cgroup is enabled && really_do_swap_account = 1 */
int do_swap_account __read_mostly;
-static int really_do_swap_account __initdata = 1; /* for remember boot option*/
+
+/* for remember boot option*/
+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED
+static int really_do_swap_account __initdata = 1;
+#else
+static int really_do_swap_account __initdata = 0;
+#endif
+
#else
#define do_swap_account (0)
#endif
@@ -4909,6 +4916,12 @@ struct cgroup_subsys mem_cgroup_subsys = {
};
#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
+static int __init enable_swap_account(char *s)
+{
+ really_do_swap_account = 1;
+ return 1;
+}
+__setup("swapaccount", enable_swap_account);
static int __init disable_swap_account(char *s)
{
--
1.7.2.3
--
Michal Hocko
L3 team
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
On Tue, 16 Nov 2010 11:17:26 +0100
Michal Hocko <[email protected]> wrote:
> Hi Andrew,
> could you consider the following patch for the Linus tree, please?
> The discussion took place in this email thread
> http://lkml.org/lkml/2010/11/10/114.
> The patch is based on top of 151f52f09c572 commit in the Linus tree.
>
> Please let me know if there I should route this patch through somebody
> else.
>
> Thanks!
>
> ---
> >From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <[email protected]>
> Date: Wed, 10 Nov 2010 13:30:04 +0100
> Subject: [PATCH] Make swap accounting default behavior configurable
>
> Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> configuration option and then it is turned on by default. There is
> a boot option (noswapaccount) which can disable this feature.
>
> This makes it hard for distributors to enable the configuration option
> as this feature leads to a bigger memory consumption and this is a no-go
> for general purpose distribution kernel. On the other hand swap
> accounting may be very usuful for some workloads.
This patch is needed by distros, and distros use the -stable tree, I
assume. Do you see reasons why this patch should be backported into
-stable, so distros don't need to patch it themselves? If so, any
particular kernel versions? 2.6.37?
> This patch adds a new configuration option which controls the default
> behavior (CGROUP_MEM_RES_CTLR_SWAP_ENABLED). If the option is selected
> then the feature is turned on by default.
>
> It also adds a new boot parameter swapaccount which (contrary to
> noswapaccount) enables the feature. (I would consider swapaccount=yes|no
> semantic with removed noswapaccount parameter much better but this
> parameter is kind of API which might be in use and unexpected breakage
> is no-go.)
>
> The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
> enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled as well)
>
> Signed-off-by: Michal Hocko <[email protected]>
> Acked-by: Daisuke Nishimura <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 3 +++
> init/Kconfig | 13 +++++++++++++
> mm/memcontrol.c | 15 ++++++++++++++-
> 3 files changed, 30 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ed45e98..14eafa5 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> improve throughput, but will also increase the
> amount of memory reserved for use by the client.
>
> + swapaccount [KNL] Enable accounting of swap in memory resource
> + controller. (See Documentation/cgroups/memory.txt)
So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
would have been better.
On Tue, Nov 16, 2010 at 12:46:15PM -0800, Andrew Morton wrote:
> On Tue, 16 Nov 2010 11:17:26 +0100
> Michal Hocko <[email protected]> wrote:
>
> > Hi Andrew,
> > could you consider the following patch for the Linus tree, please?
> > The discussion took place in this email thread
> > http://lkml.org/lkml/2010/11/10/114.
> > The patch is based on top of 151f52f09c572 commit in the Linus tree.
> >
> > Please let me know if there I should route this patch through somebody
> > else.
> >
> > Thanks!
> >
> > ---
> > >From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <[email protected]>
> > Date: Wed, 10 Nov 2010 13:30:04 +0100
> > Subject: [PATCH] Make swap accounting default behavior configurable
> >
> > Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > configuration option and then it is turned on by default. There is
> > a boot option (noswapaccount) which can disable this feature.
> >
> > This makes it hard for distributors to enable the configuration option
> > as this feature leads to a bigger memory consumption and this is a no-go
> > for general purpose distribution kernel. On the other hand swap
> > accounting may be very usuful for some workloads.
>
> This patch is needed by distros, and distros use the -stable tree, I
> assume. Do you see reasons why this patch should be backported into
> -stable, so distros don't need to patch it themselves? If so, any
> particular kernel versions? 2.6.37?
Sorry, I really don't want to start backporting features to stable
kernels if at all possible. Distros can pick them up on their own if
they determine it is needed.
thanks,
greg k-h
On Tue, 16 Nov 2010 12:46:15 -0800
Andrew Morton <[email protected]> wrote:
> On Tue, 16 Nov 2010 11:17:26 +0100
> Michal Hocko <[email protected]> wrote:
>
> > Hi Andrew,
> > could you consider the following patch for the Linus tree, please?
> > The discussion took place in this email thread
> > http://lkml.org/lkml/2010/11/10/114.
> > The patch is based on top of 151f52f09c572 commit in the Linus tree.
> >
> > Please let me know if there I should route this patch through somebody
> > else.
> >
> > Thanks!
> >
> > ---
> > >From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <[email protected]>
> > Date: Wed, 10 Nov 2010 13:30:04 +0100
> > Subject: [PATCH] Make swap accounting default behavior configurable
> >
> > Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > configuration option and then it is turned on by default. There is
> > a boot option (noswapaccount) which can disable this feature.
> >
> > This makes it hard for distributors to enable the configuration option
> > as this feature leads to a bigger memory consumption and this is a no-go
> > for general purpose distribution kernel. On the other hand swap
> > accounting may be very usuful for some workloads.
>
> This patch is needed by distros, and distros use the -stable tree, I
> assume. Do you see reasons why this patch should be backported into
> -stable, so distros don't need to patch it themselves? If so, any
> particular kernel versions? 2.6.37?
>
> > This patch adds a new configuration option which controls the default
> > behavior (CGROUP_MEM_RES_CTLR_SWAP_ENABLED). If the option is selected
> > then the feature is turned on by default.
> >
> > It also adds a new boot parameter swapaccount which (contrary to
> > noswapaccount) enables the feature. (I would consider swapaccount=yes|no
> > semantic with removed noswapaccount parameter much better but this
> > parameter is kind of API which might be in use and unexpected breakage
> > is no-go.)
> >
> > The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
> > enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled as well)
> >
> > Signed-off-by: Michal Hocko <[email protected]>
> > Acked-by: Daisuke Nishimura <[email protected]>
> > ---
> > Documentation/kernel-parameters.txt | 3 +++
> > init/Kconfig | 13 +++++++++++++
> > mm/memcontrol.c | 15 ++++++++++++++-
> > 3 files changed, 30 insertions(+), 1 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index ed45e98..14eafa5 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > improve throughput, but will also increase the
> > amount of memory reserved for use by the client.
> >
> > + swapaccount [KNL] Enable accounting of swap in memory resource
> > + controller. (See Documentation/cgroups/memory.txt)
>
> So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> would have been better.
>
I suggested to keep "noswapaccount" for compatibility.
If you and other guys don't like having two parameters, I don't stick to
the old parameter.
Thanks,
Daisuke Nishimura.
On Wed, 17 Nov 2010 09:23:39 +0900
Daisuke Nishimura <[email protected]> wrote:
> On Tue, 16 Nov 2010 12:46:15 -0800
> Andrew Morton <[email protected]> wrote:
>
> > On Tue, 16 Nov 2010 11:17:26 +0100
> > Michal Hocko <[email protected]> wrote:
> >
> > > Hi Andrew,
> > > could you consider the following patch for the Linus tree, please?
> > > The discussion took place in this email thread
> > > http://lkml.org/lkml/2010/11/10/114.
> > > The patch is based on top of 151f52f09c572 commit in the Linus tree.
> > >
> > > Please let me know if there I should route this patch through somebody
> > > else.
> > >
> > > Thanks!
> > >
> > > ---
> > > >From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <[email protected]>
> > > Date: Wed, 10 Nov 2010 13:30:04 +0100
> > > Subject: [PATCH] Make swap accounting default behavior configurable
> > >
> > > Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > > configuration option and then it is turned on by default. There is
> > > a boot option (noswapaccount) which can disable this feature.
> > >
> > > This makes it hard for distributors to enable the configuration option
> > > as this feature leads to a bigger memory consumption and this is a no-go
> > > for general purpose distribution kernel. On the other hand swap
> > > accounting may be very usuful for some workloads.
> >
> > This patch is needed by distros, and distros use the -stable tree, I
> > assume. Do you see reasons why this patch should be backported into
> > -stable, so distros don't need to patch it themselves? If so, any
> > particular kernel versions? 2.6.37?
> >
> > > This patch adds a new configuration option which controls the default
> > > behavior (CGROUP_MEM_RES_CTLR_SWAP_ENABLED). If the option is selected
> > > then the feature is turned on by default.
> > >
> > > It also adds a new boot parameter swapaccount which (contrary to
> > > noswapaccount) enables the feature. (I would consider swapaccount=yes|no
> > > semantic with removed noswapaccount parameter much better but this
> > > parameter is kind of API which might be in use and unexpected breakage
> > > is no-go.)
> > >
> > > The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
> > > enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled as well)
> > >
> > > Signed-off-by: Michal Hocko <[email protected]>
> > > Acked-by: Daisuke Nishimura <[email protected]>
> > > ---
> > > Documentation/kernel-parameters.txt | 3 +++
> > > init/Kconfig | 13 +++++++++++++
> > > mm/memcontrol.c | 15 ++++++++++++++-
> > > 3 files changed, 30 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > index ed45e98..14eafa5 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > improve throughput, but will also increase the
> > > amount of memory reserved for use by the client.
> > >
> > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > + controller. (See Documentation/cgroups/memory.txt)
> >
> > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > would have been better.
> >
> I suggested to keep "noswapaccount" for compatibility.
> If you and other guys don't like having two parameters, I don't stick to
> the old parameter.
>
I don't think "noswapaccount" is important if "enable is default" when
proper configuration is used ....because it's rarelly used.
BTW, memory usage of swap_cgroup is really important ? It consumes
1 Mbytes per 2G of swap.
Off topic.
I wonder I'll be happy if we can have default config template for all
others as recent "Add Kconfig option for default swappiness" discuss.
Thanks,
-Kame
Thanks,
-Kame
On Wed, 17 Nov 2010 09:23:39 +0900 Daisuke Nishimura <[email protected]> wrote:
> > >
> > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > index ed45e98..14eafa5 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > improve throughput, but will also increase the
> > > amount of memory reserved for use by the client.
> > >
> > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > + controller. (See Documentation/cgroups/memory.txt)
> >
> > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > would have been better.
> >
> I suggested to keep "noswapaccount" for compatibility.
> If you and other guys don't like having two parameters, I don't stick to
> the old parameter.
>
Yes, we're stuck with the old one now.
But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
Even if we didn't initially intend to add "nofoo".
On Tue, 16 Nov 2010 17:12:25 -0800
Andrew Morton <[email protected]> wrote:
> On Wed, 17 Nov 2010 09:23:39 +0900 Daisuke Nishimura <[email protected]> wrote:
>
> > > >
> > > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > > index ed45e98..14eafa5 100644
> > > > --- a/Documentation/kernel-parameters.txt
> > > > +++ b/Documentation/kernel-parameters.txt
> > > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > > improve throughput, but will also increase the
> > > > amount of memory reserved for use by the client.
> > > >
> > > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > > + controller. (See Documentation/cgroups/memory.txt)
> > >
> > > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > > would have been better.
> > >
> > I suggested to keep "noswapaccount" for compatibility.
> > If you and other guys don't like having two parameters, I don't stick to
> > the old parameter.
> >
>
> Yes, we're stuck with the old one now.
>
> But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> Even if we didn't initially intend to add "nofoo".
>
I see.
Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
Thanks,
Daisuke Nishimura.
On Tue 16-11-10 13:21:57, Greg KH wrote:
> On Tue, Nov 16, 2010 at 12:46:15PM -0800, Andrew Morton wrote:
> > On Tue, 16 Nov 2010 11:17:26 +0100
> > Michal Hocko <[email protected]> wrote:
> >
> > > Hi Andrew,
> > > could you consider the following patch for the Linus tree, please?
> > > The discussion took place in this email thread
> > > http://lkml.org/lkml/2010/11/10/114.
> > > The patch is based on top of 151f52f09c572 commit in the Linus tree.
> > >
> > > Please let me know if there I should route this patch through somebody
> > > else.
> > >
> > > Thanks!
> > >
> > > ---
> > > >From 30238aaec758988493af793939f14b0ba83dc4b3 Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <[email protected]>
> > > Date: Wed, 10 Nov 2010 13:30:04 +0100
> > > Subject: [PATCH] Make swap accounting default behavior configurable
> > >
> > > Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > > configuration option and then it is turned on by default. There is
> > > a boot option (noswapaccount) which can disable this feature.
> > >
> > > This makes it hard for distributors to enable the configuration option
> > > as this feature leads to a bigger memory consumption and this is a no-go
> > > for general purpose distribution kernel. On the other hand swap
> > > accounting may be very usuful for some workloads.
> >
> > This patch is needed by distros, and distros use the -stable tree, I
> > assume. Do you see reasons why this patch should be backported into
> > -stable, so distros don't need to patch it themselves? If so, any
> > particular kernel versions? 2.6.37?
I have suggested pushing to the stable in the original thread as well.
I was told that this is not a bug fix.
I do not care much in which particular version to push this but I
guess this doesn't qualify as "regression fix only Linus policy" so it
is probably too late for .37.
Nevertheless, if we reconsider -stable then .37 would be much really
helpful to get it into stable ASAP.
>
> Sorry, I really don't want to start backporting features to stable
> kernels if at all possible. Distros can pick them up on their own if
> they determine it is needed.
I really do agree with the part about features. But isn't this patch
basically for distros (to help them to provide the swapaccounting feature
without the cost of higher memory consumption in default configuration)?
If this doesn't go to the stable then all (interested) of them would
need to maintain the patch. Otherwise the change would come directly
from the upstream.
Moreover, it is not a new feature it just consolidates the default
behavior of the already existing functionality.
>
> thanks,
>
> greg k-h
Thanks
--
Michal Hocko
L3 team
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> On Tue, 16 Nov 2010 17:12:25 -0800
> Andrew Morton <[email protected]> wrote:
>
> > On Wed, 17 Nov 2010 09:23:39 +0900 Daisuke Nishimura <[email protected]> wrote:
> >
> > > > >
> > > > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > > > index ed45e98..14eafa5 100644
> > > > > --- a/Documentation/kernel-parameters.txt
> > > > > +++ b/Documentation/kernel-parameters.txt
> > > > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > > > improve throughput, but will also increase the
> > > > > amount of memory reserved for use by the client.
> > > > >
> > > > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > > > + controller. (See Documentation/cgroups/memory.txt)
> > > >
> > > > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > > > would have been better.
> > > >
> > > I suggested to keep "noswapaccount" for compatibility.
> > > If you and other guys don't like having two parameters, I don't stick to
> > > the old parameter.
> > >
> >
> > Yes, we're stuck with the old one now.
> >
> > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > Even if we didn't initially intend to add "nofoo".
> >
> I see.
>
> Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
I have noticed that Andrew has already taken the last version of the
patch for -mm tree. Should I still rework it to change swapaccount to
swapaccount=0|1 resp. true|false?
>
> Thanks,
> Daisuke Nishimura.
--
Michal Hocko
L3 team
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
On Thu, 18 Nov 2010 09:23:32 +0100
Michal Hocko <[email protected]> wrote:
> On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> > On Tue, 16 Nov 2010 17:12:25 -0800
> > Andrew Morton <[email protected]> wrote:
> >
> > > On Wed, 17 Nov 2010 09:23:39 +0900 Daisuke Nishimura <[email protected]> wrote:
> > >
> > > > > >
> > > > > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > > > > index ed45e98..14eafa5 100644
> > > > > > --- a/Documentation/kernel-parameters.txt
> > > > > > +++ b/Documentation/kernel-parameters.txt
> > > > > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > > > > improve throughput, but will also increase the
> > > > > > amount of memory reserved for use by the client.
> > > > > >
> > > > > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > > > > + controller. (See Documentation/cgroups/memory.txt)
> > > > >
> > > > > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > > > > would have been better.
> > > > >
> > > > I suggested to keep "noswapaccount" for compatibility.
> > > > If you and other guys don't like having two parameters, I don't stick to
> > > > the old parameter.
> > > >
> > >
> > > Yes, we're stuck with the old one now.
> > >
> > > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > > Even if we didn't initially intend to add "nofoo".
> > >
> > I see.
> >
> > Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
>
> I have noticed that Andrew has already taken the last version of the
> patch for -mm tree. Should I still rework it to change swapaccount to
> swapaccount=0|1 resp. true|false?
>
It's usual to update a patch into more sophisticated one while it is in -mm tree.
So, I think you'd better to do it(btw, I prefer 0|1 to true|false.
Reading kernel-parameters.txt, 0|1 is more commonly used.).
Thanks,
Daisuke Nishimura.
On Thu, 18 Nov 2010 17:46:54 +0900
Daisuke Nishimura <[email protected]> wrote:
> On Thu, 18 Nov 2010 09:23:32 +0100
> Michal Hocko <[email protected]> wrote:
>
> > On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> > > On Tue, 16 Nov 2010 17:12:25 -0800
> > > Andrew Morton <[email protected]> wrote:
> > >
> > > > On Wed, 17 Nov 2010 09:23:39 +0900 Daisuke Nishimura <[email protected]> wrote:
> > > >
> > > > > > >
> > > > > > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > > > > > index ed45e98..14eafa5 100644
> > > > > > > --- a/Documentation/kernel-parameters.txt
> > > > > > > +++ b/Documentation/kernel-parameters.txt
> > > > > > > @@ -2385,6 +2385,9 @@ and is between 256 and 4096 characters. It is defined in the file
> > > > > > > improve throughput, but will also increase the
> > > > > > > amount of memory reserved for use by the client.
> > > > > > >
> > > > > > > + swapaccount [KNL] Enable accounting of swap in memory resource
> > > > > > > + controller. (See Documentation/cgroups/memory.txt)
> > > > > >
> > > > > > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > > > > > would have been better.
> > > > > >
> > > > > I suggested to keep "noswapaccount" for compatibility.
> > > > > If you and other guys don't like having two parameters, I don't stick to
> > > > > the old parameter.
> > > > >
> > > >
> > > > Yes, we're stuck with the old one now.
> > > >
> > > > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > > > Even if we didn't initially intend to add "nofoo".
> > > >
> > > I see.
> > >
> > > Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
> >
> > I have noticed that Andrew has already taken the last version of the
> > patch for -mm tree. Should I still rework it to change swapaccount to
> > swapaccount=0|1 resp. true|false?
> >
> It's usual to update a patch into more sophisticated one while it is in -mm tree.
> So, I think you'd better to do it(btw, I prefer 0|1 to true|false.
> Reading kernel-parameters.txt, 0|1 is more commonly used.).
>
I vote for 0|1
Thanks,
-Kame
On Thu 18-11-10 17:53:34, KAMEZAWA Hiroyuki wrote:
> On Thu, 18 Nov 2010 17:46:54 +0900
> Daisuke Nishimura <[email protected]> wrote:
>
> > On Thu, 18 Nov 2010 09:23:32 +0100
> > Michal Hocko <[email protected]> wrote:
> >
> > > On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> > > > On Tue, 16 Nov 2010 17:12:25 -0800
> > > > Andrew Morton <[email protected]> wrote:
[...]
> > > > > Yes, we're stuck with the old one now.
> > > > >
> > > > > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > > > > Even if we didn't initially intend to add "nofoo".
> > > > >
> > > > I see.
> > > >
> > > > Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
> > >
> > > I have noticed that Andrew has already taken the last version of the
> > > patch for -mm tree. Should I still rework it to change swapaccount to
> > > swapaccount=0|1 resp. true|false?
> > >
> > It's usual to update a patch into more sophisticated one while it is in -mm tree.
> > So, I think you'd better to do it(btw, I prefer 0|1 to true|false.
> > Reading kernel-parameters.txt, 0|1 is more commonly used.).
> >
>
> I vote for 0|1
Changes since v3:
* add 0|1 parameter values handling
Changes since v2:
* put the new parameter description to the proper (alphabetically
* sorted)
place in Documentation/kernel-parameters.txt
Changes since v1:
* do not remove noswapaccount parameter and add swapaccount parameter
* instead
* Documentation/kernel-parameters.txt updated)
---
>From 72c003f6586c092c1a69f5482cd102e97d7ef40f Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Wed, 10 Nov 2010 13:30:04 +0100
Subject: [PATCH] Make swap accounting default behavior configurable
Swap accounting can be configured by CONFIG_CGROUP_MEM_RES_CTLR_SWAP
configuration option and then it is turned on by default. There is
a boot option (noswapaccount) which can disable this feature.
This makes it hard for distributors to enable the configuration option
as this feature leads to a bigger memory consumption and this is a no-go
for general purpose distribution kernel. On the other hand swap
accounting may be very usuful for some workloads.
This patch adds a new configuration option which controls the default
behavior (CGROUP_MEM_RES_CTLR_SWAP_ENABLED). If the option is selected
then the feature is turned on by default.
It also adds a new boot parameter swapaccount[=1|0] which enhances the
original noswapaccount parameter semantic by means of enable/disable
logic (defaults to 1 if no value is provided to be still consistent with
noswapaccount).
The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled as well)
Signed-off-by: Michal Hocko <[email protected]>
---
Documentation/kernel-parameters.txt | 5 +++++
init/Kconfig | 13 +++++++++++++
mm/memcontrol.c | 21 +++++++++++++++++++--
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ed45e98..98c4902 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2385,6 +2385,11 @@ and is between 256 and 4096 characters. It is defined in the file
improve throughput, but will also increase the
amount of memory reserved for use by the client.
+ swapaccount[=0|1]
+ [KNL] Enable accounting of swap in memory resource
+ controller if no parameter or 1 is given or disable
+ it if 0 is given (See Documentation/cgroups/memory.txt)
+
swiotlb= [IA-64] Number of I/O TLB slabs
switches= [HW,M68k]
diff --git a/init/Kconfig b/init/Kconfig
index 88c1046..c972899 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -613,6 +613,19 @@ config CGROUP_MEM_RES_CTLR_SWAP
if boot option "noswapaccount" is set, swap will not be accounted.
Now, memory usage of swap_cgroup is 2 bytes per entry. If swap page
size is 4096bytes, 512k per 1Gbytes of swap.
+config CGROUP_MEM_RES_CTLR_SWAP_ENABLED
+ bool "Memory Resource Controller Swap Extension enabled by default"
+ depends on CGROUP_MEM_RES_CTLR_SWAP
+ default y
+ help
+ Memory Resource Controller Swap Extension comes with its price in
+ a bigger memory consumption. General purpose distribution kernels
+ which want to enable the feautre but keep it disabled by default
+ and let the user enable it by swapaccount boot command line
+ parameter should have this option unselected.
+ For those who want to have the feature enabled by default should
+ select this option (if, for some reason, they need to disable it
+ then noswapaccount does the trick).
menuconfig CGROUP_SCHED
bool "Group CPU scheduler"
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9a99cfa..bed98b6 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -61,7 +61,14 @@ struct mem_cgroup *root_mem_cgroup __read_mostly;
#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
/* Turned on only when memory cgroup is enabled && really_do_swap_account = 1 */
int do_swap_account __read_mostly;
-static int really_do_swap_account __initdata = 1; /* for remember boot option*/
+
+/* for remember boot option*/
+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED
+static int really_do_swap_account __initdata = 1;
+#else
+static int really_do_swap_account __initdata = 0;
+#endif
+
#else
#define do_swap_account (0)
#endif
@@ -4909,10 +4916,20 @@ struct cgroup_subsys mem_cgroup_subsys = {
};
#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
+static int __init enable_swap_account(char *s)
+{
+ /* consider enabled if no parameter or 1 is given */
+ if (!s || !strcmp(s, "1"))
+ really_do_swap_account = 1;
+ else if (!strcmp(s, "0"))
+ really_do_swap_account = 0;
+ return 1;
+}
+__setup("swapaccount", enable_swap_account);
static int __init disable_swap_account(char *s)
{
- really_do_swap_account = 0;
+ enable_swap_account("0");
return 1;
}
__setup("noswapaccount", disable_swap_account);
--
1.7.2.3
--
Michal Hocko
L3 team
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
On Thu, 18 Nov 2010 10:56:07 +0100
Michal Hocko <[email protected]> wrote:
> On Thu 18-11-10 17:53:34, KAMEZAWA Hiroyuki wrote:
> > On Thu, 18 Nov 2010 17:46:54 +0900
> > Daisuke Nishimura <[email protected]> wrote:
> >
> > > On Thu, 18 Nov 2010 09:23:32 +0100
> > > Michal Hocko <[email protected]> wrote:
> > >
> > > > On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> > > > > On Tue, 16 Nov 2010 17:12:25 -0800
> > > > > Andrew Morton <[email protected]> wrote:
> [...]
> > > > > > Yes, we're stuck with the old one now.
> > > > > >
> > > > > > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > > > > > Even if we didn't initially intend to add "nofoo".
> > > > > >
> > > > > I see.
> > > > >
> > > > > Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
> > > >
> > > > I have noticed that Andrew has already taken the last version of the
> > > > patch for -mm tree. Should I still rework it to change swapaccount to
> > > > swapaccount=0|1 resp. true|false?
> > > >
> > > It's usual to update a patch into more sophisticated one while it is in -mm tree.
> > > So, I think you'd better to do it(btw, I prefer 0|1 to true|false.
> > > Reading kernel-parameters.txt, 0|1 is more commonly used.).
> > >
> >
> > I vote for 0|1
>
> Changes since v3:
> * add 0|1 parameter values handling
>
> Changes since v2:
> * put the new parameter description to the proper (alphabetically
> * sorted)
> place in Documentation/kernel-parameters.txt
>
> Changes since v1:
> * do not remove noswapaccount parameter and add swapaccount parameter
> * instead
> * Documentation/kernel-parameters.txt updated)
>
I'm sorry again and again, but I think removing "noswapaccount" completely
would be better, as Andrew said first:
> So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> would have been better.
Thanks,
Daisuke Nishimura.
[I am removing stable from CC - to shield them off the discussion]
On Thu 18-11-10 19:14:27, Daisuke Nishimura wrote:
> On Thu, 18 Nov 2010 10:56:07 +0100
> Michal Hocko <[email protected]> wrote:
>
> > On Thu 18-11-10 17:53:34, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 18 Nov 2010 17:46:54 +0900
> > > Daisuke Nishimura <[email protected]> wrote:
> > >
> > > > On Thu, 18 Nov 2010 09:23:32 +0100
> > > > Michal Hocko <[email protected]> wrote:
> > > >
> > > > > On Wed 17-11-10 12:28:01, Daisuke Nishimura wrote:
> > > > > > On Tue, 16 Nov 2010 17:12:25 -0800
> > > > > > Andrew Morton <[email protected]> wrote:
> > [...]
> > > > > > > Yes, we're stuck with the old one now.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > >
> > > > > > > But we should note that "foo=[0|1]" is superior to "foo" and "nofoo".
> > > > > > > Even if we didn't initially intend to add "nofoo".
> > > > > > >
> > > > > > I see.
> > > > > >
> > > > > > Michal-san, could you update your patch to use "swapaccount=[1|0]" ?
> > > > >
> > > > > I have noticed that Andrew has already taken the last version of the
> > > > > patch for -mm tree. Should I still rework it to change swapaccount to
> > > > > swapaccount=0|1 resp. true|false?
> > > > >
> > > > It's usual to update a patch into more sophisticated one while it is in -mm tree.
> > > > So, I think you'd better to do it(btw, I prefer 0|1 to true|false.
> > > > Reading kernel-parameters.txt, 0|1 is more commonly used.).
> > > >
> > >
> > > I vote for 0|1
> >
> > Changes since v3:
> > * add 0|1 parameter values handling
> >
> > Changes since v2:
> > * put the new parameter description to the proper (alphabetically
> > * sorted)
> > place in Documentation/kernel-parameters.txt
> >
> > Changes since v1:
> > * do not remove noswapaccount parameter and add swapaccount parameter
> > * instead
> > * Documentation/kernel-parameters.txt updated)
> >
>
> I'm sorry again and again, but I think removing "noswapaccount" completely
> would be better, as Andrew said first:
I read the above Andrew's statement that we really should stick with the
old parameter.
> > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > would have been better.
>
>
> Thanks,
> Daisuke Nishimura.
--
Michal Hocko
L3 team
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
On Thu, Nov 18, 2010 at 09:21:32AM +0100, Michal Hocko wrote:
> > Sorry, I really don't want to start backporting features to stable
> > kernels if at all possible. Distros can pick them up on their own if
> > they determine it is needed.
>
> I really do agree with the part about features. But isn't this patch
> basically for distros (to help them to provide the swapaccounting feature
> without the cost of higher memory consumption in default configuration)?
Then if the distros want it, they can pick it up themselves.
> If this doesn't go to the stable then all (interested) of them would
> need to maintain the patch. Otherwise the change would come directly
> from the upstream.
They can cherry-pick from upstream like they do for everything else, no
real change here.
> Moreover, it is not a new feature it just consolidates the default
> behavior of the already existing functionality.
Again, it doesn't match up with the stable kernel rules, sorry, no, this
is not a bugfix or regression.
thanks,
greg k-h
On Thu, 18 Nov 2010 11:23:49 +0100 Michal Hocko <[email protected]> wrote:
> > I'm sorry again and again, but I think removing "noswapaccount" completely
> > would be better, as Andrew said first:
>
> I read the above Andrew's statement that we really should stick with the
> old parameter.
yup. We shouldn't remove the existing parameter, which people might be
using already.
> > > So we have swapaccount and noswapaccount. Ho hum, "swapaccount=[1|0]"
> > > would have been better.
What I meant was that it was a mistake to add the "noswapaccount" in
the first place. We should have made it "swapaccount=0", because that
would leave open the later option of reversing the default, and
enabling "swapaccount=1".
It also give us the option of adding "swapaccount=2"! Perhaps to
enable alternative swap accounting behaviour.