Received: by 10.192.165.148 with SMTP id m20csp2618688imm; Sun, 22 Apr 2018 11:00:32 -0700 (PDT) X-Google-Smtp-Source: AIpwx49z4v6gP+rkWjyUsbTL4EdyGPAcjcHODBhbMmsCW03eNGEjMgW0kXFyxydkQzXCLgoJaZiE X-Received: by 10.101.82.68 with SMTP id q4mr6827437pgp.201.1524420032583; Sun, 22 Apr 2018 11:00:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524420032; cv=none; d=google.com; s=arc-20160816; b=KuBctvK/Xk5tSdoZPcqmXQPj3kT67xP8LJHtVgkgfmzvPKeMQQTbyxa6ZbyhlD/uda TdRr5QuQ9fkUJ2ya940nAZu9C1k6O0PObdk2TWbOn2cVRprg5Ma2qxjk4ftynejVkd2A JSkzlw1k8Bjiz6g1Zk4dvfELXRqjrgaPXeGX1/n9DvmZknY/wKm4fIXs586s2Dj/JtDL Pk/EnDAnFMt/vlwpPznpnZRW5CLQkv/2Upz7GxsM/kxHrzle4+iQPNw2HQR9SkglN0sk ChxNuGGtTaB5QWwE3TDn2/ydC+ReX/eU/USAr8F8VSdfrXFJykO+jeLWvFSjT/sq6AYC /eCA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :dkim-signature:arc-authentication-results; bh=dPChb5cNJGesRtrD4rbEq9oS/GIL8xwwl0NBRJKCj40=; b=RTzm9bYhJCRVbr12+dp92A+L0TL5hxVEwEUmsNLM2DJ6Ce+p8LpBZ6+CFhlTSNASIE sb+/xdCoY4KOp9jycsAGhTy1wU+h94+3D/Dv6mPFmjEyLJMOMKUejgAiocIH2DGGyXxN YLC5qgrls9D7A36mA7VUK/+c7oAUTm7lHn0H9lpF4HZCkywNn1OjnnLhScJ7f1wBrSNO FvIHhFsXVQOQZGoeS8UNc9+bWNCWS6No1u/yjZRhI32+Q2CefktmR9xVfoW0E585UKxJ MqAP0WSSnyIqgvjskSrUf6dCxIq6IbSKIBO2fCI3WQ2l0FwY+AgP8l/9gOwpFSwdT3Yd p10A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eth/SY6b; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id s16si5670872pgv.596.2018.04.22.11.00.16; Sun, 22 Apr 2018 11:00:32 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eth/SY6b; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753412AbeDVR7H (ORCPT + 99 others); Sun, 22 Apr 2018 13:59:07 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:41162 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751683AbeDVR7G (ORCPT ); Sun, 22 Apr 2018 13:59:06 -0400 Received: by mail-lf0-f68.google.com with SMTP id o123-v6so9079725lfe.8 for ; Sun, 22 Apr 2018 10:59:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=dPChb5cNJGesRtrD4rbEq9oS/GIL8xwwl0NBRJKCj40=; b=eth/SY6bvhCcmw3dTAx4bnbj4iUJX9ARE0z5edgZvHzRV/pr729wdFc0ncpPA1Jt7j EiTZ9cpSm4j45am+7WGMnnuNOOgTr1l/Zk+h5CjZ2AYFn1rIspbl4ysZYfqkHiYuJNSn QdbVkcsSFy6pnULHHVd9+4APeDNUPaMhCBzmUA5v5FTLgE41aSPbWw4qzKho5Ath+hYT aeFxC8D6snbjE+CANTRGEdAMQnfrDY0VDl0rlafDjm6msmyL51foVC5p5PfQvit8s7de W/3v3SYdmxV4cxEOXXgElAwV49wlUksLRRp2qY0zXjQwlXZ7jLR70nPc9LGlTdc7TliA zKBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=dPChb5cNJGesRtrD4rbEq9oS/GIL8xwwl0NBRJKCj40=; b=n4nOj8hUwnO/xx4Ah9u0mSEDGJeIxMLj6GHNTZ16OnIZiMXF28L10+vGqf22rOijGt HbquWBBIYc8iXJciAHyzCxwH9HO57hIrnJt0CLt11N3Yr4RGZt0p8GUKOOgri0lGk587 jAHyGERB6PaNs5b4a8sDX3uRcfzYhhmOZ9M24dUBNgxxN5k6roifbgGpd9fS09qi/9dF 9vTo8cw1mEn1sEdKOR9WZXoC6uzn9cQk4v18Qssqt/sU9kfubQKnj29jLAUzx+G5xtyy N8QBmrLEzsjokYk5PinKK0ACb+PK6Y8YAYhYsmlor29KEalsohgSYUAILNBTcJoB+8B5 jwww== X-Gm-Message-State: ALQs6tDjVB9krSI2vdSKTFEoLKf5AiXJ7oVIdrRt9zkOHbVVy5vB4FS4 c8mRerzBZPZg9zJfVpiifW4= X-Received: by 2002:a19:8387:: with SMTP id f129-v6mr7023149lfd.110.1524419944350; Sun, 22 Apr 2018 10:59:04 -0700 (PDT) Received: from esperanza (81.5.110.211.dhcp.mipt-telecom.ru. [81.5.110.211]) by smtp.gmail.com with ESMTPSA id x17sm1956344ljx.80.2018.04.22.10.59.02 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 22 Apr 2018 10:59:03 -0700 (PDT) Date: Sun, 22 Apr 2018 20:59:00 +0300 From: Vladimir Davydov To: Kirill Tkhai Cc: akpm@linux-foundation.org, shakeelb@google.com, viro@zeniv.linux.org.uk, hannes@cmpxchg.org, mhocko@kernel.org, tglx@linutronix.de, pombredanne@nexb.com, stummala@codeaurora.org, gregkh@linuxfoundation.org, sfr@canb.auug.org.au, guro@fb.com, mka@chromium.org, penguin-kernel@I-love.SAKURA.ne.jp, chris@chris-wilson.co.uk, longman@redhat.com, minchan@kernel.org, hillf.zj@alibaba-inc.com, ying.huang@intel.com, mgorman@techsingularity.net, jbacik@fb.com, linux@roeck-us.net, linux-kernel@vger.kernel.org, linux-mm@kvack.org, willy@infradead.org, lirongqing@baidu.com, aryabinin@virtuozzo.com Subject: Re: [PATCH v2 04/12] mm: Assign memcg-aware shrinkers bitmap to memcg Message-ID: <20180422175900.dsjmm7gt2nsqj3er@esperanza> References: <152397794111.3456.1281420602140818725.stgit@localhost.localdomain> <152399121146.3456.5459546288565589098.stgit@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <152399121146.3456.5459546288565589098.stgit@localhost.localdomain> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 17, 2018 at 09:53:31PM +0300, Kirill Tkhai wrote: > Imagine a big node with many cpus, memory cgroups and containers. > Let we have 200 containers, every container has 10 mounts, > and 10 cgroups. All container tasks don't touch foreign > containers mounts. If there is intensive pages write, > and global reclaim happens, a writing task has to iterate > over all memcgs to shrink slab, before it's able to go > to shrink_page_list(). > > Iteration over all the memcg slabs is very expensive: > the task has to visit 200 * 10 = 2000 shrinkers > for every memcg, and since there are 2000 memcgs, > the total calls are 2000 * 2000 = 4000000. > > So, the shrinker makes 4 million do_shrink_slab() calls > just to try to isolate SWAP_CLUSTER_MAX pages in one > of the actively writing memcg via shrink_page_list(). > I've observed a node spending almost 100% in kernel, > making useless iteration over already shrinked slab. > > This patch adds bitmap of memcg-aware shrinkers to memcg. > The size of the bitmap depends on bitmap_nr_ids, and during > memcg life it's maintained to be enough to fit bitmap_nr_ids > shrinkers. Every bit in the map is related to corresponding > shrinker id. > > Next patches will maintain set bit only for really charged > memcg. This will allow shrink_slab() to increase its > performance in significant way. See the last patch for > the numbers. > > Signed-off-by: Kirill Tkhai > --- > include/linux/memcontrol.h | 15 +++++ > mm/memcontrol.c | 125 ++++++++++++++++++++++++++++++++++++++++++++ > mm/vmscan.c | 21 +++++++ > 3 files changed, 160 insertions(+), 1 deletion(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index af9eed2e3e04..2ec96ab46b01 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -115,6 +115,7 @@ struct mem_cgroup_per_node { > unsigned long lru_zone_size[MAX_NR_ZONES][NR_LRU_LISTS]; > > struct mem_cgroup_reclaim_iter iter[DEF_PRIORITY + 1]; > + struct memcg_shrinker_map __rcu *shrinkers_map; shrinker_map > > struct rb_node tree_node; /* RB tree node */ > unsigned long usage_in_excess;/* Set to the value by which */ > @@ -153,6 +154,11 @@ struct mem_cgroup_thresholds { > struct mem_cgroup_threshold_ary *spare; > }; > > +struct memcg_shrinker_map { > + struct rcu_head rcu; > + unsigned long map[0]; > +}; > + This struct should be defined before struct mem_cgroup_per_node. A comment explaining what this map is for and what it maps would be really helpful. > enum memcg_kmem_state { > KMEM_NONE, > KMEM_ALLOCATED, > @@ -1200,6 +1206,8 @@ extern int memcg_nr_cache_ids; > void memcg_get_cache_ids(void); > void memcg_put_cache_ids(void); > > +extern int shrinkers_max_nr; > + memcg_shrinker_id_max? > /* > * Helper macro to loop through all memcg-specific caches. Callers must still > * check if the cache is valid (it is either valid or NULL). > @@ -1223,6 +1231,13 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg) > return memcg ? memcg->kmemcg_id : -1; > } > > +extern struct memcg_shrinker_map __rcu *root_shrinkers_map[]; > +#define SHRINKERS_MAP(memcg, nid) \ > + (memcg == root_mem_cgroup || !memcg ? \ > + root_shrinkers_map[nid] : memcg->nodeinfo[nid]->shrinkers_map) > + > +extern int expand_shrinker_maps(int old_id, int id); > + I'm strongly against using a special map for the root cgroup. I'd prefer to disable this optimization for the root cgroup altogether and simply iterate over all registered shrinkers when it comes to global reclaim. It shouldn't degrade performance as the root cgroup is singular. > #else > #define for_each_memcg_cache_index(_idx) \ > for (; NULL; ) > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 2959a454a072..562dfb1be9ef 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -305,6 +305,113 @@ EXPORT_SYMBOL(memcg_kmem_enabled_key); > > struct workqueue_struct *memcg_kmem_cache_wq; > > +static DECLARE_RWSEM(shrinkers_max_nr_rwsem); Why rwsem? AFAIU you want to synchronize between two code paths: when a memory cgroup is allocated (or switched online?) to allocate a shrinker map for it and in the function growing shrinker maps for all cgroups. A mutex should be enough for this. > +struct memcg_shrinker_map __rcu *root_shrinkers_map[MAX_NUMNODES] = { 0 }; > + > +static void get_shrinkers_max_nr(void) > +{ > + down_read(&shrinkers_max_nr_rwsem); > +} > + > +static void put_shrinkers_max_nr(void) > +{ > + up_read(&shrinkers_max_nr_rwsem); > +} > + > +static void kvfree_map_rcu(struct rcu_head *head) free_shrinker_map_rcu > +{ > + kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > +} > + > +static int memcg_expand_maps(struct mem_cgroup *memcg, int nid, Bad name: the function reallocates just one map, not many maps; the name doesn't indicate that it is about shrinkers; the name is inconsistent with alloc_shrinker_maps and free_shrinker_maps. Please fix. > + int size, int old_size) > +{ > + struct memcg_shrinker_map *new, *old; > + > + lockdep_assert_held(&shrinkers_max_nr_rwsem); > + > + new = kvmalloc(sizeof(*new) + size, GFP_KERNEL); > + if (!new) > + return -ENOMEM; > + > + /* Set all old bits, clear all new bits */ > + memset(new->map, (int)0xff, old_size); > + memset((void *)new->map + old_size, 0, size - old_size); > + > + old = rcu_dereference_protected(SHRINKERS_MAP(memcg, nid), true); > + > + if (memcg) > + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinkers_map, new); > + else > + rcu_assign_pointer(root_shrinkers_map[nid], new); > + > + if (old) > + call_rcu(&old->rcu, kvfree_map_rcu); > + > + return 0; > +} > + > +static int alloc_shrinker_maps(struct mem_cgroup *memcg, int nid) > +{ > + /* Skip allocation, when we're initializing root_mem_cgroup */ > + if (!root_mem_cgroup) > + return 0; > + > + return memcg_expand_maps(memcg, nid, shrinkers_max_nr/BITS_PER_BYTE, 0); > +} > + > +static void free_shrinker_maps(struct mem_cgroup *memcg, > + struct mem_cgroup_per_node *pn) > +{ > + struct memcg_shrinker_map *map; > + > + if (memcg == root_mem_cgroup) > + return; > + > + /* IDR unhashed long ago, and expand_shrinker_maps can't race with us */ > + map = rcu_dereference_protected(pn->shrinkers_map, true); > + kvfree_map_rcu(&map->rcu); > +} > + > +static struct idr mem_cgroup_idr; > + > +int expand_shrinker_maps(int old_nr, int nr) > +{ > + int id, size, old_size, node, ret; > + struct mem_cgroup *memcg; > + > + old_size = old_nr / BITS_PER_BYTE; > + size = nr / BITS_PER_BYTE; > + > + down_write(&shrinkers_max_nr_rwsem); > + for_each_node(node) { Iterating over cgroups first, numa nodes second seems like a better idea to me. I think you should fold for_each_node in memcg_expand_maps. > + idr_for_each_entry(&mem_cgroup_idr, memcg, id) { Iterating over mem_cgroup_idr looks strange. Why don't you use for_each_mem_cgroup? > + if (id == 1) > + memcg = NULL; > + ret = memcg_expand_maps(memcg, node, size, old_size); > + if (ret) > + goto unlock; > + } > + > + /* root_mem_cgroup is not initialized yet */ > + if (id == 0) > + ret = memcg_expand_maps(NULL, node, size, old_size); > + } > +unlock: > + up_write(&shrinkers_max_nr_rwsem); > + return ret; > +} > +#else /* CONFIG_SLOB */ > +static void get_shrinkers_max_nr(void) { } > +static void put_shrinkers_max_nr(void) { } > + > +static int alloc_shrinker_maps(struct mem_cgroup *memcg, int nid) > +{ > + return 0; > +} > +static void free_shrinker_maps(struct mem_cgroup *memcg, > + struct mem_cgroup_per_node *pn) { } > + > #endif /* !CONFIG_SLOB */ > > /** > @@ -3002,6 +3109,8 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css, > } > > #ifndef CONFIG_SLOB > +int shrinkers_max_nr; > + > static int memcg_online_kmem(struct mem_cgroup *memcg) > { > int memcg_id; > @@ -4266,7 +4375,10 @@ static DEFINE_IDR(mem_cgroup_idr); > static void mem_cgroup_id_remove(struct mem_cgroup *memcg) > { > if (memcg->id.id > 0) { > + /* Removing IDR must be visible for expand_shrinker_maps() */ > + get_shrinkers_max_nr(); > idr_remove(&mem_cgroup_idr, memcg->id.id); > + put_shrinkers_max_nr(); > memcg->id.id = 0; > } > } > @@ -4333,12 +4445,17 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > if (!pn->lruvec_stat_cpu) > goto err_pcpu; > > + if (alloc_shrinker_maps(memcg, node)) > + goto err_maps; > + > lruvec_init(&pn->lruvec); > pn->usage_in_excess = 0; > pn->on_tree = false; > pn->memcg = memcg; > return 0; > > +err_maps: > + free_percpu(pn->lruvec_stat_cpu); > err_pcpu: > memcg->nodeinfo[node] = NULL; > kfree(pn); > @@ -4352,6 +4469,7 @@ static void free_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > if (!pn) > return; > > + free_shrinker_maps(memcg, pn); > free_percpu(pn->lruvec_stat_cpu); > kfree(pn); > } > @@ -4407,13 +4525,18 @@ static struct mem_cgroup *mem_cgroup_alloc(void) > #ifdef CONFIG_CGROUP_WRITEBACK > INIT_LIST_HEAD(&memcg->cgwb_list); > #endif > + > + get_shrinkers_max_nr(); > for_each_node(node) > - if (alloc_mem_cgroup_per_node_info(memcg, node)) > + if (alloc_mem_cgroup_per_node_info(memcg, node)) { > + put_shrinkers_max_nr(); > goto fail; > + } > > memcg->id.id = idr_alloc(&mem_cgroup_idr, memcg, > 1, MEM_CGROUP_ID_MAX, > GFP_KERNEL); > + put_shrinkers_max_nr(); > if (memcg->id.id < 0) > goto fail; > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 4f02fe83537e..f63eb5596c35 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -172,6 +172,22 @@ static DECLARE_RWSEM(shrinker_rwsem); > #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB) > static DEFINE_IDR(shrinkers_id_idr); > > +static int expand_shrinker_id(int id) > +{ > + if (likely(id < shrinkers_max_nr)) > + return 0; > + > + id = shrinkers_max_nr * 2; > + if (id == 0) > + id = BITS_PER_BYTE; > + > + if (expand_shrinker_maps(shrinkers_max_nr, id)) > + return -ENOMEM; > + > + shrinkers_max_nr = id; > + return 0; > +} > + I think this function should live in memcontrol.c and shrinkers_max_nr should be private to memcontrol.c. > static int add_memcg_shrinker(struct shrinker *shrinker) > { > int id, ret; > @@ -180,6 +196,11 @@ static int add_memcg_shrinker(struct shrinker *shrinker) > ret = id = idr_alloc(&shrinkers_id_idr, shrinker, 0, 0, GFP_KERNEL); > if (ret < 0) > goto unlock; > + ret = expand_shrinker_id(id); > + if (ret < 0) { > + idr_remove(&shrinkers_id_idr, id); > + goto unlock; > + } > shrinker->id = id; > ret = 0; > unlock: >