| Message ID | 171898037079.1222367.13467317484793748519.stgit@firesoul (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] cgroup/rstat: Avoid thundering herd problem by kswapd across NUMA nodes | expand |
Hold off applying this patch, as test kernel didn't boot with this patch applied on top of TJ's cgroup tree (on commit ec9eeb89e60d86). I don't know if this is related to this patch or not. --Jesper On 21/06/2024 16.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@kernel.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; > +} > + > /** > * 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); > } > > >
On 6/21/24 10:47, Jesper Dangaard Brouer wrote: > > Hold off applying this patch, as test kernel didn't boot with this patch > applied on top of TJ's cgroup tree (on commit ec9eeb89e60d86). I don't > know if this is related to this patch or not. Commit ec9eeb89e60d86 is just a snapshot of the Linus' tree. You should use for-next/for-6.11 tag as a base. Cheers, Longman > > --Jesper > > On 21/06/2024 16.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@kernel.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; >> +} >> + >> /** >> * 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); >> } >> >> >
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@kernel.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; > +} > + > /** > * 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. Cheers, Longman
On 21/06/2024 17.13, Waiman Long wrote: > On 6/21/24 10:47, Jesper Dangaard Brouer wrote: >> >> Hold off applying this patch, as test kernel didn't boot with this patch >> applied on top of TJ's cgroup tree (on commit ec9eeb89e60d86). I don't >> know if this is related to this patch or not. > > Commit ec9eeb89e60d86 is just a snapshot of the Linus' tree. You should > use for-next/for-6.11 tag as a base. Patch works fine when applied on branch for-6.11. (This snapshot of the Linus' tree seems to be at fault). I would still like feedback on, if this code (for detecting "root" cgrp) is correct? : static inline bool cgroup_is_root(struct cgroup *cgrp) { return cgroup_parent(cgrp) == NULL; } Next step for me is to backport and deploy this on a subset of production servers. --Jesper
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@kernel.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
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; +} + /** * 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); }
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@kernel.org> --- include/linux/cgroup.h | 5 +++++ kernel/cgroup/rstat.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)