linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
To: Glauber Costa <glommer@parallels.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	cgroups@vger.kernel.org, devel@openvz.org,
	Michal Hocko <mhocko@suse.cz>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Lameter <cl@linux.com>,
	David Rientjes <rientjes@google.com>,
	Pekka Enberg <penberg@kernel.org>,
	Pekka Enberg <penberg@cs.helsinki.fi>
Subject: Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
Date: Sat, 11 Aug 2012 02:27:17 +0900	[thread overview]
Message-ID: <50254475.4000201@jp.fujitsu.com> (raw)
In-Reply-To: <1344517279-30646-7-git-send-email-glommer@parallels.com>

(2012/08/09 22:01), Glauber Costa wrote:
> This patch introduces infrastructure for tracking kernel memory pages to
> a given memcg. This will happen whenever the caller includes the flag
> __GFP_KMEMCG flag, and the task belong to a memcg other than the root.
> 
> In memcontrol.h those functions are wrapped in inline accessors.  The
> idea is to later on, patch those with static branches, so we don't incur
> any overhead when no mem cgroups with limited kmem are being used.
> 
> [ v2: improved comments and standardized function names ]
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> CC: Christoph Lameter <cl@linux.com>
> CC: Pekka Enberg <penberg@cs.helsinki.fi>
> CC: Michal Hocko <mhocko@suse.cz>
> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> CC: Johannes Weiner <hannes@cmpxchg.org>
> ---
>   include/linux/memcontrol.h |  79 +++++++++++++++++++
>   mm/memcontrol.c            | 185 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 264 insertions(+)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 8d9489f..75b247e 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -21,6 +21,7 @@
>   #define _LINUX_MEMCONTROL_H
>   #include <linux/cgroup.h>
>   #include <linux/vm_event_item.h>
> +#include <linux/hardirq.h>
>   
>   struct mem_cgroup;
>   struct page_cgroup;
> @@ -399,6 +400,11 @@ struct sock;
>   #ifdef CONFIG_MEMCG_KMEM
>   void sock_update_memcg(struct sock *sk);
>   void sock_release_memcg(struct sock *sk);
> +
> +#define memcg_kmem_on 1
> +bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order);
> +void __memcg_kmem_commit_page(struct page *page, void *handle, int order);
> +void __memcg_kmem_free_page(struct page *page, int order);
>   #else
>   static inline void sock_update_memcg(struct sock *sk)
>   {
> @@ -406,6 +412,79 @@ static inline void sock_update_memcg(struct sock *sk)
>   static inline void sock_release_memcg(struct sock *sk)
>   {
>   }
> +
> +#define memcg_kmem_on 0
> +static inline bool
> +__memcg_kmem_new_page(gfp_t gfp, void *handle, int order)
> +{
> +	return false;
> +}
> +
> +static inline void  __memcg_kmem_free_page(struct page *page, int order)
> +{
> +}
> +
> +static inline void
> +__memcg_kmem_commit_page(struct page *page, struct mem_cgroup *handle, int order)
> +{
> +}
>   #endif /* CONFIG_MEMCG_KMEM */
> +
> +/**
> + * memcg_kmem_new_page: verify if a new kmem allocation is allowed.
> + * @gfp: the gfp allocation flags.
> + * @handle: a pointer to the memcg this was charged against.
> + * @order: allocation order.
> + *
> + * returns true if the memcg where the current task belongs can hold this
> + * allocation.
> + *
> + * We return true automatically if this allocation is not to be accounted to
> + * any memcg.
> + */
> +static __always_inline bool
> +memcg_kmem_new_page(gfp_t gfp, void *handle, int order)
> +{
> +	if (!memcg_kmem_on)
> +		return true;
> +	if (!(gfp & __GFP_KMEMCG) || (gfp & __GFP_NOFAIL))
> +		return true;
> +	if (in_interrupt() || (!current->mm) || (current->flags & PF_KTHREAD))
> +		return true;
> +	return __memcg_kmem_new_page(gfp, handle, order);
> +}
> +
> +/**
> + * memcg_kmem_free_page: uncharge pages from memcg
> + * @page: pointer to struct page being freed
> + * @order: allocation order.
> + *
> + * there is no need to specify memcg here, since it is embedded in page_cgroup
> + */
> +static __always_inline void
> +memcg_kmem_free_page(struct page *page, int order)
> +{
> +	if (memcg_kmem_on)
> +		__memcg_kmem_free_page(page, order);
> +}
> +
> +/**
> + * memcg_kmem_commit_page: embeds correct memcg in a page
> + * @handle: a pointer to the memcg this was charged against.
> + * @page: pointer to struct page recently allocated
> + * @handle: the memcg structure we charged against
> + * @order: allocation order.
> + *
> + * Needs to be called after memcg_kmem_new_page, regardless of success or
> + * failure of the allocation. if @page is NULL, this function will revert the
> + * charges. Otherwise, it will commit the memcg given by @handle to the
> + * corresponding page_cgroup.
> + */
> +static __always_inline void
> +memcg_kmem_commit_page(struct page *page, struct mem_cgroup *handle, int order)
> +{
> +	if (memcg_kmem_on)
> +		__memcg_kmem_commit_page(page, handle, order);
> +}

Doesn't this 2 functions has no short-cuts ?

if (memcg_kmem_on && handle) ?

Maybe free() needs to access page_cgroup...



>   #endif /* _LINUX_MEMCONTROL_H */
>   
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 54e93de..e9824c1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -10,6 +10,10 @@
>    * Copyright (C) 2009 Nokia Corporation
>    * Author: Kirill A. Shutemov
>    *
> + * Kernel Memory Controller
> + * Copyright (C) 2012 Parallels Inc. and Google Inc.
> + * Authors: Glauber Costa and Suleiman Souhlal
> + *
>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License as published by
>    * the Free Software Foundation; either version 2 of the License, or
> @@ -434,6 +438,9 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *s)
>   #include <net/ip.h>
>   
>   static bool mem_cgroup_is_root(struct mem_cgroup *memcg);
> +static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, s64 delta);
> +static void memcg_uncharge_kmem(struct mem_cgroup *memcg, s64 delta);
> +
>   void sock_update_memcg(struct sock *sk)
>   {
>   	if (mem_cgroup_sockets_enabled) {
> @@ -488,6 +495,118 @@ struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
>   }
>   EXPORT_SYMBOL(tcp_proto_cgroup);
>   #endif /* CONFIG_INET */
> +
> +static inline bool memcg_kmem_enabled(struct mem_cgroup *memcg)
> +{
> +	return !mem_cgroup_disabled() && !mem_cgroup_is_root(memcg) &&
> +		memcg->kmem_accounted;
> +}
> +
> +/*
> + * We need to verify if the allocation against current->mm->owner's memcg is
> + * possible for the given order. But the page is not allocated yet, so we'll
> + * need a further commit step to do the final arrangements.
> + *
> + * It is possible for the task to switch cgroups in this mean time, so at
> + * commit time, we can't rely on task conversion any longer.  We'll then use
> + * the handle argument to return to the caller which cgroup we should commit
> + * against
> + *
> + * Returning true means the allocation is possible.
> + */
> +bool __memcg_kmem_new_page(gfp_t gfp, void *_handle, int order)
> +{
> +	struct mem_cgroup *memcg;
> +	struct mem_cgroup **handle = (struct mem_cgroup **)_handle;
> +	bool ret = true;
> +	size_t size;
> +	struct task_struct *p;
> +
> +	*handle = NULL;
> +	rcu_read_lock();
> +	p = rcu_dereference(current->mm->owner);
> +	memcg = mem_cgroup_from_task(p);
> +	if (!memcg_kmem_enabled(memcg))
> +		goto out;
> +
> +	mem_cgroup_get(memcg);
> +

This mem_cgroup_get() will be a potentioal performance problem.
Don't you have good idea to avoid accessing atomic counter here ?
I think some kind of percpu counter or a feature to disable "move task"
will be a help.


> +	size = PAGE_SIZE << order;
> +	ret = memcg_charge_kmem(memcg, gfp, size) == 0;
> +	if (!ret) {
> +		mem_cgroup_put(memcg);
> +		goto out;
> +	}
> +
> +	*handle = memcg;
> +out:
> +	rcu_read_unlock();
> +	return ret;
> +}
> +EXPORT_SYMBOL(__memcg_kmem_new_page);
> +
> +void __memcg_kmem_commit_page(struct page *page, void *handle, int order)
> +{
> +	struct page_cgroup *pc;
> +	struct mem_cgroup *memcg = handle;
> +
> +	if (!memcg)
> +		return;
> +
> +	WARN_ON(mem_cgroup_is_root(memcg));
> +	/* The page allocation must have failed. Revert */
> +	if (!page) {
> +		size_t size = PAGE_SIZE << order;
> +
> +		memcg_uncharge_kmem(memcg, size);
> +		mem_cgroup_put(memcg);
> +		return;
> +	}
> +
> +	pc = lookup_page_cgroup(page);
> +	lock_page_cgroup(pc);
> +	pc->mem_cgroup = memcg;
> +	SetPageCgroupUsed(pc);
> +	unlock_page_cgroup(pc);
> +}
> +
> +void __memcg_kmem_free_page(struct page *page, int order)
> +{
> +	struct mem_cgroup *memcg;
> +	size_t size;
> +	struct page_cgroup *pc;
> +
> +	if (mem_cgroup_disabled())
> +		return;
> +
> +	pc = lookup_page_cgroup(page);
> +	lock_page_cgroup(pc);
> +	memcg = pc->mem_cgroup;
> +	pc->mem_cgroup = NULL;

shouldn't this happen after checking "Used" bit ?
Ah, BTW, why do you need to clear pc->memcg ?

> +	if (!PageCgroupUsed(pc)) {
> +		unlock_page_cgroup(pc);
> +		return;
> +	}
> +	ClearPageCgroupUsed(pc);
> +	unlock_page_cgroup(pc);
> +
> +	/*
> +	 * Checking if kmem accounted is enabled won't work for uncharge, since
> +	 * it is possible that the user enabled kmem tracking, allocated, and
> +	 * then disabled it again.
> +	 *
> +	 * We trust if there is a memcg associated with the page, it is a valid
> +	 * allocation
> +	 */
> +	if (!memcg)
> +		return;
> +
> +	WARN_ON(mem_cgroup_is_root(memcg));
> +	size = (1 << order) << PAGE_SHIFT;
> +	memcg_uncharge_kmem(memcg, size);
> +	mem_cgroup_put(memcg);

Why do we need ref-counting here ? kmem res_counter cannot work as
reference ?


> +}
> +EXPORT_SYMBOL(__memcg_kmem_free_page);
>   #endif /* CONFIG_MEMCG_KMEM */
>   
>   #if defined(CONFIG_INET) && defined(CONFIG_MEMCG_KMEM)
> @@ -5759,3 +5878,69 @@ static int __init enable_swap_account(char *s)
>   __setup("swapaccount=", enable_swap_account);
>   
>   #endif
> +
> +#ifdef CONFIG_MEMCG_KMEM
> +int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, s64 delta)
> +{

What does 'delta' means ?


> +	struct res_counter *fail_res;
> +	struct mem_cgroup *_memcg;
> +	int ret;
> +	bool may_oom;
> +	bool nofail = false;
> +
> +	may_oom = (gfp & __GFP_WAIT) && (gfp & __GFP_FS) &&
> +	    !(gfp & __GFP_NORETRY);
> +
> +	ret = 0;
> +
> +	if (!memcg)
> +		return ret;
> +
> +	_memcg = memcg;
> +	ret = __mem_cgroup_try_charge(NULL, gfp, delta / PAGE_SIZE,
> +	    &_memcg, may_oom);
> +
> +	if (ret == -EINTR)  {
> +		nofail = true;
> +		/*
> +		 * __mem_cgroup_try_charge() chosed to bypass to root due to
> +		 * OOM kill or fatal signal.  Since our only options are to
> +		 * either fail the allocation or charge it to this cgroup, do
> +		 * it as a temporary condition. But we can't fail. From a
> +		 * kmem/slab perspective, the cache has already been selected,
> +		 * by mem_cgroup_get_kmem_cache(), so it is too late to change
> +		 * our minds
> +		 */
> +		res_counter_charge_nofail(&memcg->res, delta, &fail_res);
> +		if (do_swap_account)
> +			res_counter_charge_nofail(&memcg->memsw, delta,
> +						  &fail_res);
> +		ret = 0;

Hm, you returns 0 and this charge may never be uncharged....right ?

Thanks,
-Kame



  reply	other threads:[~2012-08-10 17:27 UTC|newest]

Thread overview: 135+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-09 13:01 [PATCH v2 00/11] Request for Inclusion: kmem controller for memcg Glauber Costa
2012-08-09 13:01 ` [PATCH v2 01/11] memcg: Make it possible to use the stock for more than one page Glauber Costa
2012-08-10 15:12   ` Michal Hocko
2012-08-09 13:01 ` [PATCH v2 02/11] memcg: Reclaim when more than one page needed Glauber Costa
2012-08-10 15:42   ` Michal Hocko
2012-08-10 16:49     ` Kamezawa Hiroyuki
2012-08-10 17:28       ` Michal Hocko
2012-08-10 17:56         ` Kamezawa Hiroyuki
2012-08-10 17:30   ` Michal Hocko
2012-08-10 18:52     ` Michal Hocko
2012-08-10 18:54   ` Michal Hocko
2012-08-13  8:05     ` Glauber Costa
2012-08-13 13:10       ` Michal Hocko
2012-08-09 13:01 ` [PATCH v2 03/11] memcg: change defines to an enum Glauber Costa
2012-08-10 15:43   ` Michal Hocko
2012-08-09 13:01 ` [PATCH v2 04/11] kmem accounting basic infrastructure Glauber Costa
2012-08-10 17:02   ` Kamezawa Hiroyuki
2012-08-13  8:36     ` Glauber Costa
2012-08-17  2:38       ` Kamezawa Hiroyuki
2012-08-14 16:21   ` Michal Hocko
2012-08-15  9:33     ` Glauber Costa
2012-08-15 11:12       ` James Bottomley
2012-08-15 12:55         ` Michal Hocko
2012-08-15 13:29           ` James Bottomley
2012-08-15 12:39       ` Michal Hocko
2012-08-15 12:53         ` Glauber Costa
2012-08-15 13:02           ` Michal Hocko
2012-08-15 13:04             ` Glauber Costa
2012-08-15 13:26               ` Michal Hocko
2012-08-15 13:31                 ` Glauber Costa
2012-08-15 14:10                   ` Michal Hocko
2012-08-15 14:11                     ` Glauber Costa
2012-08-15 14:47         ` Christoph Lameter
2012-08-15 15:11           ` Glauber Costa
2012-08-15 15:34             ` Christoph Lameter
2012-08-15 15:35               ` Glauber Costa
2012-08-15 17:26                 ` Christoph Lameter
2012-08-15 18:11               ` Ying Han
2012-08-15 18:25                 ` Christoph Lameter
2012-08-15 19:22                   ` Glauber Costa
2012-08-15 18:07             ` Ying Han
2012-08-15 15:19           ` Greg Thelen
2012-08-15 15:36             ` Christoph Lameter
2012-08-15 18:01         ` Ying Han
2012-08-15 18:00           ` Glauber Costa
2012-08-15 19:50     ` Ying Han
2012-08-16 15:25       ` Michal Hocko
2012-08-17  5:58         ` Ying Han
2012-08-09 13:01 ` [PATCH v2 05/11] Add a __GFP_KMEMCG flag Glauber Costa
2012-08-10 17:07   ` Kamezawa Hiroyuki
2012-08-09 13:01 ` [PATCH v2 06/11] memcg: kmem controller infrastructure Glauber Costa
2012-08-10 17:27   ` Kamezawa Hiroyuki [this message]
2012-08-13  8:28     ` Glauber Costa
2012-08-14 18:58       ` Greg Thelen
2012-08-15  9:18         ` Glauber Costa
2012-08-15 16:38           ` Greg Thelen
2012-08-15 17:00             ` Glauber Costa
2012-08-15 17:12               ` Greg Thelen
2012-08-15 19:31                 ` Glauber Costa
2012-08-16  3:37                   ` Greg Thelen
2012-08-16  7:47                     ` Glauber Costa
2012-08-20 13:36               ` Kamezawa Hiroyuki
2012-08-20 15:29                 ` Glauber Costa
2012-08-17  2:36       ` Kamezawa Hiroyuki
2012-08-17  7:04         ` Glauber Costa
2012-08-14 11:00     ` Glauber Costa
2012-08-11  5:11   ` Greg Thelen
2012-08-13  8:07     ` Glauber Costa
2012-08-13  9:59     ` Glauber Costa
2012-08-13 21:21       ` Greg Thelen
2012-08-14 17:25   ` Michal Hocko
2012-08-15  9:42     ` Glauber Costa
2012-08-15 10:44       ` Glauber Costa
2012-08-15 13:09       ` Michal Hocko
2012-08-15 14:01         ` Glauber Costa
2012-08-15 14:23           ` Michal Hocko
2012-08-15 14:27             ` Glauber Costa
2012-08-16  9:53               ` Michal Hocko
2012-08-16  9:57                 ` Glauber Costa
2012-08-16 15:05                   ` Michal Hocko
2012-08-16 15:22                     ` Glauber Costa
2012-08-21 21:50   ` Greg Thelen
2012-08-22  8:35     ` Glauber Costa
2012-08-23  0:07       ` Greg Thelen
2012-08-23  7:51         ` Glauber Costa
2012-08-09 13:01 ` [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg Glauber Costa
2012-08-09 16:33   ` Greg Thelen
2012-08-09 16:42     ` Glauber Costa
2012-08-10 17:33   ` Kamezawa Hiroyuki
2012-08-13  8:03     ` Glauber Costa
2012-08-13  8:57       ` Mel Gorman
2012-08-10 17:36   ` Greg Thelen
2012-08-13  8:02     ` Glauber Costa
2012-08-14 15:16   ` Mel Gorman
2012-08-15  9:08     ` Glauber Costa
2012-08-15 13:22       ` Mel Gorman
2012-08-15 13:39         ` Glauber Costa
2012-08-15 13:51         ` Glauber Costa
2012-08-15  9:24   ` Michal Hocko
2012-08-09 13:01 ` [PATCH v2 08/11] memcg: disable kmem code when not in use Glauber Costa
2012-08-17  7:02   ` Michal Hocko
2012-08-17  7:01     ` Glauber Costa
2012-08-17  8:04       ` Michal Hocko
2012-08-09 13:01 ` [PATCH v2 09/11] memcg: propagate kmem limiting information to children Glauber Costa
2012-08-10 17:51   ` Kamezawa Hiroyuki
2012-08-13  8:01     ` Glauber Costa
2012-08-17  9:00   ` Michal Hocko
2012-08-17  9:15     ` Glauber Costa
2012-08-17  9:35       ` Michal Hocko
2012-08-17 10:07         ` Glauber Costa
2012-08-17 10:35           ` Michal Hocko
2012-08-17 10:36             ` Glauber Costa
2012-08-21  7:54               ` Michal Hocko
2012-08-21  8:35                 ` Michal Hocko
2012-08-21  9:17                   ` Glauber Costa
2012-08-21  9:22                 ` Glauber Costa
2012-08-21 10:00                   ` Michal Hocko
2012-08-21 10:01                     ` Glauber Costa
2012-08-22  1:09                     ` Greg Thelen
2012-08-22  8:22                       ` Glauber Costa
2012-08-22 23:23                         ` Greg Thelen
2012-08-23  7:55                           ` Glauber Costa
2012-08-24  5:06                             ` Greg Thelen
2012-08-24  5:23                               ` Glauber Costa
2012-08-17 10:39             ` Glauber Costa
2012-08-09 13:01 ` [PATCH v2 10/11] memcg: allow a memcg with kmem charges to be destructed Glauber Costa
2012-08-21  8:22   ` Michal Hocko
2012-08-22  8:36     ` Glauber Costa
2012-08-09 13:01 ` [PATCH v2 11/11] protect architectures where THREAD_SIZE >= PAGE_SIZE against fork bombs Glauber Costa
2012-08-10 17:54   ` Kamezawa Hiroyuki
2012-08-21  9:35   ` Michal Hocko
2012-08-21  9:40     ` Glauber Costa
2012-08-21 10:57       ` Michal Hocko
2012-08-17 21:37 ` [PATCH v2 00/11] Request for Inclusion: kmem controller for memcg Ying Han
2012-08-20  7:51   ` Glauber Costa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50254475.4000201@jp.fujitsu.com \
    --to=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=cl@linux.com \
    --cc=devel@openvz.org \
    --cc=glommer@parallels.com \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.cz \
    --cc=penberg@cs.helsinki.fi \
    --cc=penberg@kernel.org \
    --cc=rientjes@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).