[PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Michal Hocko]

CONFIG_MEMCG might be currently enabled also for !MMU architectures
which was probably an omission because Balbir had this on the TODO
list section (https://lkml.org/lkml/2008/3/16/59)
"
Only when CONFIG_MMU is enabled, is the virtual address space control
enabled. Should we do this for nommu cases as well? My suspicion is
that we don't have to.
"
I do not see any traces for !MMU requests after then. The code compiles
with !MMU but I haven't heard about anybody using it in the real life
so it is not clear to me whether it works and it is usable at all
considering how !MMU configuration is restricted.

Let's make CONFIG_MEMCG depend on CONFIG_MMU to make our support
explicit and also to get rid of few ifdefs in the code base.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
Hi Andrew,
this came out as a result from
http://marc.info/?l=linux-mm&m=142533566331935 which tries to fix a
compilation warning when CONFIG_MMU=y && CONFIG_MEMCG.  I think it
makes more sense to get rid of CONFIG_MMU ifdefs and make MEMCG depend
on MMU. I am convinced that MEMCG is basically unusable with !MMU and
it should have depended on MMU since very beginning. Let's do it now.
We can always revert should there be a reasonable usecase later. That
would require some additional changes, though (e.g. anon pages should be
accounted and who knows what else).

 Documentation/cgroups/memory.txt |  2 --
 init/Kconfig                     |  1 +
 mm/memcontrol.c                  | 24 ------------------------
 3 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index a22df3ad35ff..9111540657d6 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -667,8 +667,6 @@ NOTE2: It is recommended to set the soft limit always below the hard limit,
 
 Users can move charges associated with a task along with task migration, that
 is, uncharge task's pages from the old cgroup and charge them to the new cgroup.
-This feature is not supported in !CONFIG_MMU environments because of lack of
-page tables.
 
 8.1 Interface
 
diff --git a/init/Kconfig b/init/Kconfig
index 9afb971497f4..b16f77fbd050 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -979,6 +979,7 @@ config MEMCG
 	bool "Memory Resource Controller for Control Groups"
 	select PAGE_COUNTER
 	select EVENTFD
+	depends on MMU
 	help
 	  Provides a memory resource controller that manages both anonymous
 	  memory and page cache. (See Documentation/cgroups/memory.txt)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0c86945bcc9a..1c4bb9c6227e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3467,7 +3467,6 @@ static u64 mem_cgroup_move_charge_read(struct cgroup_subsys_state *css,
 	return mem_cgroup_from_css(css)->move_charge_at_immigrate;
 }
 
-#ifdef CONFIG_MMU
 static int mem_cgroup_move_charge_write(struct cgroup_subsys_state *css,
 					struct cftype *cft, u64 val)
 {
@@ -3485,13 +3484,6 @@ static int mem_cgroup_move_charge_write(struct cgroup_subsys_state *css,
 	memcg->move_charge_at_immigrate = val;
 	return 0;
 }
-#else
-static int mem_cgroup_move_charge_write(struct cgroup_subsys_state *css,
-					struct cftype *cft, u64 val)
-{
-	return -ENOSYS;
-}
-#endif
 
 #ifdef CONFIG_NUMA
 static int memcg_numa_stat_show(struct seq_file *m, void *v)
@@ -4676,7 +4668,6 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 	memcg->soft_limit = PAGE_COUNTER_MAX;
 }
 
-#ifdef CONFIG_MMU
 /* Handlers for move charge at task migration. */
 static int mem_cgroup_do_precharge(unsigned long count)
 {
@@ -5209,21 +5200,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys_state *css,
 	if (mc.to)
 		mem_cgroup_clear_mc();
 }
-#else	/* !CONFIG_MMU */
-static int mem_cgroup_can_attach(struct cgroup_subsys_state *css,
-				 struct cgroup_taskset *tset)
-{
-	return 0;
-}
-static void mem_cgroup_cancel_attach(struct cgroup_subsys_state *css,
-				     struct cgroup_taskset *tset)
-{
-}
-static void mem_cgroup_move_task(struct cgroup_subsys_state *css,
-				 struct cgroup_taskset *tset)
-{
-}
-#endif
 
 /*
  * Cgroup retains root cgroups across [un]mount cycles making it necessary
-- 
2.1.4

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Johannes Weiner]

On Wed, Mar 04, 2015 at 07:07:08PM +0100, Michal Hocko wrote:
> CONFIG_MEMCG might be currently enabled also for !MMU architectures
> which was probably an omission because Balbir had this on the TODO
> list section (https://lkml.org/lkml/2008/3/16/59)
> "
> Only when CONFIG_MMU is enabled, is the virtual address space control
> enabled. Should we do this for nommu cases as well? My suspicion is
> that we don't have to.
> "
> I do not see any traces for !MMU requests after then. The code compiles
> with !MMU but I haven't heard about anybody using it in the real life
> so it is not clear to me whether it works and it is usable at all
> considering how !MMU configuration is restricted.
> 
> Let's make CONFIG_MEMCG depend on CONFIG_MMU to make our support
> explicit and also to get rid of few ifdefs in the code base.
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Sorry about the misunderstanding, I actually acked Chen's patch.  As I
said, there is nothing inherent in memcg that would prevent using it
on NOMMU systems except for this charges-follow-tasks feature, so I'd
rather fix the compiler warning than adding this dependency.

Thanks

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Michal Hocko]

[Forgot to CC Balbir - sorry - the thread starts here:
http://marc.info/?l=linux-mm&m=142549243725564&w=2]

On Wed 04-03-15 14:06:35, Johannes Weiner wrote:
> On Wed, Mar 04, 2015 at 07:07:08PM +0100, Michal Hocko wrote:
> > CONFIG_MEMCG might be currently enabled also for !MMU architectures
> > which was probably an omission because Balbir had this on the TODO
> > list section (https://lkml.org/lkml/2008/3/16/59)
> > "
> > Only when CONFIG_MMU is enabled, is the virtual address space control
> > enabled. Should we do this for nommu cases as well? My suspicion is
> > that we don't have to.
> > "
> > I do not see any traces for !MMU requests after then. The code compiles
> > with !MMU but I haven't heard about anybody using it in the real life
> > so it is not clear to me whether it works and it is usable at all
> > considering how !MMU configuration is restricted.
> > 
> > Let's make CONFIG_MEMCG depend on CONFIG_MMU to make our support
> > explicit and also to get rid of few ifdefs in the code base.
> > 
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Sorry about the misunderstanding, I actually acked Chen's patch.  As I
> said, there is nothing inherent in memcg that would prevent using it
> on NOMMU systems except for this charges-follow-tasks feature, so I'd
> rather fix the compiler warning than adding this dependency.

Does it really make sense to do this minor tweaks when the configuration
is barely usable and we are not aware of anybody actually using it in
the real life?

Sure there is nothing inherently depending on MMU but just considering
this wasn't working since ages for anon mappings and who knows what else
doesn't work.

The point is, once somebody really needs this configuration we should go
over all the missing parts and implement them but this half baked state
with random fixes to shut the compiler up is really suboptimal IMO.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[David Rientjes]

On Wed, 4 Mar 2015, Michal Hocko wrote:

> Does it really make sense to do this minor tweaks when the configuration
> is barely usable and we are not aware of anybody actually using it in
> the real life?
> 

If the memcg kmem extension continues to be improved, I'm wondering if 
anybody would want to use memcg only for kmem limiting.

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Johannes Weiner]

On Wed, Mar 04, 2015 at 08:28:36PM +0100, Michal Hocko wrote:
> > Sorry about the misunderstanding, I actually acked Chen's patch.  As I
> > said, there is nothing inherent in memcg that would prevent using it
> > on NOMMU systems except for this charges-follow-tasks feature, so I'd
> > rather fix the compiler warning than adding this dependency.
> 
> Does it really make sense to do this minor tweaks when the configuration
> is barely usable and we are not aware of anybody actually using it in
> the real life?
> 
> Sure there is nothing inherently depending on MMU

How is this even controversial?  We are not adding dependencies just
because we're not sure how we feel about the opposite.  We declare a
dependency when we know it truly exists.

> but just considering
> this wasn't working since ages for anon mappings and who knows what else
> doesn't work.

NOMMU people know that too, they don't expect to have significant test
coverage.  If they run into issues, they can still add the dependency.
This is much better than them wanting to use a feature, running into
the dependency declaration, going through all the code, scratching
their heads about why this code would have that dependency, finally
writing us an email, and then us going "ah yeah, there is nothing
INHERENTLY depending on MMU, we just weren't sure about it."

I don't even care about NOMMU, this is just wrong on principle.  And
obviously so.  NAK to your patch from me.

> The point is, once somebody really needs this configuration we should go
> over all the missing parts and implement them but this half baked state
> with random fixes to shut the compiler up is really suboptimal IMO.

Disagreed, for the above-mentioned reasons.  Chen's patch is obvious
and self-contained and doesn't at all indicate an endless stream of
future patches in that direction.  It also improves code organization.

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Andrew Morton]

On Wed, 4 Mar 2015 16:13:01 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:

> I don't even care about NOMMU, this is just wrong on principle.

Agree.  And I do care about nommu ;)

If some nommu person wants to start using memcg and manages to get it
doing something useful then good for them - we end up with a better
kernel.  We shouldn't go and rule this out without having even tried it.

Re: [PATCH] memcg: make CONFIG_MEMCG depend on CONFIG_MMU

[Michal Hocko]

On Wed 04-03-15 13:21:26, Andrew Morton wrote:
> On Wed, 4 Mar 2015 16:13:01 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > I don't even care about NOMMU, this is just wrong on principle.
> 
> Agree.  And I do care about nommu ;)
> 
> If some nommu person wants to start using memcg and manages to get it
> doing something useful then good for them - we end up with a better
> kernel.  We shouldn't go and rule this out without having even tried it.

Fair enough, but shouldn't we be explicit (and honest) that the
configuration is currently broken and hardly usable?

Would it make sense to make MEMCG depend on BROKEN for !MMU? If somebody
really has an usecase then dependency on BROKEN would suggest there is a
work to be done before it is enabled for his/her configuration. I would
expect such a user would send us an email when noticing this and submit
a bug report so that we can help making it work.
---
>From 3707f445ebd21d10c076f0cb2446a0732cf6c3bb Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Wed, 4 Mar 2015 10:48:47 +0100
Subject: [PATCH] memcg: mark CONFIG_MEMCG broken for !CONFIG_MMU

CONFIG_MEMCG might be currently enabled also for !MMU architectures
which was probably an omission because Balbir had this on the TODO
list section (https://lkml.org/lkml/2008/3/16/59)
"
Only when CONFIG_MMU is enabled, is the virtual address space control
enabled. Should we do this for nommu cases as well? My suspicion is
that we don't have to.
"
I do not see any traces for !MMU requests after then. The code compiles
with !MMU but I haven't heard about anybody using it in the real life
so it is not clear to me whether it works and it is usable at all. At
least anonymous mmaps do not try to charge the memory and who knows what
else is broken.

Let's make CONFIG_MEMCG depend on BROKEN for !CONFIG_MMU to make the
current status explicit for somebody who might be interested in using
MEMCG and report it to us so that we can help with fixing it up.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 init/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/init/Kconfig b/init/Kconfig
index 9afb971497f4..f5373c4188c0 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -979,6 +979,7 @@ config MEMCG
 	bool "Memory Resource Controller for Control Groups"
 	select PAGE_COUNTER
 	select EVENTFD
+	depends on MMU || BROKEN
 	help
 	  Provides a memory resource controller that manages both anonymous
 	  memory and page cache. (See Documentation/cgroups/memory.txt)
-- 
2.1.4

-- 
Michal Hocko
SUSE Labs