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 00D53C3DA59 for ; Mon, 22 Jul 2024 17:03:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 55B486B0082; Mon, 22 Jul 2024 13:03:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 50BE76B0083; Mon, 22 Jul 2024 13:03:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3ACAA6B0085; Mon, 22 Jul 2024 13:03:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 1A90E6B0082 for ; Mon, 22 Jul 2024 13:03:42 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B70F2161941 for ; Mon, 22 Jul 2024 17:03:41 +0000 (UTC) X-FDA: 82368010242.27.9813AE8 Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by imf07.hostedemail.com (Postfix) with ESMTP id CD8034002F for ; Mon, 22 Jul 2024 17:03:39 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=XPFk0LME; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721667768; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GbfWvp9bRUygXfQ1AuZRcm8ZUwrv50CWtS6Lj5Lx694=; b=l16Rcu4QUX/ud67gtjGvJALB2MSxo5gGd6ExMLR1kosbuLxQJYFQ7ppWxwtr0Gqy+2XB9h VG1iGo0ddMVCUsKL9Ffl/vB91TcWcflAz0nIZAcvVa/Jt90nh4kmzyzZDrd6eKAVj+NcQp N/6Bmq5Swr2mLwAid7e4IdD/p1esZNA= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721667768; a=rsa-sha256; cv=none; b=nUabvTDLPsZo/2mqfT/8osHEmdAqAppw/769a9D49TG0iB1mRYnFKh8SubWgxQ2XtOo7TQ ft3v/gZwD9PB9nK9maADQ5S0N7anWu4d8asi94uC+FcvMujzqYDaABLxmxHgGvZd4cVM4P xhtfdJCm8gVwXgfDR1eDV8hVuvvjhjU= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=XPFk0LME; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-a7a8577fd84so52054566b.1 for ; Mon, 22 Jul 2024 10:03:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1721667818; x=1722272618; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=GbfWvp9bRUygXfQ1AuZRcm8ZUwrv50CWtS6Lj5Lx694=; b=XPFk0LMExmOANYy6OHq8eEh16o1PCbou7uB1SNPOxd4A4yk4z/uvTBI7at+qeTnaq6 QTOXTN9Pu3S9HkpEb7QkFYuoyU93WlSa5eE3jANE6TBd264mxZZywFI8b5Z+PJilA5uk w+nznNA+ZMZUnrAPmeCZ3OK8FzAhmLNiQh4moSAex+GaZbFx+tVFFeTMg/tibN84FLmX bvitDk/a6qYS3BiLJNTAQkyZYsZmleUVWRQ6i4v3N8q2C9BMflxP4nhPZPJh6ASSqBSw pvxVy8cMDO/HBy5jZBQFTcYgZONk3AQZmMOQZyWGDzYIiwEXs7D6XXZR139OGyJR6sfS yMKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721667818; x=1722272618; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=GbfWvp9bRUygXfQ1AuZRcm8ZUwrv50CWtS6Lj5Lx694=; b=UEqRZKPBlR2yxbo4Yy37oPxMwyUtMpPhvZHgJO0ceLteeLsiqe72IiIcV5KiY1nQYs qCKIq7nSc7jlpJKw9brhzaywoDhG73ueWZ+s9Qpg8ZmYBMBGt8TeWhLyTCZlqqgZs/PO c022bzkp1noyayY9w5Tlpa0fhEGLRYd47E6Hy04dYxUXRUuRyFqP6mxJRXW5CnI4Otp/ lK9+3JMcMaBBs1qFEDV0ssXgpn12mjxWOBRoPc2LZZkfyNdgjHHHRLYVnXjQi7Ygvt8h 9rdRL++mgBbkoNw6KlD9GwFrVgVUM2HNimdsj8D1skuo5tifVsiLAZBwMv9ut1BVOV5+ 75cw== X-Forwarded-Encrypted: i=1; AJvYcCW3s91qQu4jMQZTtZE8n6bWMvAPjsH2Im2E00CaeX/FISWD/m2cCpfT7X//QPTC7fbCcBssu9eNb6BI2kjHuxh5F8c= X-Gm-Message-State: AOJu0YwCrMellN2OMKvyCNZXp80S3oes9JAMoY8wakHAtrxDEy+eXBQZ 76qrr0Gn6PAbCrW8Dc2UB7hzQdESP8uw+2UQEYSe8bIF91pLDYn6n1UrGJRUbC2VJT11cQYmsEP UCV+qMXcKdfj7ZkyfAaZ65oTXeTLZa09YJkji X-Google-Smtp-Source: AGHT+IGaTaf10DT2IV1A6F1EsqODUWScWS2k60cFwJziHr6WMeA2nifgXlfXDp/clvtOP/Q5CMJl4saLOwNag32jDI8= X-Received: by 2002:a17:907:20f7:b0:a72:83ad:f087 with SMTP id a640c23a62f3a-a7a0f10fd08mr1447688566b.10.1721667817340; Mon, 22 Jul 2024 10:03:37 -0700 (PDT) MIME-Version: 1.0 References: <172139415725.3084888.13770938453137383953.stgit@firesoul> In-Reply-To: <172139415725.3084888.13770938453137383953.stgit@firesoul> From: Yosry Ahmed Date: Mon, 22 Jul 2024 10:02:59 -0700 Message-ID: Subject: Re: [PATCH V8 1/2] cgroup/rstat: Avoid flushing if there is an ongoing overlapping flush To: Jesper Dangaard Brouer Cc: tj@kernel.org, cgroups@vger.kernel.org, shakeel.butt@linux.dev, hannes@cmpxchg.org, lizefan.x@bytedance.com, longman@redhat.com, kernel-team@cloudflare.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: CD8034002F X-Stat-Signature: biwont7t1ug739dg6bma7ys5m98hwpd5 X-Rspam-User: X-HE-Tag: 1721667819-848001 X-HE-Meta: U2FsdGVkX1/cyS4g80bSMTE8XzFOOti0zLc5Ri/OqCRlPlsmCuBBON/ZMRJ3Echb+WAImZZYe15fQotsuZaVSVaVppyLKnvL376qTgvk6H/QY5DRa20KaYXNY1bALMv/0u0Ce+DK8nLBti7m/HlJfV7S9eyOuBpaMTNCoEKDV92FZ3qy+nEBlEHpvSVj9PYFQ8J8KJS9+ccrEKCALoVDveyDdjD+z9y2NQpZcJHJ5yfw38E/2xCA8oZueEoQW1uc6IIqP7h4EipvV9e/I85YuDSeoFXWDVEy3QlNrvTqOB9HftRS9LoULPK9maY7QemrLmXhyedWTVQ9SyRYyz16OTk7qiN5nsMwObuKM8h9gaWTn44Fnb0oDlzZtJEyRACJlFMRvKaZ9QQJxvlrSjKwZr87WTkvQhl0mrwS6INf6aoPMd6SbcVG0XwGFQK80XIV/JZ70GIL7rVsIZ2he2h+dJWM4S74/X2PqT8hP0SP81Y87uMnVZRmvfjSY9CxKA3LqRQuPUR3/Hbcbzn2MVMp3UE6SnEg8if5TqnB1bW77wcvg8no0B9aNL6D0pgNPs1IyQhtRPkZAzXzAj44Cq9o/T4AVuOn3I9MXbMRvshnSEbbIUiJrvOYjrYcm84dcD0m5za6vy/n2d4F3YMjlepWBdREP50rNEgsSeH3msdwL1bTVJpn5YZpk1JeujSLf3parDn3SeaUSET0xJK2pH8XPcxWoE98dL58nnloh+ovdJVFJ2Kb9gaHzCjaJCifx5dhUvroeDgi/yQsjO5OOVKiNH93NpJP9Iv2/8UrWS8HH01/jLsL3wm0ofY6wcXTUUcqmx+rMpq6ZAF8jCLcXM++cqUM4JMEzjGAjYMxRfLgBWUKjkizDaHOhqQHtG+7cOUzEgjlb1LUDqTaQopQVS3bVH8T/GW20q1TuRGbYROPbmKF0k7O7G9VdiY33XNvm94+rMQz9fAPPmSacQi8UdB 8Mbc8lx2 KmpasZMvDTeXokUJKatxKL6EyZG/Bt0z5gdncr7iOvvAEBeKdXUO/Uz0sTbEHYjmG6CUeJj5b+MU2hmV1DCSaQzpSkNneDC1vBcisIg6exYd6Ki9byKVErjlMLsqJP8CXaPmcuP5WhgWPEwfcfRIHQEnee5xQUK0w1i/zUtDMMTsLPuorGm32KKpMmpgmhp0qMNT3gmDTUibITPZpAIHteS8HyNzjitgI4PzFf7ZNWC1dVnA= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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 Fri, Jul 19, 2024 at 6:06=E2=80=AFAM Jesper Dangaard Brouer wrote: > > This patch reintroduces and generalizes the "stats_flush_ongoing" concept > to avoid redundant flushes if there is an ongoing flush, addressing lock > contention issues on the global cgroup rstat lock. > > At Cloudflare, we observed significant performance degradation due to > lock contention on the rstat lock, primarily caused by kswapd. The > specific mem_cgroup_flush_stats() call inlined in shrink_node, which > takes the rstat lock, is particularly problematic. > > On our 12 NUMA node machines, each with a kswapd kthread per NUMA node, w= e > noted severe lock contention on the rstat lock, causing 12 CPUs to waste > cycles spinning every time kswapd runs. Fleet-wide stats (/proc/N/schedst= at) > 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. > > Here's a brief overview of the issue: > - __alloc_pages_slowpath calls wake_all_kswapds, causing all kswapdN thre= ads > to wake up simultaneously. > - The kswapd thread invokes shrink_node (via balance_pgdat), triggering t= he > cgroup rstat flush operation as part of its work. > - balance_pgdat() has a NULL value in target_mem_cgroup, causing > mem_cgroup_flush_stats() to flush with root_mem_cgroup. > > The kernel previously addressed this with a "stats_flush_ongoing" concept= , > which was removed in commit 7d7ef0a4686a ("mm: memcg: restore subtree sta= ts > flushing"). This patch reintroduces and generalizes the concept to apply = to > all users of cgroup rstat, not just memcg. > > If there is an ongoing rstat flush and the current cgroup is a descendant= , a > new flush is unnecessary. To ensure callers still receive updated stats, > they wait for the ongoing flush to complete before returning, but with a > timeout, as stats may already be inaccurate due to continuous updates. > > Lock yielding causes complications for ongoing flushers. Therefore, we li= mit > which cgroup can become ongoing flusher to top-level, as lock yielding > allows others to obtain the lock without being the ongoing flusher, leadi= ng > to a situation where a cgroup that isn't a descendant obtains the lock vi= a > yielding. Thus, we prefer an ongoing flusher with many descendants. If an= d > when the lock yielding is removed, such as when changing this to a mutex, > we can simplify this code. > > This change significantly reduces lock contention, especially in > environments with multiple NUMA nodes, thereby improving overall system > performance. > > Fixes: 7d7ef0a4686a ("mm: memcg: restore subtree stats flushing"). > Signed-off-by: Jesper Dangaard Brouer > --- > V8: > - Updated subject+desc based on Yosry's feedback > - Explain lock yielding challenges in comments > - Limit ongoing flushers to cgrp level 0 and 1 > > V7: https://lore.kernel.org/all/172070450139.2992819.13210624094367257881= .stgit@firesoul > V6: https://lore.kernel.org/all/172052399087.2357901.4955042377343593447.= stgit@firesoul/ > V5: https://lore.kernel.org/all/171956951930.1897969.8709279863947931285.= stgit@firesoul/ > V4: https://lore.kernel.org/all/171952312320.1810550.13209360603489797077= .stgit@firesoul/ > 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 | 114 +++++++++++++++++++++++++++++++++++++= +----- > 2 files changed, 104 insertions(+), 12 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 fb8b49437573..eaa138f2da2f 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,7 @@ > > static DEFINE_SPINLOCK(cgroup_rstat_lock); > static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock); > +static struct cgroup *cgrp_rstat_ongoing_flusher =3D NULL; > > static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu); > > @@ -279,17 +281,32 @@ __bpf_hook_end(); > * value -1 is used when obtaining the main lock else this is the CPU > * number processed last. > */ > -static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_l= oop) > +static inline bool __cgroup_rstat_trylock(struct cgroup *cgrp, int cpu_i= n_loop) > +{ > + bool locked; > + > + locked =3D spin_trylock_irq(&cgroup_rstat_lock); > + if (!locked) > + trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, true= ); > + else > + trace_cgroup_rstat_locked(cgrp, cpu_in_loop, false); > + > + return locked; > +} > + > +static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_l= oop, > + bool already_contended) > __acquires(&cgroup_rstat_lock) > { > - bool contended; > + bool locked =3D false; > > - contended =3D !spin_trylock_irq(&cgroup_rstat_lock); > - if (contended) { > - trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, cont= ended); > + if (already_contended) /* Skip trylock if already contended */ > + locked =3D __cgroup_rstat_trylock(cgrp, cpu_in_loop); Should this be the other way around? > + > + if (!locked) { > spin_lock_irq(&cgroup_rstat_lock); > + trace_cgroup_rstat_locked(cgrp, cpu_in_loop, true); > } > - trace_cgroup_rstat_locked(cgrp, cpu_in_loop, contended); > } > > static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in= _loop) > @@ -299,6 +316,72 @@ static inline void __cgroup_rstat_unlock(struct cgro= up *cgrp, int cpu_in_loop) > spin_unlock_irq(&cgroup_rstat_lock); > } > > +#define MAX_WAIT msecs_to_jiffies(100) > +/** > + * cgroup_rstat_trylock_flusher - Trylock that checks for on ongoing flu= sher > + * @cgrp: target cgroup > + * > + * Function return value follow trylock semantics. Returning true when l= ock is > + * obtained. Returning false when not locked and it detected flushing ca= n be > + * skipped as another ongoing flusher took care of the flush. > + */ > +static bool cgroup_rstat_trylock_flusher(struct cgroup *cgrp) > +{ > + struct cgroup *ongoing; > + bool locked; > + > + /* > + * Check if ongoing flusher is already taking care of this, if > + * we are a descendant skip work, but wait for ongoing flusher > + * to complete work. > + */ > +retry: > + ongoing =3D READ_ONCE(cgrp_rstat_ongoing_flusher); > + if (ongoing && cgroup_is_descendant(cgrp, ongoing)) { The discussion about cgrp_rstat_ongoing_flusher possibly going away in parallel never reached a conclusion AFAICT. Should we use cgroup_tryget() here to get a ref on 'ongoing' until wait completes? This shouldn't add much complexity AFAICT. I think just using RCU here wouldn't be enough as we can flush rstat after the RCU grace period when a cgroup is being freed. > + wait_for_completion_interruptible_timeout( > + &ongoing->flush_done, MAX_WAIT); > + /* TODO: Add tracepoint here */ > + return false; > + } > + > + locked =3D __cgroup_rstat_trylock(cgrp, -1); > + if (!locked) { > + /* Contended: Handle losing race for ongoing flusher */ > + if (!ongoing && READ_ONCE(cgrp_rstat_ongoing_flusher)) > + goto retry; > + > + __cgroup_rstat_lock(cgrp, -1, true); > + } > + /* > + * Obtained lock, record this cgrp as the ongoing flusher. > + * Due to lock yielding, we might obtain lock while another > + * ongoing flusher (that isn't a parent) owns ongoing_flusher. > + */ > + ongoing =3D READ_ONCE(cgrp_rstat_ongoing_flusher); > + if (!ongoing) { I think we don't need protection here since we never dereference 'cgrp_rstat_ongoing_flusher', but I think it may be clearer to directly check it to make this obvious: if (!READ_ONCE(cgrp_rstat_ongoing_flusher)) { Perhaps we can also explicitly mention in the comment why we do not need any protection here, but I am not sure how helpful that will be. > + /* > + * Limit to top-level as lock yielding allows others to o= btain > + * lock without being ongoing_flusher. Leading to cgroup = that > + * isn't descendant to obtain lock via yielding. So, pref= er > + * ongoing_flusher with many descendants. > + */ > + if (cgrp->level < 2) { This covers roots and top-level cgroups under them, right? Did them improve the numbers you were observing? AFAICT, we can remove this restriction completely if/when we use a mutex and support a single ongoing flusher. If so, let's explicitly mention this, perhaps: XXX: Remove this restriction if/when lock yielding is removed > + reinit_completion(&cgrp->flush_done); > + WRITE_ONCE(cgrp_rstat_ongoing_flusher, cgrp); > + } > + } > + return true; > +} > + > +static void cgroup_rstat_unlock_flusher(struct cgroup *cgrp) > +{ > + if (cgrp =3D=3D READ_ONCE(cgrp_rstat_ongoing_flusher)) { > + 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) > @@ -328,7 +411,7 @@ static void cgroup_rstat_flush_locked(struct cgroup *= cgrp) > __cgroup_rstat_unlock(cgrp, cpu); > if (!cond_resched()) > cpu_relax(); > - __cgroup_rstat_lock(cgrp, cpu); > + __cgroup_rstat_lock(cgrp, cpu, false); > } > } > } > @@ -350,9 +433,11 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *c= grp) > { > might_sleep(); > > - __cgroup_rstat_lock(cgrp, -1); > + if (!cgroup_rstat_trylock_flusher(cgrp)) > + return; > + > cgroup_rstat_flush_locked(cgrp); > - __cgroup_rstat_unlock(cgrp, -1); > + cgroup_rstat_unlock_flusher(cgrp); > } > > /** > @@ -368,8 +453,11 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp) > __acquires(&cgroup_rstat_lock) > { > might_sleep(); > - __cgroup_rstat_lock(cgrp, -1); > - cgroup_rstat_flush_locked(cgrp); > + > + if (cgroup_rstat_trylock_flusher(cgrp)) > + cgroup_rstat_flush_locked(cgrp); > + else > + __cgroup_rstat_lock(cgrp, -1, true); > } > > /** > @@ -379,7 +467,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) > @@ -401,6 +489,8 @@ int cgroup_rstat_init(struct cgroup *cgrp) > u64_stats_init(&rstatc->bsync); > } > > + init_completion(&cgrp->flush_done); > + > return 0; > } > > > >