At the moment, the kernel flushes the memcg stats on every refault and
also on every reclaim iteration. Although rstat maintains per-cpu update
tree but on the flush the kernel still has to go through all the cpu rstat
update tree to check if there is anything to flush. This patch adds the
tracking on the stats update side to make flush side more clever by
skipping the flush if there is no update.
The stats update codepath is very sensitive performance wise for many
workloads and benchmarks. So, we can not follow what the commit
aa48e47e3906 ("memcg: infrastructure to flush memcg stats") did which was
triggering async flush through queue_work() and caused a lot performance
regression reports. That got reverted by the commit 1f828223b799 ("memcg:
flush lruvec stats in the refault").
In this patch we kept the stats update codepath very minimal and let the
stats reader side to flush the stats only when the updates are over a
specific threshold. For now the threshold is (nr_cpus * CHARGE_BATCH).
To evaluate the impact of this patch, an 8 GiB tmpfs file is created on a
system with swap-on-zram and the file was pushed to swap through
memory.force_empty interface. On reading the whole file, the memcg stat
flush in the refault code path is triggered. With this patch, we observed
63% reduction in the read time of 8 GiB file.
Signed-off-by: Shakeel Butt <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: "Michal Koutný" <[email protected]>
Cc: Andrew Morton <[email protected]>
---
Changelog since v1:
- Moved code and comment added as suggested by Johannes Weiner
mm/memcontrol.c | 78 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 23 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7c9d5703700e..25f55636ca37 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -103,11 +103,6 @@ static bool do_memsw_account(void)
return !cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_noswap;
}
-/* memcg and lruvec stats flushing */
-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);
-
#define THRESHOLDS_EVENTS_TARGET 128
#define SOFTLIMIT_EVENTS_TARGET 1024
@@ -613,6 +608,56 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
return mz;
}
+/*
+ * memcg and lruvec stats flushing
+ *
+ * Many codepaths leading to stats update or read are performance sensitive and
+ * adding stats flushing in such codepaths is not desirable. So, to optimize the
+ * flushing the kernel does:
+ *
+ * 1) Periodically and asynchronously flush the stats every 2 seconds to not let
+ * rstat update tree grow unbounded.
+ *
+ * 2) Flush the stats synchronously on reader side only when there are more than
+ * (MEMCG_CHARGE_BATCH * nr_cpus) update events. Though this optimization
+ * will let stats be out of sync by atmost (MEMCG_CHARGE_BATCH * nr_cpus) but
+ * only for 2 seconds due to (1).
+ */
+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_threshold = ATOMIC_INIT(0);
+
+static inline void memcg_rstat_updated(struct mem_cgroup *memcg)
+{
+ cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
+ if (!(__this_cpu_inc_return(stats_updates) % MEMCG_CHARGE_BATCH))
+ atomic_inc(&stats_flush_threshold);
+}
+
+static void __mem_cgroup_flush_stats(void)
+{
+ if (!spin_trylock(&stats_flush_lock))
+ return;
+
+ cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup);
+ atomic_set(&stats_flush_threshold, 0);
+ spin_unlock(&stats_flush_lock);
+}
+
+void mem_cgroup_flush_stats(void)
+{
+ if (atomic_read(&stats_flush_threshold) > num_online_cpus())
+ __mem_cgroup_flush_stats();
+}
+
+static void flush_memcg_stats_dwork(struct work_struct *w)
+{
+ mem_cgroup_flush_stats();
+ queue_delayed_work(system_unbound_wq, &stats_flush_dwork, 2UL*HZ);
+}
+
/**
* __mod_memcg_state - update cgroup memory statistics
* @memcg: the memory cgroup
@@ -625,7 +670,7 @@ void __mod_memcg_state(struct mem_cgroup *memcg, int idx, int val)
return;
__this_cpu_add(memcg->vmstats_percpu->state[idx], val);
- cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
+ memcg_rstat_updated(memcg);
}
/* idx can be of type enum memcg_stat_item or node_stat_item. */
@@ -653,10 +698,12 @@ void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
memcg = pn->memcg;
/* Update memcg */
- __mod_memcg_state(memcg, idx, val);
+ __this_cpu_add(memcg->vmstats_percpu->state[idx], val);
/* Update lruvec */
__this_cpu_add(pn->lruvec_stats_percpu->state[idx], val);
+
+ memcg_rstat_updated(memcg);
}
/**
@@ -758,7 +805,7 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
return;
__this_cpu_add(memcg->vmstats_percpu->events[idx], count);
- cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
+ memcg_rstat_updated(memcg);
}
static unsigned long memcg_events(struct mem_cgroup *memcg, int event)
@@ -5342,21 +5389,6 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
memcg_wb_domain_size_changed(memcg);
}
-void mem_cgroup_flush_stats(void)
-{
- if (!spin_trylock(&stats_flush_lock))
- return;
-
- cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup);
- spin_unlock(&stats_flush_lock);
-}
-
-static void flush_memcg_stats_dwork(struct work_struct *w)
-{
- mem_cgroup_flush_stats();
- queue_delayed_work(system_unbound_wq, &stats_flush_dwork, 2UL*HZ);
-}
-
static void mem_cgroup_css_rstat_flush(struct cgroup_subsys_state *css, int cpu)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
--
2.33.0.800.g4c38ced690-goog
The memcg stats can be flushed in multiple context and potentially in
parallel too. For example multiple parallel user space readers for memcg
stats will contend on the rstat locks with each other. There is no need
for that. We just need one flusher and everyone else can benefit. In
addition after aa48e47e3906 ("memcg: infrastructure to flush memcg stats")
the kernel periodically flush the memcg stats from the root, so, the other
flushers will potentially have much less work to do.
Signed-off-by: Shakeel Butt <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: "Michal Koutný" <[email protected]>
Cc: Andrew Morton <[email protected]>
---
Changelog since v1:
- N/A
mm/memcontrol.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 25f55636ca37..22d905f30a30 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -638,12 +638,14 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg)
static void __mem_cgroup_flush_stats(void)
{
- if (!spin_trylock(&stats_flush_lock))
+ unsigned long flag;
+
+ if (!spin_trylock_irqsave(&stats_flush_lock, flag))
return;
cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup);
atomic_set(&stats_flush_threshold, 0);
- spin_unlock(&stats_flush_lock);
+ spin_unlock_irqrestore(&stats_flush_lock, flag);
}
void mem_cgroup_flush_stats(void)
@@ -1462,7 +1464,7 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
*
* Current memory state:
*/
- cgroup_rstat_flush(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
for (i = 0; i < ARRAY_SIZE(memory_stats); i++) {
u64 size;
@@ -3566,8 +3568,7 @@ static unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
unsigned long val;
if (mem_cgroup_is_root(memcg)) {
- /* mem_cgroup_threshold() calls here from irqsafe context */
- cgroup_rstat_flush_irqsafe(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
val = memcg_page_state(memcg, NR_FILE_PAGES) +
memcg_page_state(memcg, NR_ANON_MAPPED);
if (swap)
@@ -3948,7 +3949,7 @@ static int memcg_numa_stat_show(struct seq_file *m, void *v)
int nid;
struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
- cgroup_rstat_flush(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
for (stat = stats; stat < stats + ARRAY_SIZE(stats); stat++) {
seq_printf(m, "%s=%lu", stat->name,
@@ -4020,7 +4021,7 @@ static int memcg_stat_show(struct seq_file *m, void *v)
BUILD_BUG_ON(ARRAY_SIZE(memcg1_stat_names) != ARRAY_SIZE(memcg1_stats));
- cgroup_rstat_flush(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
for (i = 0; i < ARRAY_SIZE(memcg1_stats); i++) {
unsigned long nr;
@@ -4523,7 +4524,7 @@ void mem_cgroup_wb_stats(struct bdi_writeback *wb, unsigned long *pfilepages,
struct mem_cgroup *memcg = mem_cgroup_from_css(wb->memcg_css);
struct mem_cgroup *parent;
- cgroup_rstat_flush_irqsafe(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
*pdirty = memcg_page_state(memcg, NR_FILE_DIRTY);
*pwriteback = memcg_page_state(memcg, NR_WRITEBACK);
@@ -6408,7 +6409,7 @@ static int memory_numa_stat_show(struct seq_file *m, void *v)
int i;
struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
- cgroup_rstat_flush(memcg->css.cgroup);
+ mem_cgroup_flush_stats();
for (i = 0; i < ARRAY_SIZE(memory_stats); i++) {
int nid;
--
2.33.0.800.g4c38ced690-goog
Hello Shakeel.
(Sorry for taking so long getting down to this.)
On Fri, Oct 01, 2021 at 12:00:40PM -0700, Shakeel Butt <[email protected]> wrote:
> There is no need for that. We just need one flusher and everyone else
> can benefit.
I imagine a cgroup with an intricate deep hiearchy with many updates and
a separate (simpler) sibling/independent cgroup that would need to pay
the costs of the first hierarchy updates [1] when it asks just for its
own stats (bound by the amount that's leftover from the periodic
updates).
The stats files (or wb stats) are likely not that time sensitive and the
reclaim (that can be local only but is slow path anyway) already uses
the global flushing.
I wonder whether the bigger benefit would be to retain the global
stats_flush_threshold counter but flush only local subtree.
Thanks,
Michal
[1] At first I thought non-memcg updates would interfere too via rstat
tree but I see it's actually filtered with the stats_flush_threshold so
only foreign memcg updates are relevant.
On Fri, Oct 01, 2021 at 12:00:39PM -0700, Shakeel Butt <[email protected]> wrote:
> In this patch we kept the stats update codepath very minimal and let the
> stats reader side to flush the stats only when the updates are over a
> specific threshold. For now the threshold is (nr_cpus * CHARGE_BATCH).
BTW, a noob question -- are the updates always single page sized?
This is motivated by apples vs oranges comparison since the
nr_cpus * MEMCG_CHARGE_BATCH
suggests what could the expected error be in pages (bytes). But it's mostly
wrong since: a) uncertain single-page updates, b) various counter
updates summed together. I wonder whether the formula can serve to
provide at least some (upper) estimate.
> +static inline void memcg_rstat_updated(struct mem_cgroup *memcg)
> +{
> + cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
> + if (!(__this_cpu_inc_return(stats_updates) % MEMCG_CHARGE_BATCH))
> + atomic_inc(&stats_flush_threshold);
> +}
Neat trick! (I guess there are no benchmarks complaining about this
(yet)).
Overall,
Reviewed-by: Michal Koutn? <[email protected]>
On Wed, Oct 13, 2021 at 11:01 AM Michal Koutný <[email protected]> wrote:
>
> Hello Shakeel.
>
> (Sorry for taking so long getting down to this.)
>
> On Fri, Oct 01, 2021 at 12:00:40PM -0700, Shakeel Butt <[email protected]> wrote:
> > There is no need for that. We just need one flusher and everyone else
> > can benefit.
>
> I imagine a cgroup with an intricate deep hiearchy with many updates and
> a separate (simpler) sibling/independent cgroup that would need to pay
> the costs of the first hierarchy updates [1] when it asks just for its
> own stats (bound by the amount that's leftover from the periodic
> updates).
>
> The stats files (or wb stats) are likely not that time sensitive and the
> reclaim (that can be local only but is slow path anyway) already uses
> the global flushing.
>
> I wonder whether the bigger benefit would be to retain the global
> stats_flush_threshold counter but flush only local subtree.
I did contemplate on this (i.e. a stat read paying the flushing price
for everyone else) but decided to keep as is based on:
1) The periodic async flush will keep the update tree small and will
keep infrequent readers cheap.
2) Keep things simple for now and come back if someone complains for
very frequent stats readers.
Shakeel
On Wed, Oct 13, 2021 at 12:24:31PM -0700, Shakeel Butt <[email protected]> wrote:
> 1) The periodic async flush will keep the update tree small and will
> keep infrequent readers cheap.
> 2) Keep things simple for now and come back if someone complains for
> very frequent stats readers.
OK, understood.
Michal
Hi Michal,
On Wed, Oct 13, 2021 at 11:01 AM Michal Koutný <[email protected]> wrote:
>
> On Fri, Oct 01, 2021 at 12:00:39PM -0700, Shakeel Butt <[email protected]> wrote:
> > In this patch we kept the stats update codepath very minimal and let the
> > stats reader side to flush the stats only when the updates are over a
> > specific threshold. For now the threshold is (nr_cpus * CHARGE_BATCH).
>
> BTW, a noob question -- are the updates always single page sized?
>
> This is motivated by apples vs oranges comparison since the
> nr_cpus * MEMCG_CHARGE_BATCH
> suggests what could the expected error be in pages (bytes). But it's mostly
> wrong since: a) uncertain single-page updates, b) various counter
> updates summed together. I wonder whether the formula can serve to
> provide at least some (upper) estimate.
>
Thanks for your review. This forces me to think more on this because each
update does not necessarily be a single page sized update e.g. adding a hugepage
to an LRU.
Though I think the error is time bounded by 2 seconds but in those 2 seconds
mathematically the error can be large. What do you think of the following
change? It will bound the error better within the 2 seconds window.
From e87a36eedd02b0d10d8f66f83833bd6e2bae17b8 Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Thu, 14 Oct 2021 08:49:06 -0700
Subject: [PATCH] Better bounds on the stats error
---
mm/memcontrol.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8f1d9c028897..e5d5c850a521 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -626,14 +626,20 @@ 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 DEFINE_PER_CPU(int, stats_diff);
static atomic_t stats_flush_threshold = ATOMIC_INIT(0);
-static inline void memcg_rstat_updated(struct mem_cgroup *memcg)
+static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
{
+ unsigned int x;
+
cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
- if (!(__this_cpu_inc_return(stats_updates) % MEMCG_CHARGE_BATCH))
- atomic_inc(&stats_flush_threshold);
+
+ x = abs(__this_cpu_add_return(stats_diff, val));
+ if (x > MEMCG_CHARGE_BATCH) {
+ atomic_add(x / MEMCG_CHARGE_BATCH, &stats_flush_threshold);
+ __this_cpu_write(stats_diff, 0);
+ }
}
static void __mem_cgroup_flush_stats(void)
@@ -672,7 +678,7 @@ void __mod_memcg_state(struct mem_cgroup *memcg, int idx, int val)
return;
__this_cpu_add(memcg->vmstats_percpu->state[idx], val);
- memcg_rstat_updated(memcg);
+ memcg_rstat_updated(memcg, val);
}
/* idx can be of type enum memcg_stat_item or node_stat_item. */
@@ -705,7 +711,7 @@ void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
/* Update lruvec */
__this_cpu_add(pn->lruvec_stats_percpu->state[idx], val);
- memcg_rstat_updated(memcg);
+ memcg_rstat_updated(memcg, val);
}
/**
@@ -807,7 +813,7 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
return;
__this_cpu_add(memcg->vmstats_percpu->events[idx], count);
- memcg_rstat_updated(memcg);
+ memcg_rstat_updated(memcg, val);
}
static unsigned long memcg_events(struct mem_cgroup *memcg, int event)
--
2.33.0.882.g93a45727a2-goog
On Thu, Oct 14, 2021 at 09:31:46AM -0700, Shakeel Butt <[email protected]> wrote:
> Thanks for your review. This forces me to think more on this because each
> update does not necessarily be a single page sized update e.g. adding a hugepage
> to an LRU.
Aha, hugepages... (I also noticed that on the opposite size scale are
NR_SLAB_{UN,}RECLAIMABLE_B, the complementary problem to too big error
would be too frequent flushes.)
> Though I think the error is time bounded by 2 seconds but in those 2 seconds
> mathematically the error can be large.
Honestly, I can't tell how much the (transient) errors in various
node_stat_item entries will or won't affect MM behavior. But having some
guards on it sounds practical when some problems to troubleshoot arise.
> What do you think of the following change? It will bound the error
> better within the 2 seconds window.
> [...]
> -static inline void memcg_rstat_updated(struct mem_cgroup *memcg)
> +static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
> {
> + unsigned int x;
> +
> cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
> - if (!(__this_cpu_inc_return(stats_updates) % MEMCG_CHARGE_BATCH))
> - atomic_inc(&stats_flush_threshold);
> +
> + x = abs(__this_cpu_add_return(stats_diff, val));
> + if (x > MEMCG_CHARGE_BATCH) {
> + atomic_add(x / MEMCG_CHARGE_BATCH, &stats_flush_threshold);
> + __this_cpu_write(stats_diff, 0);
> + }
> }
Looks better wrt meaningful error calculation (and hopefully still
doesn't add too much to the hot path).
> @@ -807,7 +813,7 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
> return;
>
> __this_cpu_add(memcg->vmstats_percpu->events[idx], count);
> - memcg_rstat_updated(memcg);
> + memcg_rstat_updated(memcg, val);
s/val/count/
(Just thinking loudly.) At one moment I thought, it could
effectively be even s/val/0/ since events aren't(?) inputs for reclaim
calculations. But with the introduced stats_diff it may happen
stats_diff flickers (its abs value) within the MEMCG_CHARGE_BATCH and
stats_flush_threshold would never be incremented. Basically disabling
the periodic flush. Therefore the events must also increment the
stats_diff.
On Thu, 14 Oct 2021 09:31:46 -0700 Shakeel Butt <[email protected]> wrote:
> Hi Michal,
>
> On Wed, Oct 13, 2021 at 11:01 AM Michal Koutn? <[email protected]> wrote:
> >
> > On Fri, Oct 01, 2021 at 12:00:39PM -0700, Shakeel Butt <[email protected]> wrote:
> > > In this patch we kept the stats update codepath very minimal and let the
> > > stats reader side to flush the stats only when the updates are over a
> > > specific threshold. ?For now the threshold is (nr_cpus * CHARGE_BATCH).
> >
> > BTW, a noob question -- are the updates always single page sized?
> >
> > This is motivated by apples vs oranges comparison since the
> > ? ? ? ? nr_cpus * MEMCG_CHARGE_BATCH
> > suggests what could the expected error be in pages (bytes). But it's mostly
> > wrong since: a) uncertain single-page updates, b) various counter
> > updates summed together. I wonder whether the formula can serve to
> > provide at least some (upper) estimate.
> >
>
> Thanks for your review. This forces me to think more on this because each
> update does not necessarily be a single page sized update e.g. adding a hugepage
> to an LRU.
>
> Though I think the error is time bounded by 2 seconds but in those 2 seconds
> mathematically the error can be large.
Sounds significant?
> What do you think of the following
> change? It will bound the error better within the 2 seconds window.
This didn't seem to go anywhere. I'll send "memcg: flush stats only if
updated" Linuswards, but please remember to resurrect this idea soonish
(this month?) if you think such a change is desirable.
On Thu, Nov 4, 2021 at 2:27 PM Andrew Morton <[email protected]> wrote:
>
> On Thu, 14 Oct 2021 09:31:46 -0700 Shakeel Butt <[email protected]> wrote:
>
> > Hi Michal,
> >
> > On Wed, Oct 13, 2021 at 11:01 AM Michal Koutný <[email protected]> wrote:
> > >
> > > On Fri, Oct 01, 2021 at 12:00:39PM -0700, Shakeel Butt <[email protected]> wrote:
> > > > In this patch we kept the stats update codepath very minimal and let the
> > > > stats reader side to flush the stats only when the updates are over a
> > > > specific threshold. For now the threshold is (nr_cpus * CHARGE_BATCH).
> > >
> > > BTW, a noob question -- are the updates always single page sized?
> > >
> > > This is motivated by apples vs oranges comparison since the
> > > nr_cpus * MEMCG_CHARGE_BATCH
> > > suggests what could the expected error be in pages (bytes). But it's mostly
> > > wrong since: a) uncertain single-page updates, b) various counter
> > > updates summed together. I wonder whether the formula can serve to
> > > provide at least some (upper) estimate.
> > >
> >
> > Thanks for your review. This forces me to think more on this because each
> > update does not necessarily be a single page sized update e.g. adding a hugepage
> > to an LRU.
> >
> > Though I think the error is time bounded by 2 seconds but in those 2 seconds
> > mathematically the error can be large.
>
> Sounds significant?
Yes it can be.
>
> > What do you think of the following
> > change? It will bound the error better within the 2 seconds window.
>
> This didn't seem to go anywhere. I'll send "memcg: flush stats only if
> updated" Linuswards, but please remember to resurrect this idea soonish
> (this month?) if you think such a change is desirable.
>
Yes, I will follow up on this soon.