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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48F05C433EF for ; Fri, 11 Mar 2022 16:00:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9EF6A8D0002; Fri, 11 Mar 2022 11:00:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9A02B8D0001; Fri, 11 Mar 2022 11:00:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 88FF98D0002; Fri, 11 Mar 2022 11:00:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0233.hostedemail.com [216.40.44.233]) by kanga.kvack.org (Postfix) with ESMTP id 79B5B8D0001 for ; Fri, 11 Mar 2022 11:00:55 -0500 (EST) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 294D9181B9DD1 for ; Fri, 11 Mar 2022 16:00:55 +0000 (UTC) X-FDA: 79232568870.21.324E56D Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by imf07.hostedemail.com (Postfix) with ESMTP id 601344002D for ; Fri, 11 Mar 2022 16:00:54 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 1B6B621106; Fri, 11 Mar 2022 16:00:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1647014453; h=from:from:reply-to: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=SAaUOGAe1srGHsWHBvn1FZ7QUyxX7XpJoMnIGqLANGo=; b=JC9r4EOT7aEN0yB8Qf9FysIjDt/Lfq9D5zM4Ke/NbM9uN/KN1uAZpE5K4BgUSbilo63d9z ht/QmX4FXDK1KjhUAtCwUayHbXzLF9H3rBvvtmdT+W0F37+q8YPjroPqnmmlYH+VtrjBi7 QtXDY6i3jI+KGH0cCUUGI9NsJFBQN9M= Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id DE50513A89; Fri, 11 Mar 2022 16:00:52 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id gxi4NTRyK2IjHwAAMHmgww (envelope-from ); Fri, 11 Mar 2022 16:00:52 +0000 Date: Fri, 11 Mar 2022 17:00:51 +0100 From: Michal =?iso-8859-1?Q?Koutn=FD?= To: Shakeel Butt Cc: Johannes Weiner , Michal Hocko , Roman Gushchin , Ivan Babrou , Frank Hofmann , Andrew Morton , cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Daniel Dao , stable@vger.kernel.org Subject: Re: [PATCH] memcg: sync flush only if periodic flush is delayed Message-ID: <20220311160051.GA24796@blackbody.suse.cz> References: <20220304184040.1304781-1-shakeelb@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220304184040.1304781-1-shakeelb@google.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 601344002D X-Stat-Signature: 1tjh6cfeiaoesxaw7tm17hg711wuobd5 Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=JC9r4EOT; spf=pass (imf07.hostedemail.com: domain of mkoutny@suse.com designates 195.135.220.28 as permitted sender) smtp.mailfrom=mkoutny@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com X-HE-Tag: 1647014454-738818 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.006317, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hello. TL;DR rstats are slow but accurate on reader side. To tackle the performance regression no flush seems simpler than this patch. So, I've made an overview for myself what were the relevant changes with rstat introduction. The amount of work is: - before R: O(1) W: O(tree_depth) - after R: O(nr_cpus * nr_cgroups(subtree) * nr_counters)=20 W: O(tree_depth) That doesn't look like a positive change especially on the reader side. (In theory, the reader's work would be amortized but as the original report here shows, not all workloads are diluting the flushes sufficiently. [1]) The benefit this was traded for was the greater accuracy, the possible error is: - before - O(nr_cpus * nr_cgroups(subtree) * MEMCG_CHARGE_BATCH) (1) - after O(nr_cpus * MEMCG_CHARGE_BATCH) // sync. flush or O(flush_period * max_cr) // periodic flush only (2) // max_cr is per-counter max change rate So we could argue that if the pre-rstat kernels did just fine with the error (1), they would not be worse with periodic flush if we can compare (1) and (2).=20 On Fri, Mar 04, 2022 at 06:40:40PM +0000, Shakeel Butt wrote: > This patch fixes this regression by making the rstat flushing > conditional in the performance critical codepaths. More specifically, > the kernel relies on the async periodic rstat flusher to flush the stat= s > and only if the periodic flusher is delayed by more than twice the > amount of its normal time window then the kernel allows rstat flushing > from the performance critical codepaths. I'm not sure whether your patch attempts to solve the problem of (a) periodic flush getting stuck or (b) limiting error on refault path. If it's (a), it should be tackled more systematically (dedicated wq?). If it's (b), why not just rely on periodic flush (self answer: (1) and (2) comparison is workload dependent). > Now the question: what are the side-effects of this change? The worst > that can happen is the refault codepath will see 4sec old lruvec stats > and may cause false (or missed) activations of the refaulted page which > may under-or-overestimate the workingset size. Though that is not very > concerning as the kernel can already miss or do false activations. We can't argue what's the effect of periodic only flushing so this newly introduced factor would inherit that too. I find it superfluous. Michal [1] This is worth looking at in more detail. >From the flush condition we have cr * =CE=94t =3D nr_cpus * MEMCG_CHARGE_BATCH=20 where =CE=94t is time between flushes and cr is global change rate. cr composes of all updates together (corresponds to stats_updates in memcg_rstat_updated(), max_cr is change rate per counter) cr =3D =CE=A3 cr_i <=3D nr_counters * max_cr=20 By combining these two we get shortest time between flushes: cr * =CE=94t <=3D nr_counters * max_cr * =CE=94t nr_cpus * MEMCG_CHARGE_BATCH <=3D nr_counters * max_cr * =CE=94t =CE=94t >=3D (nr_cpus * MEMCG_CHARGE_BATCH) / (nr_counters * max_cr) We are interested in=20 R_amort =3D flush_work / =CE=94t which is R_amort <=3D flush_work * nr_counters * max_cr / (nr_cpus * MEMCG_CHARG= E_BATCH) R_amort: O( nr_cpus * nr_cgroups(subtree) * nr_counters * (nr_counters * = max_cr) / (nr_cpus * MEMCG_CHARGE_BATCH) ) R_amort: O( nr_cgroups(subtree) * nr_counters^2 * max_cr) / (MEMCG_CHARGE= _BATCH) ) The square looks interesting given there are already tens of counters. (As data from Ivan have shown, we can hardly restore the pre-rstat performance on the read side even with mere mod_delayed_work().) This is what you partially solved with introduction of NR_MEMCG_EVENTS but the stats_updates was still sum of all events, so the flush might have still triggered too frequently. Maybe that would be better long-term approach, splitting into accurate and approximate counters and reflect that in the error estimator stats_up= dates. Or some other optimization of mem_cgroup_css_rstat_flush().