linux-kernel - Re: -mm merge plans for 2.6.26 (memcgroup)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0804211158001.10521@blonde.site>
Date:	Mon, 21 Apr 2008 12:22:26 +0100 (BST)
From:	Hugh Dickins <hugh@...itas.com>
To:	Balbir Singh <balbir@...ux.vnet.ibm.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Andi Kleen <andi@...stfloor.org>, menage@...gle.com,
	xemul@...nvz.org, shiwh@...fujitsu.com, mel@....ul.ie,
	linux-kernel@...r.kernel.org
Subject: Re: -mm merge plans for 2.6.26 (memcgroup)

On Mon, 21 Apr 2008, Balbir Singh wrote:
> * Balbir Singh <balbir@...ux.vnet.ibm.com> [2008-04-21 12:14:28]:
> > Andrew Morton wrote:
> > >> On Mon, 21 Apr 2008 09:30:59 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:
> > >> On Mon, 21 Apr 2008 00:51:30 +0100 (BST)
> > >> Hugh Dickins <hugh@...itas.com> wrote:
> > >>>>  disable-the-memory-controller-by-default-v3.patch
> > >>>>  disable-the-memory-controller-by-default-v3-fix.patch
> > >>> If those are to go in, then the sooner the better, yes.
> > >>>
> > >>> But though I argued for cgroup_disable=memory (or some such),
> > >>> I think myself that taking it even further now (requiring an
> > >>> additional cgroup_enable=memory at boottime to get the memcg
> > >>> stuff you chose with CGROUP_MEM_RES_CTLR=y at build time) is
> > >>> confusing overkill, just messing around.
> > > 
> > > Yes, it does sound a bit silly.  I'd say just enable it, and provide a
> > > cgroup_disable.

And 2.6.25 does already have that cgroup_disable=memory bootoption to
disable the config-enabled MEM_RES_CTLR and almost all of its overhead
(just leaving the extra pointer in struct page).

> OK, fair enough. Andi Kleen spoke about the overhead and how distros would
> be impacted if they enabled CONFIG_MEM_RES_CTLR and it was not disabled by
> default. I think the enable/disable is good. I just need to turn on/off
> mem_cgroup_subsys.disabled. The patch is as simple as
>  
> Signed-off-by: Balbir Singh <balbir@...ux.vnet.ibm.com>

That's a simple patch on top of:
disable-the-memory-controller-by-default-v3.patch
disable-the-memory-controller-by-default-v3-fix.patch

But even simpler is just to drop those patches, yes?
What's the point of adding cgroup_enable if nothing uses it?
And especially not for going into -stable.

I've added Andi to the Cc's, I don't want to sneak away something
he's expecting there.  If 2.6.25 had gone out with a convention that
controller infrastructure gets compiled in, but has then to be enabled
at boottime or runtime, okay; but switching back and forth between
defaults of enabled and disabled between releases (and between
controllers, as you seem to envisage) bothers me.

The config helptext (along with some misinformation) does conclude
	  Only enable when you're ok with these trade offs and really
	  sure you need the memory resource controller.
so I don't see why we now need to switch to disabled by default both
in config and at bootime - too many hurdles to my mind.

Hugh

> ---
> 
>  mm/memcontrol.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff -puN mm/memcontrol.c~memcg-dont-disable-by-default mm/memcontrol.c
> --- linux-2.6.25/mm/memcontrol.c~memcg-dont-disable-by-default	2008-04-21 12:11:31.000000000 +0530
> +++ linux-2.6.25-balbir/mm/memcontrol.c	2008-04-21 12:11:40.000000000 +0530
> @@ -1108,5 +1108,4 @@ struct cgroup_subsys mem_cgroup_subsys =
>  	.populate = mem_cgroup_populate,
>  	.attach = mem_cgroup_move_task,
>  	.early_init = 0,
> -	.disabled = 1,
>  };
> _
> 
> This would enable the memory controller by default, it can be disabled
> using cgroup_disable=memory at boot time.
>  
>  
> > >>> Others think differently.  A compromise would be to improve the
> > >>> helptext for CGROUP_MEM_RES_CTLR (some of it is presently nonsense,
> > >>> isn't it?  Certainly there's a significant overhead, but it's the
> > >>> 32-bit struct page not the 64-bit which then suffers from crossing
> > >>> cacheline boundaries).  Not much point in mentioning
> > >>> cgroup_disable=memory if those patches go in, but needs to say
> > >>> cgroup_enable=memory bootoption also needed.
> > >>>
> > >> My concern around this is "default" action of cgroups may be different
> > >> from each otther. It's confusing...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/