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=-17.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 B94C5C433B4 for ; Mon, 19 Apr 2021 23:42:16 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2315E611C9 for ; Mon, 19 Apr 2021 23:42:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2315E611C9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A5A166B006E; Mon, 19 Apr 2021 19:42:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A303C6B0070; Mon, 19 Apr 2021 19:42:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8A9978D0001; Mon, 19 Apr 2021 19:42:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0149.hostedemail.com [216.40.44.149]) by kanga.kvack.org (Postfix) with ESMTP id 6A6906B006E for ; Mon, 19 Apr 2021 19:42:15 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 27A5A180AD82F for ; Mon, 19 Apr 2021 23:42:15 +0000 (UTC) X-FDA: 78050742630.12.FC1250B Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf06.hostedemail.com (Postfix) with ESMTP id 5FA54C0007D2 for ; Mon, 19 Apr 2021 23:42:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618875734; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=x2fbpuknZ4GBkIrJZ+q/s36SJDLlw3O5/8yiMZIDB2k=; b=S5YONUvi8WeW5XJJPS7I06Z+mOb48lIaXSbG/D5nO829tFLiZ2CTZlzYvrLjpQnqhLSj2o xTO4HRjnZsruXAUb1sA3EXlnji1MZX0cN8nPopoOr7vrjT7Rv/2VbjmtxONUUlI4+U3heH KCL8J5CfhPRkQM8aKO14SAP1RhS9Kgs= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-360-CyHkTgyEN7up4guKBAhj4g-1; Mon, 19 Apr 2021 19:42:10 -0400 X-MC-Unique: CyHkTgyEN7up4guKBAhj4g-1 Received: by mail-qv1-f72.google.com with SMTP id e20-20020ad442b40000b029019aa511c767so10344724qvr.18 for ; Mon, 19 Apr 2021 16:42:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=x2fbpuknZ4GBkIrJZ+q/s36SJDLlw3O5/8yiMZIDB2k=; b=EgzZWQr98qO3NR1Pz8rkpPO9JrsnL2g80pgis5xIjbItA9g+zQ6HCO++OlZx+TgDC8 lZR1fCAaXk5mxHOCyz554UjZexl40n7IGQ5h2UQWw0wnsCo8QdQ4tz7nMn7KT/QPTdX6 En4LH8OOQUG/a7NKBz0xN73S8l3tXWmXls+oiBavcNb6p5vMJJtyGphPmcSa0OrIIaSy oSfO2vMe780KgjusaCLGpa/zqE3zSEtSSgQIlZL5TH395kgylhCvJDR0932m1qGv7Bai zvBMT6xp5cqFD2R4E7R4f3f45LpYFZLI2dMVzYjYTjEkGfrwqmpdjhVE77tQja4KATTt /YRA== X-Gm-Message-State: AOAM530BVnuhBte7S5WSaeGLTed2vT4fcbPcSUQvQNJ/3ZVMJhHJ3K9t wUfISeO5J7ttS/PQJ1ovPr9cOHHxq8ltkP0d79ioq0i2hQS6Ug/tWYiYUwWJK2RybsIgPn21Jv5 CQzaRQVn1eHY= X-Received: by 2002:a05:620a:44ce:: with SMTP id y14mr14496791qkp.171.1618875729802; Mon, 19 Apr 2021 16:42:09 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw9fC71GXWYH36qmAJMjWJoLcPTXWY88zaRU+br2/4wzu/F8zydzIcPi0q1wOkOFH+5QU+zCQ== X-Received: by 2002:a05:620a:44ce:: with SMTP id y14mr14496759qkp.171.1618875729551; Mon, 19 Apr 2021 16:42:09 -0700 (PDT) Received: from llong.remote.csb ([2601:191:8500:76c0::cdbc]) by smtp.gmail.com with ESMTPSA id n6sm10102518qtx.22.2021.04.19.16.42.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 Apr 2021 16:42:09 -0700 (PDT) From: Waiman Long X-Google-Original-From: Waiman Long Subject: Re: [PATCH v4 2/5] mm/memcg: Cache vmstat data in percpu memcg_stock_pcp To: Johannes Weiner 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 References: <20210419000032.5432-1-longman@redhat.com> <20210419000032.5432-3-longman@redhat.com> Message-ID: <09ea1749-8978-091b-7727-d86f8e6c49cc@redhat.com> Date: Mon, 19 Apr 2021 19:42:07 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=llong@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 5FA54C0007D2 X-Stat-Signature: sn66pki6pnczhrmzwoc647xidfsjya6q Received-SPF: none (redhat.com>: No applicable sender policy available) receiver=imf06; identity=mailfrom; envelope-from=""; helo=us-smtp-delivery-124.mimecast.com; client-ip=170.10.133.124 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1618875737-227018 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 4/19/21 12:38 PM, Johannes Weiner wrote: > 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. > I will change its name to, say, mod_objcg_mlstate() to indicate that it is something different. Actually, it is hard to come up with a good name which is not too long. >> @@ -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? That is a good idea. Will do that. > >> + } 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. I am going to merge patch 2 and patch 4 to avoid the confusion. > > 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. The testing done on a 2-socket system indicated that pgdat changes roughly 10-20% of time. So it does happen, especially on the kfree() path, I think. I have tried to cached vmstat update for those on the local node only, but I got more misses with that. So I am just going to change pgdat and flush out existing data for now. > >> + } 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...) If you look at patch 5, I am trying to avoid doing drain_obj_stock() unless the objcg change. I am going to do the same here. Cheers, Longman