From: Yosry Ahmed <yosryahmed@google.com>
To: "Tejun Heo" <tj@kernel.org>, "Josef Bacik" <josef@toxicpanda.com>,
"Jens Axboe" <axboe@kernel.dk>,
"Zefan Li" <lizefan.x@bytedance.com>,
"Johannes Weiner" <hannes@cmpxchg.org>,
"Michal Hocko" <mhocko@kernel.org>,
"Roman Gushchin" <roman.gushchin@linux.dev>,
"Shakeel Butt" <shakeelb@google.com>,
"Muchun Song" <muchun.song@linux.dev>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Michal Koutný" <mkoutny@suse.com>
Cc: Vasily Averin <vasily.averin@linux.dev>,
cgroups@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
bpf@vger.kernel.org, Yosry Ahmed <yosryahmed@google.com>
Subject: [PATCH v2 5/9] memcg: replace stats_flush_lock with an atomic
Date: Tue, 28 Mar 2023 22:16:40 +0000 [thread overview]
Message-ID: <20230328221644.803272-6-yosryahmed@google.com> (raw)
In-Reply-To: <20230328221644.803272-1-yosryahmed@google.com>
As Johannes notes in [1], stats_flush_lock is currently used to:
(a) Protect updated to stats_flush_threshold.
(b) Protect updates to flush_next_time.
(c) Serializes calls to cgroup_rstat_flush() based on those ratelimits.
However:
1. stats_flush_threshold is already an atomic
2. flush_next_time is not atomic. The writer is locked, but the reader
is lockless. If the reader races with a flush, you could see this:
if (time_after(jiffies, flush_next_time))
spin_trylock()
flush_next_time = now + delay
flush()
spin_unlock()
spin_trylock()
flush_next_time = now + delay
flush()
spin_unlock()
which means we already can get flushes at a higher frequency than
FLUSH_TIME during races. But it isn't really a problem.
The reader could also see garbled partial updates, so it needs at
least READ_ONCE and WRITE_ONCE protection.
3. Serializing cgroup_rstat_flush() calls against the ratelimit
factors is currently broken because of the race in 2. But the race
is actually harmless, all we might get is the occasional earlier
flush. If there is no delta, the flush won't do much. And if there
is, the flush is justified.
So the lock can be removed all together. However, the lock also served
the purpose of preventing a thundering herd problem for concurrent
flushers, see [2]. Use an atomic instead to serve the purpose of
unifying concurrent flushers.
[1]https://lore.kernel.org/lkml/20230323172732.GE739026@cmpxchg.org/
[2]https://lore.kernel.org/lkml/20210716212137.1391164-2-shakeelb@google.com/
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
---
mm/memcontrol.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ff39f78f962e..65750f8b8259 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -585,8 +585,8 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
*/
static void flush_memcg_stats_dwork(struct work_struct *w);
static DECLARE_DEFERRABLE_WORK(stats_flush_dwork, flush_memcg_stats_dwork);
-static DEFINE_SPINLOCK(stats_flush_lock);
static DEFINE_PER_CPU(unsigned int, stats_updates);
+static atomic_t stats_flush_ongoing = ATOMIC_INIT(0);
static atomic_t stats_flush_threshold = ATOMIC_INIT(0);
static u64 flush_next_time;
@@ -636,15 +636,19 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
static void __mem_cgroup_flush_stats(void)
{
- unsigned long flag;
-
- if (!spin_trylock_irqsave(&stats_flush_lock, flag))
+ /*
+ * We always flush the entire tree, so concurrent flushers can just
+ * skip. This avoids a thundering herd problem on the rstat global lock
+ * from memcg flushers (e.g. reclaim, refault, etc).
+ */
+ if (atomic_read(&stats_flush_ongoing) ||
+ atomic_xchg(&stats_flush_ongoing, 1))
return;
- flush_next_time = jiffies_64 + 2*FLUSH_TIME;
+ WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME);
cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup);
atomic_set(&stats_flush_threshold, 0);
- spin_unlock_irqrestore(&stats_flush_lock, flag);
+ atomic_set(&stats_flush_ongoing, 0);
}
void mem_cgroup_flush_stats(void)
@@ -655,7 +659,7 @@ void mem_cgroup_flush_stats(void)
void mem_cgroup_flush_stats_ratelimited(void)
{
- if (time_after64(jiffies_64, flush_next_time))
+ if (time_after64(jiffies_64, READ_ONCE(flush_next_time)))
mem_cgroup_flush_stats();
}
--
2.40.0.348.gf938b09366-goog
next prev parent reply other threads:[~2023-03-28 22:17 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-28 22:16 [PATCH v2 0/9] memcg: make rstat flushing irq and sleep Yosry Ahmed
2023-03-28 22:16 ` [PATCH v2 1/9] cgroup: rename cgroup_rstat_flush_"irqsafe" to "atomic" Yosry Ahmed
2023-03-28 22:16 ` [PATCH v2 2/9] memcg: rename mem_cgroup_flush_stats_"delayed" to "ratelimited" Yosry Ahmed
2023-03-29 11:56 ` Michal Hocko
2023-03-28 22:16 ` [PATCH v2 3/9] memcg: do not flush stats in irq context Yosry Ahmed
2023-03-29 11:58 ` Michal Hocko
2023-03-28 22:16 ` [PATCH v2 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context Yosry Ahmed
2023-03-29 11:22 ` Michal Hocko
2023-03-29 18:41 ` Yosry Ahmed
2023-03-29 19:20 ` Shakeel Butt
2023-03-30 7:06 ` Michal Hocko
2023-03-30 7:13 ` Yosry Ahmed
2023-03-30 7:49 ` Michal Hocko
2023-03-30 8:06 ` Yosry Ahmed
2023-03-30 8:14 ` Michal Hocko
2023-03-30 8:19 ` Yosry Ahmed
2023-03-30 8:39 ` Michal Hocko
2023-03-30 8:53 ` Yosry Ahmed
2023-03-31 11:02 ` Michal Hocko
2023-03-31 19:03 ` Yosry Ahmed
2023-04-03 8:38 ` Michal Hocko
2023-04-03 20:39 ` Yosry Ahmed
2023-03-28 22:16 ` Yosry Ahmed [this message]
2023-03-28 22:22 ` [PATCH v2 5/9] memcg: replace stats_flush_lock with an atomic Shakeel Butt
2023-03-29 15:58 ` Michal Hocko
2023-03-29 18:45 ` Yosry Ahmed
2023-03-28 22:16 ` [PATCH v2 6/9] memcg: sleep during flushing stats in safe contexts Yosry Ahmed
2023-03-30 7:35 ` Michal Hocko
2023-03-28 22:16 ` [PATCH v2 7/9] workingset: memcg: sleep when flushing stats in workingset_refault() Yosry Ahmed
2023-03-30 7:39 ` Michal Hocko
2023-03-30 7:42 ` Yosry Ahmed
2023-03-30 7:50 ` Michal Hocko
2023-03-30 7:55 ` Yosry Ahmed
2023-03-28 22:16 ` [PATCH v2 8/9] vmscan: memcg: sleep when flushing stats during reclaim Yosry Ahmed
2023-03-30 7:40 ` Michal Hocko
2023-03-30 7:44 ` Yosry Ahmed
2023-03-30 7:52 ` Michal Hocko
2023-03-30 7:54 ` Yosry Ahmed
2023-03-28 22:16 ` [PATCH v2 9/9] memcg: do not modify rstat tree for zero updates Yosry Ahmed
2023-03-30 7:43 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230328221644.803272-6-yosryahmed@google.com \
--to=yosryahmed@google.com \
--cc=akpm@linux-foundation.org \
--cc=axboe@kernel.dk \
--cc=bpf@vger.kernel.org \
--cc=cgroups@vger.kernel.org \
--cc=hannes@cmpxchg.org \
--cc=josef@toxicpanda.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lizefan.x@bytedance.com \
--cc=mhocko@kernel.org \
--cc=mkoutny@suse.com \
--cc=muchun.song@linux.dev \
--cc=roman.gushchin@linux.dev \
--cc=shakeelb@google.com \
--cc=tj@kernel.org \
--cc=vasily.averin@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).