| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c98bd274-1af3-4d0d-8b14-fb524c954c1d@kernel.org>
Date: Mon, 24 Jun 2024 12:49:20 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Waiman Long <longman@...hat.com>, tj@...nel.org, cgroups@...r.kernel.org,
yosryahmed@...gle.com, shakeel.butt@...ux.dev
Cc: hannes@...xchg.org, lizefan.x@...edance.com, kernel-team@...udflare.com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] cgroup/rstat: Avoid thundering herd problem by kswapd
across NUMA nodes
On 21/06/2024 18.08, Waiman Long wrote:
>
> On 6/21/24 10:32, 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, __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 resolve the kswapd issue, we generalized the "stats_flush_ongoing"
>> concept to apply to all users of cgroup rstat, not just memcg. This
>> concept was originally reverted in commit 7d7ef0a4686a ("mm: memcg:
>> restore subtree stats flushing"). If there is an ongoing rstat flush,
>> limited to the root cgroup, the flush is skipped. This is effective as
>> kswapd operates on the root tree, sufficiently mitigating the thundering
>> herd problem.
>>
>> This lowers contention on the global rstat lock, although limited to the
>> root cgroup. Flushing cgroup subtree's can still lead to lock contention.
>>
>> Fixes: 7d7ef0a4686a ("mm: memcg: restore subtree stats flushing").
>> Signed-off-by: Jesper Dangaard Brouer <hawk@...nel.org>
>> ---
>> include/linux/cgroup.h | 5 +++++
>> kernel/cgroup/rstat.c | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 2150ca60394b..ad41cca5c3b6 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -499,6 +499,11 @@ static inline struct cgroup *cgroup_parent(struct
>> cgroup *cgrp)
>> return NULL;
>> }
>> +static inline bool cgroup_is_root(struct cgroup *cgrp)
>> +{
>> + return cgroup_parent(cgrp) == NULL;
>> +}
Please let me know if above code is correct?
>> +
>> /**
>> * cgroup_is_descendant - test ancestry
>> * @cgrp: the cgroup to be tested
>> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
>> index fb8b49437573..5aba95e92d31 100644
>> --- a/kernel/cgroup/rstat.c
>> +++ b/kernel/cgroup/rstat.c
>> @@ -11,6 +11,7 @@
>> static DEFINE_SPINLOCK(cgroup_rstat_lock);
>> static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock);
>> +static atomic_t root_rstat_flush_ongoing = ATOMIC_INIT(0);
>> static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu);
>> @@ -350,8 +351,25 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup
>> *cgrp)
>> {
>> might_sleep();
>> + /*
>> + * This avoids thundering herd problem on global rstat lock. When an
>> + * ongoing flush of the entire tree is in progress, then skip flush.
>> + */
>> + if (atomic_read(&root_rstat_flush_ongoing))
>> + return;
>> +
>> + /* Grab right to be ongoing flusher, return if loosing race */
>> + if (cgroup_is_root(cgrp) &&
>> + atomic_xchg(&root_rstat_flush_ongoing, 1))
>> + return;
>> +
>> __cgroup_rstat_lock(cgrp, -1);
>> +
>> cgroup_rstat_flush_locked(cgrp);
>> +
>> + if (cgroup_is_root(cgrp))
>> + atomic_set(&root_rstat_flush_ongoing, 0);
>> +
>> __cgroup_rstat_unlock(cgrp, -1);
>> }
>> @@ -362,13 +380,20 @@ __bpf_kfunc void cgroup_rstat_flush(struct
>> cgroup *cgrp)
>> * Flush stats in @cgrp's subtree and prevent further flushes. Must be
>> * paired with cgroup_rstat_flush_release().
>> *
>> + * Current invariant, not called with root cgrp.
>> + *
>> * This function may block.
>> */
>> void cgroup_rstat_flush_hold(struct cgroup *cgrp)
>> __acquires(&cgroup_rstat_lock)
>> {
>> might_sleep();
>> +
>> __cgroup_rstat_lock(cgrp, -1);
>> +
>> + if (atomic_read(&root_rstat_flush_ongoing))
>> + return;
>> +
>> cgroup_rstat_flush_locked(cgrp);
>> }
>> @@ -379,6 +404,9 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp)
>> void cgroup_rstat_flush_release(struct cgroup *cgrp)
>> __releases(&cgroup_rstat_lock)
>> {
>> + if (cgroup_is_root(cgrp))
>> + atomic_set(&root_rstat_flush_ongoing, 0);
>> +
>> __cgroup_rstat_unlock(cgrp, -1);
>> }
>
> Since both cgroup_rstat_flush_hold() and cgroup_rstat_flush_release()
> are not called with root cgroup, the cgroup_rstat_flush_hold() hunk is
> essentially dead code.
>
Yes, the cgroup_rstat_flush_release chunk is essentially dead code.
I will send a V2 with this code removed.
--Jesper
Powered by blists - more mailing lists