All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] memcg: flush stats only if updated
@ 2021-09-30  4:47 ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-09-30  4:47 UTC (permalink / raw)
  To: Johannes Weiner, Michal Hocko
  Cc: Michal Koutný,
	Andrew Morton, cgroups, linux-mm, linux-kernel, Shakeel Butt

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
bserved 63% reduction in the read time of 8 GiB file.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
 mm/memcontrol.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6da5020a8656..933dde29c67b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -107,6 +107,8 @@ static bool do_memsw_account(void)
 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);
 
 #define THRESHOLDS_EVENTS_TARGET 128
 #define SOFTLIMIT_EVENTS_TARGET 1024
@@ -635,6 +637,13 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
 	return mz;
 }
 
+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);
+}
+
 /**
  * __mod_memcg_state - update cgroup memory statistics
  * @memcg: the memory cgroup
@@ -647,7 +656,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. */
@@ -675,10 +684,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);
 }
 
 /**
@@ -780,7 +791,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)
@@ -5341,15 +5352,22 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 	memcg_wb_domain_size_changed(memcg);
 }
 
-void mem_cgroup_flush_stats(void)
+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();
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 1/2] memcg: flush stats only if updated
@ 2021-09-30  4:47 ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-09-30  4:47 UTC (permalink / raw)
  To: Johannes Weiner, Michal Hocko
  Cc: Michal Koutný,
	Andrew Morton, cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shakeel Butt

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
bserved 63% reduction in the read time of 8 GiB file.

Signed-off-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 mm/memcontrol.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6da5020a8656..933dde29c67b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -107,6 +107,8 @@ static bool do_memsw_account(void)
 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);
 
 #define THRESHOLDS_EVENTS_TARGET 128
 #define SOFTLIMIT_EVENTS_TARGET 1024
@@ -635,6 +637,13 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
 	return mz;
 }
 
+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);
+}
+
 /**
  * __mod_memcg_state - update cgroup memory statistics
  * @memcg: the memory cgroup
@@ -647,7 +656,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. */
@@ -675,10 +684,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);
 }
 
 /**
@@ -780,7 +791,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)
@@ -5341,15 +5352,22 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 	memcg_wb_domain_size_changed(memcg);
 }
 
-void mem_cgroup_flush_stats(void)
+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();
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] memcg: unify memcg stat flushing
@ 2021-09-30  4:47   ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-09-30  4:47 UTC (permalink / raw)
  To: Johannes Weiner, Michal Hocko
  Cc: Michal Koutný,
	Andrew Morton, cgroups, linux-mm, linux-kernel, Shakeel Butt

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 <shakeelb@google.com>
---
 mm/memcontrol.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 933dde29c67b..688c891448dd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1425,7 +1425,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;
@@ -3529,8 +3529,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)
@@ -3911,7 +3910,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,
@@ -3983,7 +3982,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;
@@ -4486,7 +4485,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);
@@ -5354,12 +5353,14 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 
 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)
@@ -6391,7 +6392,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.685.g46640cef36-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] memcg: unify memcg stat flushing
@ 2021-09-30  4:47   ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-09-30  4:47 UTC (permalink / raw)
  To: Johannes Weiner, Michal Hocko
  Cc: Michal Koutný,
	Andrew Morton, cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shakeel Butt

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 <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 mm/memcontrol.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 933dde29c67b..688c891448dd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1425,7 +1425,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;
@@ -3529,8 +3529,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)
@@ -3911,7 +3910,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,
@@ -3983,7 +3982,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;
@@ -4486,7 +4485,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);
@@ -5354,12 +5353,14 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 
 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)
@@ -6391,7 +6392,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.685.g46640cef36-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] memcg: flush stats only if updated
@ 2021-10-01 14:28   ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2021-10-01 14:28 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Michal Hocko, Michal Koutný,
	Andrew Morton, cgroups, linux-mm, linux-kernel

On Wed, Sep 29, 2021 at 09:47:10PM -0700, Shakeel Butt wrote:
> 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
> bserved 63% reduction in the read time of 8 GiB file.
> 
> Signed-off-by: Shakeel Butt <shakeelb@google.com>

This is a great idea.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

One minor nit:

> @@ -107,6 +107,8 @@ static bool do_memsw_account(void)
>  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);
>  
>  #define THRESHOLDS_EVENTS_TARGET 128
>  #define SOFTLIMIT_EVENTS_TARGET 1024
> @@ -635,6 +637,13 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
>  	return mz;
>  }
>  
> +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);
> +}
> +
>  /**
>   * __mod_memcg_state - update cgroup memory statistics
>   * @memcg: the memory cgroup
> @@ -647,7 +656,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. */
> @@ -675,10 +684,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);
>  }
>  
>  /**
> @@ -780,7 +791,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)
> @@ -5341,15 +5352,22 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
>  	memcg_wb_domain_size_changed(memcg);
>  }
>  
> -void mem_cgroup_flush_stats(void)
> +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();
> +}

Because of the way the updates and the flush interact through these
variables now, it might be better to move these up and together.

It'd also be good to have a small explanation of the optimization in
the code as well - that we accept (limited) percpu fuzz in lieu of not
having to check all percpus for every flush.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] memcg: flush stats only if updated
@ 2021-10-01 14:28   ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2021-10-01 14:28 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Michal Hocko, Michal Koutný,
	Andrew Morton, cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Wed, Sep 29, 2021 at 09:47:10PM -0700, Shakeel Butt wrote:
> 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
> bserved 63% reduction in the read time of 8 GiB file.
> 
> Signed-off-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

This is a great idea.

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

One minor nit:

> @@ -107,6 +107,8 @@ static bool do_memsw_account(void)
>  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);
>  
>  #define THRESHOLDS_EVENTS_TARGET 128
>  #define SOFTLIMIT_EVENTS_TARGET 1024
> @@ -635,6 +637,13 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz)
>  	return mz;
>  }
>  
> +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);
> +}
> +
>  /**
>   * __mod_memcg_state - update cgroup memory statistics
>   * @memcg: the memory cgroup
> @@ -647,7 +656,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. */
> @@ -675,10 +684,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);
>  }
>  
>  /**
> @@ -780,7 +791,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)
> @@ -5341,15 +5352,22 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
>  	memcg_wb_domain_size_changed(memcg);
>  }
>  
> -void mem_cgroup_flush_stats(void)
> +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();
> +}

Because of the way the updates and the flush interact through these
variables now, it might be better to move these up and together.

It'd also be good to have a small explanation of the optimization in
the code as well - that we accept (limited) percpu fuzz in lieu of not
having to check all percpus for every flush.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] memcg: unify memcg stat flushing
@ 2021-10-01 14:29     ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2021-10-01 14:29 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Michal Hocko, Michal Koutný,
	Andrew Morton, cgroups, linux-mm, linux-kernel

On Wed, Sep 29, 2021 at 09:47:11PM -0700, Shakeel Butt wrote:
> 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 <shakeelb@google.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] memcg: unify memcg stat flushing
@ 2021-10-01 14:29     ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2021-10-01 14:29 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Michal Hocko, Michal Koutný,
	Andrew Morton, cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Wed, Sep 29, 2021 at 09:47:11PM -0700, Shakeel Butt wrote:
> 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 <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] memcg: flush stats only if updated
@ 2021-10-01 14:41     ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-10-01 14:41 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Michal Hocko, Michal Koutný, Andrew Morton, Cgroups, Linux MM, LKML

On Fri, Oct 1, 2021 at 7:26 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Sep 29, 2021 at 09:47:10PM -0700, Shakeel Butt wrote:
> > 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
> > bserved 63% reduction in the read time of 8 GiB file.
> >
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
>
> This is a great idea.
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks.

>
> One minor nit:
>
[...]
>
> Because of the way the updates and the flush interact through these
> variables now, it might be better to move these up and together.
>
> It'd also be good to have a small explanation of the optimization in
> the code as well - that we accept (limited) percpu fuzz in lieu of not
> having to check all percpus for every flush.

I will move the code and add the comment on the optimization in the
next version.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] memcg: flush stats only if updated
@ 2021-10-01 14:41     ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2021-10-01 14:41 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Michal Hocko, Michal Koutný, Andrew Morton, Cgroups, Linux MM, LKML

On Fri, Oct 1, 2021 at 7:26 AM Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> wrote:
>
> On Wed, Sep 29, 2021 at 09:47:10PM -0700, Shakeel Butt wrote:
> > 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
> > bserved 63% reduction in the read time of 8 GiB file.
> >
> > Signed-off-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> This is a great idea.
>
> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

Thanks.

>
> One minor nit:
>
[...]
>
> Because of the way the updates and the flush interact through these
> variables now, it might be better to move these up and together.
>
> It'd also be good to have a small explanation of the optimization in
> the code as well - that we accept (limited) percpu fuzz in lieu of not
> having to check all percpus for every flush.

I will move the code and add the comment on the optimization in the
next version.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-10-01 14:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30  4:47 [PATCH 1/2] memcg: flush stats only if updated Shakeel Butt
2021-09-30  4:47 ` Shakeel Butt
2021-09-30  4:47 ` [PATCH 2/2] memcg: unify memcg stat flushing Shakeel Butt
2021-09-30  4:47   ` Shakeel Butt
2021-10-01 14:29   ` Johannes Weiner
2021-10-01 14:29     ` Johannes Weiner
2021-10-01 14:28 ` [PATCH 1/2] memcg: flush stats only if updated Johannes Weiner
2021-10-01 14:28   ` Johannes Weiner
2021-10-01 14:41   ` Shakeel Butt
2021-10-01 14:41     ` Shakeel Butt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.