linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@kernel.org>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Tejun Heo <tj@kernel.org>, Roman Gushchin <guro@fb.com>,
	linux-mm@kvack.org, cgroups@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH] mm: memcontrol: asynchronous reclaim for memory.high
Date: Thu, 20 Feb 2020 10:46:39 +0100	[thread overview]
Message-ID: <20200220094639.GD20509@dhcp22.suse.cz> (raw)
In-Reply-To: <20200219211735.GD54486@cmpxchg.org>

On Wed 19-02-20 16:17:35, Johannes Weiner wrote:
> On Wed, Feb 19, 2020 at 08:53:32PM +0100, Michal Hocko wrote:
> > On Wed 19-02-20 14:16:18, Johannes Weiner wrote:
[...]
> > > [ This is generally work in process: for example, if you isolate
> > >   workloads with memory.low, kswapd cpu time isn't accounted to the
> > >   cgroup that causes it. Swap IO issued by kswapd isn't accounted to
> > >   the group that is getting swapped.
> > 
> > Well, kswapd is a system activity and as such it is acceptable that it
> > is accounted to the system. But in this case we are talking about a
> > memcg configuration which influences all other workloads by stealing CPU
> > cycles from them 
> 
> From a user perspective this isn't a meaningful distinction.
> 
> If I partition my memory among containers and one cgroup is acting
> out, I would want the culprit to be charged for the cpu cycles the
> reclaim is causing. Whether I divide my machine up using memory.low or
> using memory.max doesn't really matter: I'm choosing between the two
> based on a *memory policy* I want to implement - work-conserving vs
> non-conserving. I shouldn't have to worry about the kernel tracking
> CPU cycles properly in the respective implementations of these knobs.
> 
> So kswapd is very much a cgroup-attributable activity, *especially* if
> I'm using memory.low to delineate different memory domains.

While I understand what you are saying I do not think this is easily
achievable with the current implementation. The biggest problem I can
see is that you do not have a clear information who to charge for
the memory shortage on a particular NUMA node with a pure low limit
based balancing because the limit is not NUMA aware. Besides that the
origin of the memory pressure might be outside of any memcg.  You can
punish/account all memcgs in excess in some manner, e.g. proportionally
to their size/excess but I am not really sure how fair that will
be. Sounds like an interesting project but also sounds like tangent to
this patch.

High/Max limits are quite different because they are dealing with
the internal memory pressure and you can attribute it to the
cgroup/hierarchy which is in excess. There is a clear domain to reclaim
from. This is an easier model to reason about IMHO.

> > without much throttling on the consumer side - especially when the
> > memory is reclaimable without a lot of sleeping or contention on
> > locks etc.
> 
> The limiting factor on the consumer side is IO. Reading a page is way
> more costly than reclaiming it, which is why we built our isolation
> stack starting with memory and IO control and are only now getting to
> working on proper CPU isolation.
> 
> > I am absolutely aware that we will never achieve a perfect isolation due
> > to all sorts of shared data structures, lock contention and what not but
> > this patch alone just allows spill over to unaccounted work way too
> > easily IMHO.
> 
> I understand your concern about CPU cycles escaping, and I share
> it. My point is that this patch isn't adding a problem that isn't
> already there, nor is it that much of a practical concern at the time
> of this writing given the state of CPU isolation in general.

I beg to differ here. Ppu controller should be able to isolate user
contexts performing high limit reclaim now. Your patch is changing that
functionality to become unaccounted for a large part and that might be
seen as a regression for those workloads which partition the system by
using high limit and also rely on cpu controller because workloads are
CPU sensitive.

Without the CPU controller support this patch is not complete and I do
not see an absolute must to marge it ASAP because it is not a regression
fix or something we cannot live without.
-- 
Michal Hocko
SUSE Labs

  reply	other threads:[~2020-02-20  9:46 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-19 18:12 [PATCH] mm: memcontrol: asynchronous reclaim for memory.high Johannes Weiner
2020-02-19 18:37 ` Michal Hocko
2020-02-19 19:16   ` Johannes Weiner
2020-02-19 19:53     ` Michal Hocko
2020-02-19 21:17       ` Johannes Weiner
2020-02-20  9:46         ` Michal Hocko [this message]
2020-02-20 14:41           ` Johannes Weiner
2020-02-19 21:41       ` Daniel Jordan
2020-02-19 22:08         ` Johannes Weiner
2020-02-20 15:45           ` Daniel Jordan
2020-02-20 15:56             ` Tejun Heo
2020-02-20 18:23               ` Daniel Jordan
2020-02-20 18:45                 ` Tejun Heo
2020-02-20 19:55                   ` Daniel Jordan
2020-02-20 20:54                     ` Tejun Heo
2020-02-19 19:17   ` Chris Down
2020-02-19 19:31   ` Andrew Morton
2020-02-19 21:33     ` Johannes Weiner
2020-02-26 20:25 ` Shakeel Butt
2020-02-26 22:26   ` Johannes Weiner
2020-02-26 23:36     ` Shakeel Butt
2020-02-26 23:46       ` Johannes Weiner
2020-02-27  0:12     ` Yang Shi
2020-02-27  2:42       ` Shakeel Butt
2020-02-27  9:58       ` Michal Hocko
2020-02-27 12:50       ` Johannes Weiner
2020-02-26 23:59   ` Yang Shi
2020-02-27  2:36     ` Shakeel Butt

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=20200220094639.GD20509@dhcp22.suse.cz \
    --to=mhocko@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.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 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).