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 98763C2BD09 for ; Thu, 27 Jun 2024 23:34:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E87EE6B0092; Thu, 27 Jun 2024 19:34:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E10ED6B00A0; Thu, 27 Jun 2024 19:34:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C62FC6B00A3; Thu, 27 Jun 2024 19:34:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id A4FEA6B0092 for ; Thu, 27 Jun 2024 19:34:32 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 17669C0C13 for ; Thu, 27 Jun 2024 23:34:32 +0000 (UTC) X-FDA: 82278275184.07.5097ADF Received: from out-182.mta1.migadu.com (out-182.mta1.migadu.com [95.215.58.182]) by imf09.hostedemail.com (Postfix) with ESMTP id AA2A9140009 for ; Thu, 27 Jun 2024 23:34:29 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="Wg5kRHy/"; spf=pass (imf09.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719531246; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=rlKYYvcrAhje7I3jr3QA/EGpapoyOIv7xLn5q2CRVCc=; b=hTzUOG8cVD15SKdGL+jdmIY6TI0S2GXk3x0l7EFULdFKIgeKBdKZ135OoQCOVp9q7RB8UZ roe8G9awp9Wi3i11YqP7k8WfhyRBTdF7jvGaTxVOiUvhmwPhQGhbzz5sgkLCKPq+rou/kX Pfdg0wGgd24XOZ9XCXToZTvbxofRo4U= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="Wg5kRHy/"; spf=pass (imf09.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719531246; a=rsa-sha256; cv=none; b=tJqiVeA+89JvXamgvBxiyk4vqCtVDi9kfDdRdx4mo4BTHKljs+OLTJ5g70wz6SVVc/n+Lo 581cT2KbJwU2zTfuoT4yNUK05g+zhLn1KJ9VZzi2wH8+sB4ekK5iKvo4sXhwv/D9r827lu UZqxIJ0xCA7O855GXQ0eyPy4hSxCgis= X-Envelope-To: hawk@kernel.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1719531267; 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: in-reply-to:in-reply-to:references:references; bh=rlKYYvcrAhje7I3jr3QA/EGpapoyOIv7xLn5q2CRVCc=; b=Wg5kRHy/H+aJGY3Lu99Av7zHqOzqYCUgh19hcBVLpA3b3SHO/iMH6mjIjflbx5X4vgfVcR fWpc+deoyN3nVhVfx9h+JqEVK+8u+drcKEcIKAOXOamuSRvfUcGU9WuBMS/dcQKia9AlTq kN2bW+XxAWfAcXF+IbgEkt8LKtLWwjM= X-Envelope-To: tj@kernel.org X-Envelope-To: cgroups@vger.kernel.org X-Envelope-To: yosryahmed@google.com X-Envelope-To: hannes@cmpxchg.org X-Envelope-To: lizefan.x@bytedance.com X-Envelope-To: longman@redhat.com X-Envelope-To: kernel-team@cloudflare.com X-Envelope-To: linux-mm@kvack.org X-Envelope-To: linux-kernel@vger.kernel.org Date: Thu, 27 Jun 2024 16:34:21 -0700 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: Jesper Dangaard Brouer Cc: tj@kernel.org, cgroups@vger.kernel.org, yosryahmed@google.com, hannes@cmpxchg.org, lizefan.x@bytedance.com, longman@redhat.com, kernel-team@cloudflare.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V4 2/2] cgroup/rstat: Avoid thundering herd problem by kswapd across NUMA nodes Message-ID: <4n3qu75efpznkomxytm7irwfiq44hhi4hb5igjbd55ooxgmvwa@tbgmwvcqsy75> References: <171952310959.1810550.17003659816794335660.stgit@firesoul> <171952312320.1810550.13209360603489797077.stgit@firesoul> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <171952312320.1810550.13209360603489797077.stgit@firesoul> X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: AA2A9140009 X-Stat-Signature: ssgnp4xr47dzxtqzjgf6a51pmr6kpn9u X-Rspam-User: X-HE-Tag: 1719531269-511755 X-HE-Meta: U2FsdGVkX1/UjpqD/ZD2BUh+W8z7SVXVZfpPE/g49T8gsRh3UweU3dDJmkjxiHUY/iCGkIfs4bsHW6gen1Axty1yo1cQKF5+bMvzFUBRA23sOC4il8HOhoo9YkBuUngvn9Hr13XdtIewIK6+86NdoWVGUpJnMfP2FspZIm7+0BfqngQYJCeq8gbMGsxkpxuAfp+im+you7l3BXYylYlE+j4A6zEUzr7uuAML5NJkaBegXS97bNIFSYob0eJUpRIPjZ6OPut1xStjZ+pRHTX0ayPYct1a2bocJGmEfN0Hws6hxbtSOCzzwIRLbmz3lyNwp7Yz1d6hgLctrvAijTnwnCN8I68rjwkGly8wjz3neLwwBsRojstR5MODThoJkw+dkSCwP1uN31g4nY6XiYCqEAdeGdSO0K+BJQOzkmW+cGbkxsYHeUI/5tCfngxLAbkCOviuMKAIE3LJqCyDZTZ3++fEiTT9d6RdeMDQ+VcnnjpAdzIexT5h9H6R1zn+nLWKo5tEYMHLJVldN4ojRRKJTxS5a5eM1FWwCt/p6q7B4wjacHJW6CS0Nr/XwlZFJ+AJG5TobstqvnG+U03JCaJNYeWIQ61gDZSFwjlIbEmwfy/c63h4KEYBsuPpVrHBw94oyVI9syZdGTnyr1xPJwJvhMilKe9JbMWB+l+ycJpZhL2f0DupUWx1jFkIDjdJD/uix7Lk1q9ixpFtorYnS3loipiqp9bwscKoWJ+ncVrLVTTb4qlTiS2fChst//DgcqFBbQ+O3JTUAO7aeUDHiD3JHfETq7wT68IwnfRNxuGUQbuOuBgzHI1WJ2kA642N+t0uQ9222GH7qLnK5SrzdtmBaV9zD8HUs8PjLeme8czICFWGuefKkxJY4KlLS55Q32Y5Qnk8P8niKzJCMSyMlEG6vUDAeWbYApepc5nOPMFDdijBUu4ZU3mFOYb+kmFuNLkbXsNOBu//YkBxnQx/rN3 0EKD4iwN p2ijWk4xAhZfy79MO16x6YNZ/ZFzuHeh1lvqSVyP309xvNKmnqFHyL+/NiuHjFt0QUSqJYsdLfCf1g11REVdc/oJOhdsdXlpEhsw6MZMADs5u9jLX2Zqx3ZwY/uLrV5d/oiPZIb00qTgEnjzvMJZ87JKW1wsrEFHQfeWmzvMVk+/RTFPWJT9fQrvDsw== 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: List-Subscribe: List-Unsubscribe: On Thu, Jun 27, 2024 at 11:18:56PM GMT, Jesper Dangaard Brouer wrote: > Avoid lock contention on the global cgroup rstat lock caused by kswapd > starting on all NUMA nodes simultaneously. At Cloudflare, we observed > massive issues due to kswapd and the specific mem_cgroup_flush_stats() > call inlined in shrink_node, which takes the rstat lock. > > On our 12 NUMA node machines, each with a kswapd kthread per NUMA node, > we noted severe lock contention on the rstat lock. This contention > causes 12 CPUs to waste cycles spinning every time kswapd runs. > Fleet-wide stats (/proc/N/schedstat) for kthreads revealed that we are > burning an average of 20,000 CPU cores fleet-wide on kswapd, primarily > due to spinning on the rstat lock. > > To help reviewer follow code: When the Per-CPU-Pages (PCP) freelist is > empty, Remove the "When the Per-CPU-Pages (PCP) freelist is empty" as there are a lot more conditions needed for the waking up kswapds which are not needed to be explained here. Just "__alloc_pages_slowpath waking up kswapds given the allocation context" or similar text should suffice. > __alloc_pages_slowpath calls wake_all_kswapds(), causing all > kswapdN threads to wake up simultaneously. The kswapd thread invokes > shrink_node (via balance_pgdat) triggering the cgroup rstat flush > operation as part of its work. This results in kernel self-induced rstat > lock contention by waking up all kswapd threads simultaneously. > Leveraging this detail: balance_pgdat() have NULL value in > target_mem_cgroup, this cause mem_cgroup_flush_stats() to do flush with > root_mem_cgroup. > > To avoid this kind of thundering herd problem, kernel previously had a > "stats_flush_ongoing" concept, but this was removed as part of commit > 7d7ef0a4686a ("mm: memcg: restore subtree stats flushing"). This patch > reintroduce and generalized the concept to apply to all users of cgroup > rstat, not just memcg. > > If there is an ongoing rstat flush, and current cgroup is a descendant, > then it is unnecessary to do the flush. For callers to still see updated > stats, wait for ongoing flusher to complete before returning, but add > timeout as stats are already inaccurate given updaters keeps running. > > Fixes: 7d7ef0a4686a ("mm: memcg: restore subtree stats flushing"). > Signed-off-by: Jesper Dangaard Brouer > --- > V3: https://lore.kernel.org/all/171943668946.1638606.1320095353103578332.stgit@firesoul/ > V2: https://lore.kernel.org/all/171923011608.1500238.3591002573732683639.stgit@firesoul/ > V1: https://lore.kernel.org/all/171898037079.1222367.13467317484793748519.stgit@firesoul/ > RFC: https://lore.kernel.org/all/171895533185.1084853.3033751561302228252.stgit@firesoul/ > > include/linux/cgroup-defs.h | 2 + > kernel/cgroup/rstat.c | 64 ++++++++++++++++++++++++++++++++++++------- > 2 files changed, 55 insertions(+), 11 deletions(-) > > diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h > index b36690ca0d3f..a33b37514c29 100644 > --- a/include/linux/cgroup-defs.h > +++ b/include/linux/cgroup-defs.h > @@ -548,6 +548,8 @@ struct cgroup { > #ifdef CONFIG_BPF_SYSCALL > struct bpf_local_storage __rcu *bpf_cgrp_storage; > #endif > + /* completion queue for cgrp_rstat_ongoing_flusher */ > + struct completion flush_done; > > /* All ancestors including self */ > struct cgroup *ancestors[]; > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > index 2a42be3a9bb3..a98af43bdce7 100644 > --- a/kernel/cgroup/rstat.c > +++ b/kernel/cgroup/rstat.c > @@ -2,6 +2,7 @@ > #include "cgroup-internal.h" > > #include > +#include > > #include > #include > @@ -11,6 +12,8 @@ > > static DEFINE_SPINLOCK(cgroup_rstat_lock); > static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock); > +static struct cgroup *cgrp_rstat_ongoing_flusher = NULL; > +static DECLARE_COMPLETION(cgrp_rstat_flusher_done); cgrp_rstat_flusher_done is not needed anymore. > > static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu); > > @@ -312,6 +315,45 @@ static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop) > spin_unlock_irq(&cgroup_rstat_lock); > } > > +#define MAX_WAIT msecs_to_jiffies(100) > +/* Trylock helper that also checks for on ongoing flusher */ > +static bool cgroup_rstat_trylock_flusher(struct cgroup *cgrp) > +{ > + bool locked = __cgroup_rstat_trylock(cgrp, -1); > + if (!locked) { > + struct cgroup *cgrp_ongoing; > + > + /* Lock is contended, lets check if ongoing flusher is already > + * taking care of this, if we are a descendant. > + */ > + cgrp_ongoing = READ_ONCE(cgrp_rstat_ongoing_flusher); > + if (cgrp_ongoing && cgroup_is_descendant(cgrp, cgrp_ongoing)) { I wonder if READ_ONCE() and cgroup_is_descendant() needs to happen within in rcu section. On a preemptable kernel, let's say we got preempted in between them, the flusher was unrelated and got freed before we get the CPU. In that case we are accessing freed memory. > + wait_for_completion_interruptible_timeout( > + &cgrp_ongoing->flush_done, MAX_WAIT); > + > + return false; > + } > + __cgroup_rstat_lock(cgrp, -1, false); > + } > + /* Obtained lock, record this cgrp as the ongoing flusher */ > + if (!READ_ONCE(cgrp_rstat_ongoing_flusher)) { Can the above condition will ever be false? > + reinit_completion(&cgrp->flush_done); > + WRITE_ONCE(cgrp_rstat_ongoing_flusher, cgrp); > + } > + > + return true; /* locked */ > +} > + > +static void cgroup_rstat_unlock_flusher(struct cgroup *cgrp) > +{ > + /* Detect if we are the ongoing flusher */ > + if (cgrp == READ_ONCE(cgrp_rstat_ongoing_flusher)) { Same. > + WRITE_ONCE(cgrp_rstat_ongoing_flusher, NULL); > + complete_all(&cgrp->flush_done); > + } > + __cgroup_rstat_unlock(cgrp, -1); > +} > + > /* see cgroup_rstat_flush() */ > static void cgroup_rstat_flush_locked(struct cgroup *cgrp) > __releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock) > @@ -361,18 +403,13 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp) > */ > __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp) > { > - bool locked; > - > might_sleep(); > > - locked = __cgroup_rstat_trylock(cgrp, -1); > - if (!locked) { > - /* Opportunity to ongoing flush detection */ > - __cgroup_rstat_lock(cgrp, -1, false); > - } > + if (!cgroup_rstat_trylock_flusher(cgrp)) > + return; > > cgroup_rstat_flush_locked(cgrp); > - __cgroup_rstat_unlock(cgrp, -1); > + cgroup_rstat_unlock_flusher(cgrp); > } > > /** > @@ -388,8 +425,11 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp) > __acquires(&cgroup_rstat_lock) > { > might_sleep(); > - __cgroup_rstat_lock(cgrp, -1, true); > - cgroup_rstat_flush_locked(cgrp); > + > + if (cgroup_rstat_trylock_flusher(cgrp)) > + cgroup_rstat_flush_locked(cgrp); > + else > + __cgroup_rstat_lock(cgrp, -1, true); > } > > /** > @@ -399,7 +439,7 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp) > void cgroup_rstat_flush_release(struct cgroup *cgrp) > __releases(&cgroup_rstat_lock) > { > - __cgroup_rstat_unlock(cgrp, -1); > + cgroup_rstat_unlock_flusher(cgrp); > } > > int cgroup_rstat_init(struct cgroup *cgrp) > @@ -421,6 +461,8 @@ int cgroup_rstat_init(struct cgroup *cgrp) > u64_stats_init(&rstatc->bsync); > } > > + init_completion(&cgrp->flush_done); > + > return 0; > } > > >