From: Vladimir Davydov <vdavydov@parallels.com>
To: Tejun Heo <tj@kernel.org>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, mhocko@kernel.org,
cgroups@vger.kernel.org, linux-mm@kvack.org, kernel-team@fb.com
Subject: Re: [PATCH 2/3] memcg: ratify and consolidate over-charge handling
Date: Mon, 14 Sep 2015 15:44:20 +0300 [thread overview]
Message-ID: <20150914124420.GE30743@esperanza> (raw)
In-Reply-To: <20150913201442.GD25369@htj.duckdns.org>
On Sun, Sep 13, 2015 at 04:14:42PM -0400, Tejun Heo wrote:
> try_charge() is the main charging logic of memcg. When it hits the
> limit but either can't fail the allocation due to __GFP_NOFAIL or the
> task is likely to free memory very soon, being OOM killed, has SIGKILL
> pending or exiting, it "bypasses" the charge to the root memcg and
> returns -EINTR. While this is one approach which can be taken for
> these situations, it has several issues.
>
> * It unnecessarily lies about the reality. The number itself doesn't
> go over the limit but the actual usage does. memcg is either forced
> to or actively chooses to go over the limit because that is the
> right behavior under the circumstances, which is completely fine,
> but, if at all avoidable, it shouldn't be misrepresenting what's
> happening by sneaking the charges into the root memcg.
>
> * Despite trying, we already do over-charge. kmemcg can't deal with
> switching over to the root memcg by the point try_charge() returns
> -EINTR, so it open-codes over-charing.
>
> * It complicates the callers. Each try_charge() user has to handle
> the weird -EINTR exception. memcg_charge_kmem() does the manual
> over-charging. mem_cgroup_do_precharge() performs unnecessary
> uncharging of root memcg, which BTW is inconsistent with what
Hmm, cancel_charge(root_mem_cgroup) is a no-op. Looks like this is a
leftover from the times when we did charge root_mem_cgroup.
Anyway, the rationale makes sense to me, and the patch looks good.
Reviewed-by: Vladimir Davydov <vdavydov@parallels.com>
> memcg_charge_kmem() does. mem_cgroup_try_charge() needs to switch
> the returned cgroup to the root one.
>
> The reality is that in memcg there are cases where we are forced
> and/or willing to go over the limit. Each such case needs to be
> scrutinized and justified but there definitely are situations where
> that is the right thing to do. We alredy do this but with a
> superficial and inconsistent disguise which leads to unnecessary
> complications.
>
> This patch updates try_charge() so that it over-charges and returns 0
> when deemed necessary. -EINTR return is removed along with all
> special case handling in the callers.
>
> While at it, remove the local variable @ret, which was initialized to
> zero and never changed, along with done: label which just returned the
> always zero @ret.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
...
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
To: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org,
hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org,
mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org,
kernel-team-b10kYP2dOMg@public.gmane.org
Subject: Re: [PATCH 2/3] memcg: ratify and consolidate over-charge handling
Date: Mon, 14 Sep 2015 15:44:20 +0300 [thread overview]
Message-ID: <20150914124420.GE30743@esperanza> (raw)
In-Reply-To: <20150913201442.GD25369-piEFEHQLUPpN0TnZuCh8vA@public.gmane.org>
On Sun, Sep 13, 2015 at 04:14:42PM -0400, Tejun Heo wrote:
> try_charge() is the main charging logic of memcg. When it hits the
> limit but either can't fail the allocation due to __GFP_NOFAIL or the
> task is likely to free memory very soon, being OOM killed, has SIGKILL
> pending or exiting, it "bypasses" the charge to the root memcg and
> returns -EINTR. While this is one approach which can be taken for
> these situations, it has several issues.
>
> * It unnecessarily lies about the reality. The number itself doesn't
> go over the limit but the actual usage does. memcg is either forced
> to or actively chooses to go over the limit because that is the
> right behavior under the circumstances, which is completely fine,
> but, if at all avoidable, it shouldn't be misrepresenting what's
> happening by sneaking the charges into the root memcg.
>
> * Despite trying, we already do over-charge. kmemcg can't deal with
> switching over to the root memcg by the point try_charge() returns
> -EINTR, so it open-codes over-charing.
>
> * It complicates the callers. Each try_charge() user has to handle
> the weird -EINTR exception. memcg_charge_kmem() does the manual
> over-charging. mem_cgroup_do_precharge() performs unnecessary
> uncharging of root memcg, which BTW is inconsistent with what
Hmm, cancel_charge(root_mem_cgroup) is a no-op. Looks like this is a
leftover from the times when we did charge root_mem_cgroup.
Anyway, the rationale makes sense to me, and the patch looks good.
Reviewed-by: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> memcg_charge_kmem() does. mem_cgroup_try_charge() needs to switch
> the returned cgroup to the root one.
>
> The reality is that in memcg there are cases where we are forced
> and/or willing to go over the limit. Each such case needs to be
> scrutinized and justified but there definitely are situations where
> that is the right thing to do. We alredy do this but with a
> superficial and inconsistent disguise which leads to unnecessary
> complications.
>
> This patch updates try_charge() so that it over-charges and returns 0
> when deemed necessary. -EINTR return is removed along with all
> special case handling in the callers.
>
> While at it, remove the local variable @ret, which was initialized to
> zero and never changed, along with done: label which just returned the
> always zero @ret.
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
...
next prev parent reply other threads:[~2015-09-14 12:44 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-13 20:14 [PATCH 1/3] memcg: collect kmem bypass conditions into __memcg_kmem_bypass() Tejun Heo
2015-09-13 20:14 ` [PATCH 2/3] memcg: ratify and consolidate over-charge handling Tejun Heo
2015-09-13 20:14 ` Tejun Heo
2015-09-13 20:15 ` [PATCH 3/3] memcg: drop unnecessary cold-path tests from __memcg_kmem_bypass() Tejun Heo
2015-09-13 20:15 ` Tejun Heo
2015-09-14 12:51 ` Vladimir Davydov
2015-09-14 12:51 ` Vladimir Davydov
2015-09-14 19:40 ` Michal Hocko
2015-09-14 19:40 ` Michal Hocko
2015-09-14 12:44 ` Vladimir Davydov [this message]
2015-09-14 12:44 ` [PATCH 2/3] memcg: ratify and consolidate over-charge handling Vladimir Davydov
2015-09-14 15:51 ` Tejun Heo
2015-09-14 15:51 ` Tejun Heo
2015-09-14 19:32 ` Michal Hocko
2015-09-14 19:56 ` Tejun Heo
2015-09-14 19:56 ` Tejun Heo
2015-09-15 8:01 ` Michal Hocko
2015-09-15 15:50 ` Tejun Heo
2015-09-14 20:07 ` [PATCH v2 " Tejun Heo
2015-09-15 16:18 ` Johannes Weiner
2015-09-15 16:18 ` Johannes Weiner
2015-09-14 9:03 ` [PATCH 1/3] memcg: collect kmem bypass conditions into __memcg_kmem_bypass() Vladimir Davydov
2015-09-14 9:03 ` Vladimir Davydov
2015-09-14 15:21 ` Michal Hocko
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=20150914124420.GE30743@esperanza \
--to=vdavydov@parallels.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=hannes@cmpxchg.org \
--cc=kernel-team@fb.com \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=tj@kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.