From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BBB49C433ED for ; Mon, 19 Apr 2021 16:38:31 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 203E86100B for ; Mon, 19 Apr 2021 16:38:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 203E86100B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7519A6B006C; Mon, 19 Apr 2021 12:38:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 729AB6B006E; Mon, 19 Apr 2021 12:38:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5CA336B0070; Mon, 19 Apr 2021 12:38:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 419B36B006C for ; Mon, 19 Apr 2021 12:38:30 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id F2F233626 for ; Mon, 19 Apr 2021 16:38:29 +0000 (UTC) X-FDA: 78049674780.09.629AFDB Received: from mail-qt1-f175.google.com (mail-qt1-f175.google.com [209.85.160.175]) by imf06.hostedemail.com (Postfix) with ESMTP id E93DBC0007D6 for ; Mon, 19 Apr 2021 16:38:31 +0000 (UTC) Received: by mail-qt1-f175.google.com with SMTP id c6so26559663qtc.1 for ; Mon, 19 Apr 2021 09:38:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=vpCqSo1bi/Itljn0ARQBAE88SdcDOq4CLh+u6Q1ryxk=; b=U/sz0/KTAjVUZKU3ywblEqS+Xfp7IcXV20jwhosV0NhrBK3OLhAEz3ZxHF6YkzX2s5 SLXvchft5Wow+756f0u3h9h1ovxymMTNqN09gzC7FMXeZ/RBj13ZWfXItLyWZAYWD2qt FXbF6BnRW/P8WyVHirhaHvhRGLDPUFpKj36Qs12Ug3/UaW9w0y2CfQ2NbKPa+KjQYVtW AXkyuJMsyoOLXhgXL+1OknkZ/wQJy/L+tSQn/XSUVXULwS7qn2ui7ZqRQKNA35oz4NM3 Fr3KahNRnWKD6FRQLlQKD+Gt0C7IV8uTn+VsMoL0uran9OHtkyFSZ4GEsQ5BwK1a3SEL rqNA== 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=vpCqSo1bi/Itljn0ARQBAE88SdcDOq4CLh+u6Q1ryxk=; b=D3qShe7AWKo93e/gOhEOOxdLRS4m+kIhu8l7iQegycTP+bPEckJe9SvTGabWMCWs9D u0ywu80TynaqCbl9n/bNnIqOCkSWO6Sr/bE9ZL0VBGam61ZmZWPvPpB5VlxNZez3zJU4 z3eiGyOS+oHsVMsB42cejkCFQVgDtaxVLgAKO5Qkf1ZfwSrzVBiTt45xW7wMB9P0N9Hm CDszHw7lgQ8MHGhU4dvV6dvEULFBLPf9tIhAEGmKKlSLUT99CIBzbeaxjs1OT7CFlM1n 15+36RwmhMjh8G2mz4uqVf0rnMqPAk1+/9FaMQhwYnqCbpzXMNI/xeOvm9Tw3SJ+mNqB 0dmA== X-Gm-Message-State: AOAM530enl+WoGdNQokhBww7/z/L5XpkTSBz4NdXBe/VSmqtruNCqt0L YJlOQ78+VBRxKaGSiAMPr1/XmA== X-Google-Smtp-Source: ABdhPJzb3uL7lMp8OSB1SKoU6Hb7bhiEV+btM2N//ZOVJ7VHEgyfXj+WLMpYxhiXniSItifgCTM9xw== X-Received: by 2002:ac8:7cb9:: with SMTP id z25mr4226302qtv.361.1618850308559; Mon, 19 Apr 2021 09:38:28 -0700 (PDT) Received: from localhost (70.44.39.90.res-cmts.bus.ptd.net. [70.44.39.90]) by smtp.gmail.com with ESMTPSA id w67sm10252775qkc.79.2021.04.19.09.38.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Apr 2021 09:38:27 -0700 (PDT) Date: Mon, 19 Apr 2021 12:38:27 -0400 From: Johannes Weiner To: Waiman Long Cc: Michal Hocko , Vladimir Davydov , Andrew Morton , Tejun Heo , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Vlastimil Babka , Roman Gushchin , linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Shakeel Butt , Muchun Song , Alex Shi , Chris Down , Yafang Shao , Wei Yang , Masayoshi Mizuma , Xing Zhengjun , Matthew Wilcox Subject: Re: [PATCH v4 2/5] mm/memcg: Cache vmstat data in percpu memcg_stock_pcp Message-ID: References: <20210419000032.5432-1-longman@redhat.com> <20210419000032.5432-3-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210419000032.5432-3-longman@redhat.com> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: E93DBC0007D6 X-Stat-Signature: ba6h4tgnybzi8ysrnbwfoh19m9htdixy Received-SPF: none (cmpxchg.org>: No applicable sender policy available) receiver=imf06; identity=mailfrom; envelope-from=""; helo=mail-qt1-f175.google.com; client-ip=209.85.160.175 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1618850311-533750 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sun, Apr 18, 2021 at 08:00:29PM -0400, Waiman Long wrote: > Before the new slab memory controller with per object byte charging, > charging and vmstat data update happen only when new slab pages are > allocated or freed. Now they are done with every kmem_cache_alloc() > and kmem_cache_free(). This causes additional overhead for workloads > that generate a lot of alloc and free calls. > > The memcg_stock_pcp is used to cache byte charge for a specific > obj_cgroup to reduce that overhead. To further reducing it, this patch > makes the vmstat data cached in the memcg_stock_pcp structure as well > until it accumulates a page size worth of update or when other cached > data change. Caching the vmstat data in the per-cpu stock eliminates two > writes to non-hot cachelines for memcg specific as well as memcg-lruvecs > specific vmstat data by a write to a hot local stock cacheline. > > On a 2-socket Cascade Lake server with instrumentation enabled and this > patch applied, it was found that about 20% (634400 out of 3243830) > of the time when mod_objcg_state() is called leads to an actual call > to __mod_objcg_state() after initial boot. When doing parallel kernel > build, the figure was about 17% (24329265 out of 142512465). So caching > the vmstat data reduces the number of calls to __mod_objcg_state() > by more than 80%. > > Signed-off-by: Waiman Long > Reviewed-by: Shakeel Butt > --- > mm/memcontrol.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 61 insertions(+), 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index dc9032f28f2e..693453f95d99 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2213,7 +2213,10 @@ struct memcg_stock_pcp { > > #ifdef CONFIG_MEMCG_KMEM > struct obj_cgroup *cached_objcg; > + struct pglist_data *cached_pgdat; > unsigned int nr_bytes; > + int vmstat_idx; > + int vmstat_bytes; > #endif > > struct work_struct work; > @@ -3150,8 +3153,9 @@ void __memcg_kmem_uncharge_page(struct page *page, int order) > css_put(&memcg->css); > } > > -void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, > - enum node_stat_item idx, int nr) > +static inline void __mod_objcg_state(struct obj_cgroup *objcg, > + struct pglist_data *pgdat, > + enum node_stat_item idx, int nr) This naming is dangerous, as the __mod_foo naming scheme we use everywhere else suggests it's the same function as mod_foo() just with preemption/irqs disabled. > @@ -3159,10 +3163,53 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, > rcu_read_lock(); > memcg = obj_cgroup_memcg(objcg); > lruvec = mem_cgroup_lruvec(memcg, pgdat); > - mod_memcg_lruvec_state(lruvec, idx, nr); > + __mod_memcg_lruvec_state(lruvec, idx, nr); > rcu_read_unlock(); > } > > +void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, > + enum node_stat_item idx, int nr) > +{ > + struct memcg_stock_pcp *stock; > + unsigned long flags; > + > + local_irq_save(flags); > + stock = this_cpu_ptr(&memcg_stock); > + > + /* > + * Save vmstat data in stock and skip vmstat array update unless > + * accumulating over a page of vmstat data or when pgdat or idx > + * changes. > + */ > + if (stock->cached_objcg != objcg) { > + /* Output the current data as is */ When you get here with the wrong objcg and hit the cold path, it's usually immediately followed by an uncharge -> refill_obj_stock() that will then flush and reset cached_objcg. Instead of doing two cold paths, why not flush the old objcg right away and set the new so that refill_obj_stock() can use the fast path? > + } else if (!stock->vmstat_bytes) { > + /* Save the current data */ > + stock->vmstat_bytes = nr; > + stock->vmstat_idx = idx; > + stock->cached_pgdat = pgdat; > + nr = 0; > + } else if ((stock->cached_pgdat != pgdat) || > + (stock->vmstat_idx != idx)) { > + /* Output the cached data & save the current data */ > + swap(nr, stock->vmstat_bytes); > + swap(idx, stock->vmstat_idx); > + swap(pgdat, stock->cached_pgdat); Is this optimization worth doing? You later split vmstat_bytes and idx doesn't change anymore. How often does the pgdat change? This is a per-cpu cache after all, and the numa node a given cpu allocates from tends to not change that often. Even with interleaving mode, which I think is pretty rare, the interleaving happens at the slab/page level, not the object level, and the cache isn't bigger than a page anyway. > + } else { > + stock->vmstat_bytes += nr; > + if (abs(stock->vmstat_bytes) > PAGE_SIZE) { > + nr = stock->vmstat_bytes; > + stock->vmstat_bytes = 0; > + } else { > + nr = 0; > + } ..and this is the regular overflow handling done by the objcg and memcg charge stock as well. How about this? if (stock->cached_objcg != objcg || stock->cached_pgdat != pgdat || stock->vmstat_idx != idx) { drain_obj_stock(stock); obj_cgroup_get(objcg); stock->cached_objcg = objcg; stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); stock->vmstat_idx = idx; } stock->vmstat_bytes += nr_bytes; if (abs(stock->vmstat_bytes > PAGE_SIZE)) drain_obj_stock(stock); (Maybe we could be clever, here since the charge and stat caches are the same size: don't flush an oversized charge cache from refill_obj_stock in the charge path, but leave it to the mod_objcg_state() that follows; likewise don't flush an undersized vmstat stock from mod_objcg_state() in the uncharge path, but leave it to the refill_obj_stock() that follows. Could get a bit complicated...) > @@ -3213,6 +3260,17 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) > stock->nr_bytes = 0; > } > > + /* > + * Flush the vmstat data in current stock > + */ > + if (stock->vmstat_bytes) { > + __mod_objcg_state(old, stock->cached_pgdat, stock->vmstat_idx, > + stock->vmstat_bytes); ... then inline __mod_objcg_state() here into the only caller, and there won't be any need to come up with a better name. > + stock->cached_pgdat = NULL; > + stock->vmstat_bytes = 0; > + stock->vmstat_idx = 0; > + } > + > obj_cgroup_put(old); > stock->cached_objcg = NULL;