Re: [RFC PATCH] Make swap accounting default behavior configurable v2

From: Michal Hocko
Date: Wednesday, November 10, 2010 - 5:51 am

Hi,
could you consider the patch bellow? It basically changes the default
swap accounting behavior (when it is turned on in configuration) to be
configurable as well. 

The rationale is described in the patch but in short it makes it much
more easier to enable this feature in distribution kernels as the
functionality can be provided in the general purpose kernel (with the
option disabled) without any drawbacks and interested users can enable
it. This is not possible currently.

I am aware that boot command line parameter name change is not ideal but
the original semantic wasn't good enough and I don't like
noswapaccount=yes|no very much. 

If we really have to stick to it I can rework the patch to keep the name
and just add the yes|no logic, though. Or we can keep the original one
and add swapaccount paramete which would mean the oposite as the other
one.

The patch is based on the current Linus tree.

Any thoughts?
---

From c874f2c1ff1493e49611f19308434e564c2d37c6 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
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) and changes the original
noswapaccount parameter to swapaccount=true|false which provides
a more fine grained way to control this feature.

The default behavior is unchanged (if CONFIG_CGROUP_MEM_RES_CTLR_SWAP is
enabled then CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED is enabled ...
From: Daisuke Nishimura
Date: Wednesday, November 10, 2010 - 5:46 pm

On Wed, 10 Nov 2010 13:51:54 +0100
hmm, I agree that current parameter name(noswapaccount) is not desirable
for yes|no, but IMHO changing the user interface(iow, making what worked before 
unusable) is worse.

Although I'm not sure how many people are using this parameter, I vote for
using "noswapaccount[=(yes|no)]".
And you should update Documentation/kernel-parameters.txt too.

Thanks,
--

From: Michal Hocko
Date: Thursday, November 11, 2010 - 2:31 am

Isn't a new swapaccount parameter better than that? I know we don't want
to have too many parameters but having a something with a clear meaning

Yes, I am aware of that and will do that once there is an agreement on
the patch itself. At this stage, I just wanted to have a feadback about

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

From: Daisuke Nishimura
Date: Thursday, November 11, 2010 - 5:41 pm

On Thu, 11 Nov 2010 10:31:55 +0100
I'll ack your patch when it's been released with documentation update.

Thanks,
Daisuke Nishimura.
--

From: Michal Hocko
Date: Friday, November 12, 2010 - 1:31 am

Changes since v1:
* do not remove noswapaccount parameter and add swapaccount parameter
  instead
* Documentation/kernel-parameters.txt updated

--- 
From c60e3d76702cd9b4b5b13c01be335da31d03bc1c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
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 <mhocko@suse.cz>
---
 Documentation/kernel-parameters.txt |    2 ++
 init/Kconfig                        |   13 +++++++++++++
 mm/memcontrol.c                     |   15 ++++++++++++++-
 3 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ed45e98..7077148 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1752,6 +1752,8 @@ and is between 256 and 4096 characters. It is defined in the file
 ...
From: Daisuke Nishimura
Date: Sunday, November 14, 2010 - 6:13 pm

On Fri, 12 Nov 2010 09:31:03 +0100
(I've add Andrew and Balbir to CC-list.)
It seems that almost all parameters are listed in alphabetic order in the document,
so I think it would be better to obey the rule.

Thanks,
--

From: Balbir Singh
Date: Sunday, November 14, 2010 - 7:03 pm

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2010-11-15 10:13:35]:

Thanks Nishimura-San

It seems like the motivation for the patch is to allow distros to
enable memory cgroups and swap control, but to have swap control
turned off by default (because we provide default on today)
 - is my understanding correct?

-- 
	Three Cheers,
	Balbir
--

From: Daisuke Nishimura
Date: Sunday, November 14, 2010 - 7:31 pm

On Mon, 15 Nov 2010 07:33:30 +0530
I think you're right.

This patch make the default behavior configurable by .config and let users
turn swap control on even if it's disabled by .config.
It will give distros and users wider choice.

Thanks,
Daisuke Nishimura.
--

From: Michal Hocko
Date: Monday, November 15, 2010 - 1:35 am

You are right. The header of the file says:

" The following is a consolidated list of the kernel parameters as
implemented (mostly) by the __setup() macro and sorted into English
Dictionary order (defined as ignoring all punctuation and sorting digits
before letters in a case insensitive manner), and with descriptions
where known."


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 21df3801e2b65f47a2807534487ebb353dad6340 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
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 <mhocko@suse.cz>
---
 ...
From: Daisuke Nishimura
Date: Monday, November 15, 2010 - 9:48 pm

Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

Thank you for your work.
Daisuke Nishimura.

On Mon, 15 Nov 2010 09:35:40 +0100
--

From: Michal Hocko
Date: Tuesday, November 16, 2010 - 1:15 am

Thank you. What should be next steps? Waiting for other ACKs or push it
through Andrew?
Btw. is this a stable tree material? I guess that distribution would
like to have this patch and the stable is the easiest way how to deliver

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

From: Daisuke Nishimura
Date: Tuesday, November 16, 2010 - 3:03 am

On Tue, 16 Nov 2010 09:15:44 +0100
I recommend you to resend the patch removing "RFC" in a clean patch format
It's not stable material. This is not a BUG or anything that meets the conditions
described in Documentation/stable_kernel_rules.txt.


Thanks,
Daisuke Nishimura.
--