mm, memcontrol: Make cgroup_rstat available to controllers

PATCH New Tejun Heo 4 participants 13 messages Mar 24, 2018 – Apr 10, 2018

Subsystems (6)

Stable branch Memory management cgroup MGLRU +2 more subsystems

lore

Share & tools

lore

Tejun Heo

author maintainer

2018-03-24 16:08 UTC #0 lore raw

PATCH 1/1 mm, memcontrol: Make cgroup_rstat available to controllers

Hello,

Since a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting"), memcg uses percpu batch-overflowing for all stat accounting. While the propagation delay is okay for statistics, it doesn't work for events. If a notification for an event is sent out, the relevant counter must reflect the event when read afterwards. With the percpu batching, it's easy to miss, for example, an oom or oom_kill event because it's still buffered in one of the percpu counters.

cgroup already has a mechanism to efficiently handle hierarchical statistics in a scalable manner, cgroup_rstat, and it now can be used by controllers.

This patchset addresses the forementioned problem by converting event accounting to cgroup_rstat. While the stat part isn't broken, it's also converted for consistency and a few other benefits. Also, while trying to convert lruvec_stat, I found out that it has no users. Remove it too (not sure whether it's needed for some non-obvious reasons tho).

0001-mm-memcontrol-Use-cgroup_rstat-for-event-accounting.patch 0002-mm-memcontrol-Use-cgroup_rstat-for-stat-accounting.patch 0003-mm-memcontrol-Remove-lruvec_stat.patch

This patchset is on top of the "cgroup/for-4.17: Make cgroup_rstat available to controllers" patchset[1] and also available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_rstat

diffstat follows.

 include/linux/memcontrol.h |  131 ++++++++++++------------
 mm/memcontrol.c            |  238 ++++++++++++++++++++-------------------------
 mm/vmscan.c                |    4 
 3 files changed, 180 insertions(+), 193 deletions(-)

Thanks.

--
tejun

[1] http://lkml.kernel.org/r/[email protected]

Tejun Heo

author maintainer

2018-03-24 16:08 UTC #1 lore raw

PATCH 1/3 mm: memcontrol: Use cgroup_rstat for event accounting

To fix scalability issue, a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting") made the per-cpu counters batch-overflow into the global one instead of summing them up on reads.

While the approach works for statistics which don't care about controlled errors, it doesn't work for events. For example, when a process in a cgroup is oom killed, a notification is generated on the memory.events file. A user reading the file after receiving the notification must be able to see the increased oom_kill event counter but often won't be able to due to the per-cpu batching.

The problem the original commit tried to fix was avoiding excessively high complexity on reads. This can be solved using cgroup_rstat instead which has the following properties.

* Per-cpu stat updates with a small bookkeeping overhead.

* Lazy, accurate and on-demand hierarchical stat propagation with the complexity of O(number of cgroups which have been active in the subtree since last read).

This patch converts event accounting to use cgroup_rstat.

* mem_cgroup_stat_cpu->last_events[] and mem_cgroup->pending_events[] are added to track propagation. As memcg makes use of both local and hierarchical stats, mem_cgroup->tree_events[] is added to track hierarchical numbers.

* The per-cpu counters are unsigned long and the collected counters are unsigned long long. This makes stat updates simple while avoiding overflows in the collected counters on 32bit machines. The existing code used unsigned long longs in some places but didn't cover enough to avoid overflows.

* memcg_sum_events() and tree_events() are replaced with direct accesses to mem_cgroup->events[] and ->tree_events[]. The accesses are wrapped between cgroup_rstat_flush_hold() and cgroup_rstat_flush_release().

Signed-off-by Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Roman Gushchin <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/memcontrol.h |  19 ++++----
 mm/memcontrol.c            | 114 ++++++++++++++++++++++++---------------------
 2 files changed, 70 insertions(+), 63 deletions(-)

include/linux/memcontrol.h +10 -9

index c46016b..f1afbf6 100644
@@ -91,6 +91,9 @@ struct mem_cgroup_stat_cpu {
 	unsigned long events[MEMCG_NR_EVENTS];
 	unsigned long nr_page_events;
 	unsigned long targets[MEM_CGROUP_NTARGETS];
+
+	/* for cgroup rstat delta calculation */
+	unsigned long last_events[MEMCG_NR_EVENTS];
 };
 
 struct mem_cgroup_reclaim_iter {

@@ -233,7 +236,11 @@ struct mem_cgroup {
 
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
 	atomic_long_t		stat[MEMCG_NR_STAT];
-	atomic_long_t		events[MEMCG_NR_EVENTS];
+
+	/* events is managed by cgroup rstat */
+	unsigned long long	events[MEMCG_NR_EVENTS];	/* local */
+	unsigned long long	tree_events[MEMCG_NR_EVENTS];	/* subtree */
+	unsigned long long	pending_events[MEMCG_NR_EVENTS];/* propagation */
 
 	unsigned long		socket_pressure;

@@ -649,17 +656,11 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 static inline void __count_memcg_events(struct mem_cgroup *memcg,
 					int idx, unsigned long count)
 {
-	unsigned long x;
-
 	if (mem_cgroup_disabled())
 		return;
 
-	x = count + __this_cpu_read(memcg->stat_cpu->events[idx]);
-	if (unlikely(x > MEMCG_CHARGE_BATCH)) {
-		atomic_long_add(x, &memcg->events[idx]);
-		x = 0;
-	}
-	__this_cpu_write(memcg->stat_cpu->events[idx], x);
+	__this_cpu_add(memcg->stat_cpu->events[idx], count);
+	cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
 }
 
 static inline void count_memcg_events(struct mem_cgroup *memcg,

mm/memcontrol.c +60 ~~-54~~

210 lines not on this page · raw message

Signature · 1 line

2.9.5

Johannes Weiner

maintainer

2018-04-04 14:07 UTC #2 lore raw

Re: [PATCH 1/3] mm: memcontrol: Use cgroup_rstat for event accounting

Quoted 11 lines · Tejun Heo

include/linux/memcontrol.h +3 -0

@@ -91,6 +91,9 @@ struct mem_cgroup_stat_cpu {
 	unsigned long events[MEMCG_NR_EVENTS];
 	unsigned long nr_page_events;
 	unsigned long targets[MEM_CGROUP_NTARGETS];
+
+	/* for cgroup rstat delta calculation */
+	unsigned long last_events[MEMCG_NR_EVENTS];
 };
 
 struct mem_cgroup_reclaim_iter {

include/linux/memcontrol.h struct mem_cgroup { · line 243

Quoted 9 lines

include/linux/memcontrol.h +5 -1

@@ -233,7 +236,11 @@ struct mem_cgroup {
 
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
 	atomic_long_t		stat[MEMCG_NR_STAT];
-	atomic_long_t		events[MEMCG_NR_EVENTS];
+
+	/* events is managed by cgroup rstat */
+	unsigned long long	events[MEMCG_NR_EVENTS];	/* local */
+	unsigned long long	tree_events[MEMCG_NR_EVENTS];	/* subtree */
+	unsigned long long	pending_events[MEMCG_NR_EVENTS];/* propagation */

The lazy updates are neat, but I'm a little concerned at the memory footprint. On a 64-cpu machine for example, this adds close to 9000 words to struct mem_cgroup. And we really only need the accuracy for the 4 cgroup items in memory.events, not all VM events and stats.

Why not restrict the patch to those? It would also get rid of the weird sharing between VM and cgroup enums.

Johannes Weiner

maintainer

2018-04-04 14:17 UTC #3 lore raw

Re: [PATCH 1/3] mm: memcontrol: Use cgroup_rstat for event accounting

Quoted 27 lines · Johannes Weiner

Quoted 11 lines · Tejun Heo
include/linux/memcontrol.h +3 -0
@@ -91,6 +91,9 @@ struct mem_cgroup_stat_cpu {
 	unsigned long events[MEMCG_NR_EVENTS];
 	unsigned long nr_page_events;
 	unsigned long targets[MEM_CGROUP_NTARGETS];
+
+	/* for cgroup rstat delta calculation */
+	unsigned long last_events[MEMCG_NR_EVENTS];
 };
 
 struct mem_cgroup_reclaim_iter {
include/linux/memcontrol.h struct mem_cgroup { · line 243
Quoted 9 lines
include/linux/memcontrol.h +5 -1
@@ -233,7 +236,11 @@ struct mem_cgroup {
 
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
 	atomic_long_t		stat[MEMCG_NR_STAT];
-	atomic_long_t		events[MEMCG_NR_EVENTS];
+
+	/* events is managed by cgroup rstat */
+	unsigned long long	events[MEMCG_NR_EVENTS];	/* local */
+	unsigned long long	tree_events[MEMCG_NR_EVENTS];	/* subtree */
+	unsigned long long	pending_events[MEMCG_NR_EVENTS];/* propagation */
The lazy updates are neat, but I'm a little concerned at the memory footprint. On a 64-cpu machine for example, this adds close to 9000 words to struct mem_cgroup. And we really only need the accuracy for the 4 cgroup items in memory.events, not all VM events and stats.

Quoted 2 lines

Why not restrict the patch to those? It would also get rid of the weird sharing between VM and cgroup enums.

In fact, I wonder if we need per-cpuness for MEMCG_LOW, MEMCG_HIGH etc. in the first place. They describe super high-level reclaim and OOM events, so they're not nearly as hot as other VM events and stats. We could probably just have a per-memcg array of atomics.

Michal Hocko

maintainer

2018-04-04 14:34 UTC #4 lore raw

Re: [PATCH 1/3] mm: memcontrol: Use cgroup_rstat for event accounting

Quoted 28 lines · Johannes Weiner

Quoted 27 lines · Johannes Weiner
Quoted 11 lines · Tejun Heo
include/linux/memcontrol.h +3 -0
@@ -91,6 +91,9 @@ struct mem_cgroup_stat_cpu {
 	unsigned long events[MEMCG_NR_EVENTS];
 	unsigned long nr_page_events;
 	unsigned long targets[MEM_CGROUP_NTARGETS];
+
+	/* for cgroup rstat delta calculation */
+	unsigned long last_events[MEMCG_NR_EVENTS];
 };
 
 struct mem_cgroup_reclaim_iter {
include/linux/memcontrol.h struct mem_cgroup { · line 243
Quoted 9 lines
include/linux/memcontrol.h +5 -1
@@ -233,7 +236,11 @@ struct mem_cgroup {
 
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
 	atomic_long_t		stat[MEMCG_NR_STAT];
-	atomic_long_t		events[MEMCG_NR_EVENTS];
+
+	/* events is managed by cgroup rstat */
+	unsigned long long	events[MEMCG_NR_EVENTS];	/* local */
+	unsigned long long	tree_events[MEMCG_NR_EVENTS];	/* subtree */
+	unsigned long long	pending_events[MEMCG_NR_EVENTS];/* propagation */
The lazy updates are neat, but I'm a little concerned at the memory footprint. On a 64-cpu machine for example, this adds close to 9000 words to struct mem_cgroup. And we really only need the accuracy for the 4 cgroup items in memory.events, not all VM events and stats.

Quoted 7 lines

Quoted 2 lines
Why not restrict the patch to those? It would also get rid of the weird sharing between VM and cgroup enums.
In fact, I wonder if we need per-cpuness for MEMCG_LOW, MEMCG_HIGH etc. in the first place. They describe super high-level reclaim and OOM events, so they're not nearly as hot as other VM events and stats. We could probably just have a per-memcg array of atomics.

Agreed!

Signature · 2 lines

Michal Hocko
SUSE Labs

Tejun Heo

author maintainer

2018-04-04 16:58 UTC #5 lore raw

Re: [PATCH 1/3] mm: memcontrol: Use cgroup_rstat for event accounting

Hello,

Quoted 8 lines · Michal Hocko

Quoted 8 lines
Quoted 7 lines
The lazy updates are neat, but I'm a little concerned at the memory footprint. On a 64-cpu machine for example, this adds close to 9000 words to struct mem_cgroup. And we really only need the accuracy for the 4 cgroup items in memory.events, not all VM events and stats.
Why not restrict the patch to those? It would also get rid of the weird sharing between VM and cgroup enums.

Quoted 6 lines

Quoted 4 lines
In fact, I wonder if we need per-cpuness for MEMCG_LOW, MEMCG_HIGH etc. in the first place. They describe super high-level reclaim and OOM events, so they're not nearly as hot as other VM events and stats. We could probably just have a per-memcg array of atomics.
Agreed!

Ah, yeah, if we aren't worried about the update frequency of MEMCG_HIGH, which likely is the highest freq, we can just switch to atomic_t. I'm gonna apply the cgroup stat refactoring patches to cgroup, so if we ever wanna switch the counter to rstat, we can easily do that later.

Thasnks.

Signature · 1 line

tejun

Johannes Weiner

maintainer

2018-04-05 17:55 UTC #6 lore raw

PATCH 1/1 mm: memcg: make sure memory.events is uptodate when waking pollers

Quoted 14 lines · Tejun Heo

Quoted 13 lines · Michal Hocko
Quoted 12 lines
Quoted 7 lines
The lazy updates are neat, but I'm a little concerned at the memory footprint. On a 64-cpu machine for example, this adds close to 9000 words to struct mem_cgroup. And we really only need the accuracy for the 4 cgroup items in memory.events, not all VM events and stats.
Why not restrict the patch to those? It would also get rid of the weird sharing between VM and cgroup enums.
In fact, I wonder if we need per-cpuness for MEMCG_LOW, MEMCG_HIGH etc. in the first place. They describe super high-level reclaim and OOM events, so they're not nearly as hot as other VM events and stats. We could probably just have a per-memcg array of atomics.

Quoted 7 lines

Quoted 1 line
Agreed!
Ah, yeah, if we aren't worried about the update frequency of MEMCG_HIGH, which likely is the highest freq, we can just switch to atomic_t. I'm gonna apply the cgroup stat refactoring patches to cgroup, so if we ever wanna switch the counter to rstat, we can easily do that later.

Yeah, that's still great to have as generalized infrastructure.

For memory.events, how about this instead?

---

From 4369ce161a9085aa408f2eca54f9de72909ee1b1 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Thu, 5 Apr 2018 11:53:55 -0400
Subject: [PATCH] mm: memcg: make sure memory.events is uptodate when waking
pollers

a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting") added per-cpu drift to all memory cgroup stats and events shown in memory.stat and memory.events.

For memory.stat this is acceptable. But memory.events issues file notifications, and somebody polling the file for changes will be confused when the counters in it are unchanged after a wakeup.

Luckily, the events in memory.events - MEMCG_LOW, MEMCG_HIGH, MEMCG_MAX, MEMCG_OOM - are sufficiently rare and high-level that we don't need per-cpu buffering for them: MEMCG_HIGH and MEMCG_MAX would be the most frequent, but they're counting invocations of reclaim, which is a complex operation that touches many shared cachelines.

This splits memory.events from the generic VM events and tracks them in their own, unbuffered atomic counters. That's also cleaner, as it eliminates the ugly enum nesting of VM and cgroup events.

Fixes a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting")

Reported-by Tejun Heo <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

---
 include/linux/memcontrol.h | 35 ++++++++++++++++++-----------------
 mm/memcontrol.c            | 31 ++++++++++++++++++-------------
 mm/vmscan.c                |  2 +-
 3 files changed, 37 insertions(+), 31 deletions(-)

include/linux/memcontrol.h +18 ~~-17~~

index c46016bb25eb..6503a9ca27c1 100644
@@ -48,13 +48,12 @@ enum memcg_stat_item {
 	MEMCG_NR_STAT,
 };
 
-/* Cgroup-specific events, on top of universal VM events */
-enum memcg_event_item {
-	MEMCG_LOW = NR_VM_EVENT_ITEMS,
+enum memcg_memory_event {
+	MEMCG_LOW,
 	MEMCG_HIGH,
 	MEMCG_MAX,
 	MEMCG_OOM,
-	MEMCG_NR_EVENTS,
+	MEMCG_NR_MEMORY_EVENTS,
 };
 
 struct mem_cgroup_reclaim_cookie {

@@ -88,7 +87,7 @@ enum mem_cgroup_events_target {
 
 struct mem_cgroup_stat_cpu {
 	long count[MEMCG_NR_STAT];
-	unsigned long events[MEMCG_NR_EVENTS];
+	unsigned long events[NR_VM_EVENT_ITEMS];
 	unsigned long nr_page_events;
 	unsigned long targets[MEM_CGROUP_NTARGETS];
 };

@@ -202,7 +201,8 @@ struct mem_cgroup {
 	/* OOM-Killer disable */
 	int		oom_kill_disable;
 
-	/* handle for "memory.events" */
+	/* memory.events */
+	atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS];
 	struct cgroup_file events_file;
 
 	/* protect arrays of thresholds */

@@ -231,9 +231,10 @@ struct mem_cgroup {
 	struct task_struct	*move_lock_task;
 	unsigned long		move_lock_flags;
 
+	/* memory.stat */
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
 	atomic_long_t		stat[MEMCG_NR_STAT];
-	atomic_long_t		events[MEMCG_NR_EVENTS];
+	atomic_long_t		events[NR_VM_EVENT_ITEMS];
 
 	unsigned long		socket_pressure;

@@ -645,9 +646,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 						gfp_t gfp_mask,
 						unsigned long *total_scanned);
 
-/* idx can be of type enum memcg_event_item or vm_event_item */
 static inline void __count_memcg_events(struct mem_cgroup *memcg,
-					int idx, unsigned long count)
+					enum vm_event_item idx,
+					unsigned long count)
 {
 	unsigned long x;

@@ -663,7 +664,8 @@ static inline void __count_memcg_events(struct mem_cgroup *memcg,
 }
 
 static inline void count_memcg_events(struct mem_cgroup *memcg,
-				      int idx, unsigned long count)
+				      enum vm_event_item idx,
+				      unsigned long count)
 {
 	unsigned long flags;

@@ -672,9 +674,8 @@ static inline void count_memcg_events(struct mem_cgroup *memcg,
 	local_irq_restore(flags);
 }
 
-/* idx can be of type enum memcg_event_item or vm_event_item */
 static inline void count_memcg_page_event(struct page *page,
-					  int idx)
+					  enum vm_event_item idx)
 {
 	if (page->mem_cgroup)
 		count_memcg_events(page->mem_cgroup, idx, 1);

@@ -698,10 +699,10 @@ static inline void count_memcg_event_mm(struct mm_struct *mm,
 	rcu_read_unlock();
 }
 
-static inline void mem_cgroup_event(struct mem_cgroup *memcg,
-				    enum memcg_event_item event)
+static inline void memcg_memory_event(struct mem_cgroup *memcg,
+				      enum memcg_memory_event event)
 {
-	count_memcg_events(memcg, event, 1);
+	atomic_long_inc(&memcg->memory_events[event]);
 	cgroup_file_notify(&memcg->events_file);
 }

@@ -721,8 +722,8 @@ static inline bool mem_cgroup_disabled(void)
 	return true;
 }
 
-static inline void mem_cgroup_event(struct mem_cgroup *memcg,
-				    enum memcg_event_item event)
+static inline void memcg_memory_event(struct mem_cgroup *memcg,
+				      enum memcg_memory_event event)
 {
 }

mm/memcontrol.c +18 ~~-13~~

index 9ec024b862ac..e29c80b4a9ee 100644
@@ -1839,7 +1839,7 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
 			}
 		}
 
-		for (i = 0; i < MEMCG_NR_EVENTS; i++) {
+		for (i = 0; i < NR_VM_EVENT_ITEMS; i++) {
 			long x;
 
 			x = this_cpu_xchg(memcg->stat_cpu->events[i], 0);

@@ -1858,7 +1858,7 @@ static void reclaim_high(struct mem_cgroup *memcg,
 	do {
 		if (page_counter_read(&memcg->memory) <= memcg->high)
 			continue;
-		mem_cgroup_event(memcg, MEMCG_HIGH);
+		memcg_memory_event(memcg, MEMCG_HIGH);
 		try_to_free_mem_cgroup_pages(memcg, nr_pages, gfp_mask, true);
 	} while ((memcg = parent_mem_cgroup(memcg)));
 }

@@ -1949,7 +1949,7 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (!gfpflags_allow_blocking(gfp_mask))
 		goto nomem;
 
-	mem_cgroup_event(mem_over_limit, MEMCG_MAX);
+	memcg_memory_event(mem_over_limit, MEMCG_MAX);
 
 	nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
 						    gfp_mask, may_swap);

@@ -1992,7 +1992,7 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (fatal_signal_pending(current))
 		goto force;
 
-	mem_cgroup_event(mem_over_limit, MEMCG_OOM);
+	memcg_memory_event(mem_over_limit, MEMCG_OOM);
 
 	mem_cgroup_oom(mem_over_limit, gfp_mask,
 		       get_order(nr_pages * PAGE_SIZE));

@@ -2688,10 +2688,10 @@ static void tree_events(struct mem_cgroup *memcg, unsigned long *events)
 	struct mem_cgroup *iter;
 	int i;
 
-	memset(events, 0, sizeof(*events) * MEMCG_NR_EVENTS);
+	memset(events, 0, sizeof(*events) * NR_VM_EVENT_ITEMS);
 
 	for_each_mem_cgroup_tree(iter, memcg) {
-		for (i = 0; i < MEMCG_NR_EVENTS; i++)
+		for (i = 0; i < NR_VM_EVENT_ITEMS; i++)
 			events[i] += memcg_sum_events(iter, i);
 	}
 }

@@ -5178,7 +5178,7 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
 			continue;
 		}
 
-		mem_cgroup_event(memcg, MEMCG_OOM);
+		memcg_memory_event(memcg, MEMCG_OOM);
 		if (!mem_cgroup_out_of_memory(memcg, GFP_KERNEL, 0))
 			break;
 	}

@@ -5191,11 +5191,16 @@ static int memory_events_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
 
-	seq_printf(m, "low %lu\n", memcg_sum_events(memcg, MEMCG_LOW));
-	seq_printf(m, "high %lu\n", memcg_sum_events(memcg, MEMCG_HIGH));
-	seq_printf(m, "max %lu\n", memcg_sum_events(memcg, MEMCG_MAX));
-	seq_printf(m, "oom %lu\n", memcg_sum_events(memcg, MEMCG_OOM));
-	seq_printf(m, "oom_kill %lu\n", memcg_sum_events(memcg, OOM_KILL));
+	seq_printf(m, "low %lu\n",
+		   atomic_long_read(&memcg->memory_events[MEMCG_LOW]));
+	seq_printf(m, "high %lu\n",
+		   atomic_long_read(&memcg->memory_events[MEMCG_HIGH]));
+	seq_printf(m, "max %lu\n",
+		   atomic_long_read(&memcg->memory_events[MEMCG_MAX]));
+	seq_printf(m, "oom %lu\n",
+		   atomic_long_read(&memcg->memory_events[MEMCG_OOM]));
+	seq_printf(m, "oom_kill %lu\n",
+		   atomic_long_read(&memcg->memory_events[OOM_KILL]));
 
 	return 0;
 }

@@ -5204,7 +5209,7 @@ static int memory_stat_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
 	unsigned long stat[MEMCG_NR_STAT];
-	unsigned long events[MEMCG_NR_EVENTS];
+	unsigned long events[NR_VM_EVENT_ITEMS];
 	int i;
 
 	/*

mm/vmscan.c +1 -1

index cd5dc3faaa57..7cdace56222f 100644
@@ -2544,7 +2544,7 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 					sc->memcg_low_skipped = 1;
 					continue;
 				}
-				mem_cgroup_event(memcg, MEMCG_LOW);
+				memcg_memory_event(memcg, MEMCG_LOW);
 			}
 
 			reclaimed = sc->nr_reclaimed;

Signature · 1 line

2.16.3

Tejun Heo

author maintainer Acked-by on v1 1/1

2018-04-05 19:45 UTC #7 lore raw

Re: [PATCH] mm: memcg: make sure memory.events is uptodate when waking pollers

Quoted 20 lines · Johannes Weiner

From 4369ce161a9085aa408f2eca54f9de72909ee1b1 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Thu, 5 Apr 2018 11:53:55 -0400
Subject: [PATCH] mm: memcg: make sure memory.events is uptodate when waking
pollers
a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting") added per-cpu drift to all memory cgroup stats and events shown in memory.stat and memory.events.
For memory.stat this is acceptable. But memory.events issues file notifications, and somebody polling the file for changes will be confused when the counters in it are unchanged after a wakeup.
Luckily, the events in memory.events - MEMCG_LOW, MEMCG_HIGH, MEMCG_MAX, MEMCG_OOM - are sufficiently rare and high-level that we don't need per-cpu buffering for them: MEMCG_HIGH and MEMCG_MAX would be the most frequent, but they're counting invocations of reclaim, which is a complex operation that touches many shared cachelines.

Quoted 7 lines

This splits memory.events from the generic VM events and tracks them in their own, unbuffered atomic counters. That's also cleaner, as it eliminates the ugly enum nesting of VM and cgroup events.
Fixes a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting")
Reported-by Tejun Heo <[email protected]>
Signed-off-by Johannes Weiner <[email protected]>

Yeah, that works. FWIW,

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Michal Hocko

maintainer Acked-by on v1 1/1

2018-04-06 12:03 UTC #8 lore raw

Re: [PATCH] mm: memcg: make sure memory.events is uptodate when waking pollers

In reply to Johannes Weiner

On Thu 05-04-18 13:55:16, Johannes Weiner wrote:
[...]

Quoted 20 lines

Quoted 1 line
From 4369ce161a9085aa408f2eca54f9de72909ee1b1 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Thu, 5 Apr 2018 11:53:55 -0400
Subject: [PATCH] mm: memcg: make sure memory.events is uptodate when waking
pollers
a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting") added per-cpu drift to all memory cgroup stats and events shown in memory.stat and memory.events.
For memory.stat this is acceptable. But memory.events issues file notifications, and somebody polling the file for changes will be confused when the counters in it are unchanged after a wakeup.
Luckily, the events in memory.events - MEMCG_LOW, MEMCG_HIGH, MEMCG_MAX, MEMCG_OOM - are sufficiently rare and high-level that we don't need per-cpu buffering for them: MEMCG_HIGH and MEMCG_MAX would be the most frequent, but they're counting invocations of reclaim, which is a complex operation that touches many shared cachelines.

Quoted 3 lines

This splits memory.events from the generic VM events and tracks them in their own, unbuffered atomic counters. That's also cleaner, as it eliminates the ugly enum nesting of VM and cgroup events.

I agree with the patch I am just worried about the naming a bit. It is quite confusing TBH. events should be vm_events and memory_events should be limit_events or something like that.

Quoted 3 lines

Fixes a983b5ebee57 ("mm: memcontrol: fix excessive complexity in memory.stat reporting")
Reported-by Tejun Heo <[email protected]>
Signed-off-by Johannes Weiner <[email protected]>

Anyway

Acked-by Michal Hocko <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs

Sasha Levin

2018-04-10 13:49 UTC #9 lore raw

Re: [PATCH] mm: memcg: make sure memory.events is uptodate when waking pollers

In reply to Johannes Weiner

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: a983b5ebee57 mm: memcontrol: fix excessive complexity in memory.stat reporting.

The bot has also determined it's probably a bug fixing patch. (score: 35.1321)

The bot has tested the following trees: v4.16.1.

v4.16.1: Build OK!

--
Thanks,
Sasha

Tejun Heo

author maintainer

2018-03-24 16:09 UTC #10 lore raw

PATCH 2/3 mm: memcontrol: Use cgroup_rstat for stat accounting

In reply to Tejun Heo

The approach didn't for events and the previous patch switched event accounting to cgroup_rstat. Unlike events, it works for stat accounting but switching to cgroup_rstat has the following benefits while keeping computational complexity low.

* More accurate accounting. The accumulated per-cpu errors with the batch approach could add up and cause unintended results with extreme configurations (e.g. balance_dirty_pages misbehavior with very low dirty ratio in a cgroup with a low memory limit).

* Consistency with event accounting.

* Cheaper and simpler access to hierarchical stats.

This patch converts stat accounting to use cgroup_rstat.

* mem_cgroup_stat_cpu->last_count[] and mem_cgroup->pending_stat[] are added to track propagation. As memcg makes use of both local and hierarchical stats, mem_cgroup->tree_stat[] is added to track hierarchical numbers.

* An rstat flush wrapper, memcg_stat_flush(), is added for memcg stat consumers outside memcg proper.

* Accessors are updated / added.

Signed-off-by Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Roman Gushchin <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/memcontrol.h | 74 +++++++++++++++++++++++++++++---------
 mm/memcontrol.c            | 90 +++++++++++++++++++++++++---------------------
 mm/vmscan.c                |  4 ++-
 3 files changed, 110 insertions(+), 58 deletions(-)

include/linux/memcontrol.h +58 ~~-16~~

index f1afbf6..0cf6d5a 100644
@@ -93,6 +93,7 @@ struct mem_cgroup_stat_cpu {
 	unsigned long targets[MEM_CGROUP_NTARGETS];
 
 	/* for cgroup rstat delta calculation */
+	unsigned long last_count[MEMCG_NR_STAT];
 	unsigned long last_events[MEMCG_NR_EVENTS];
 };

@@ -235,9 +236,12 @@ struct mem_cgroup {
 	unsigned long		move_lock_flags;
 
 	struct mem_cgroup_stat_cpu __percpu *stat_cpu;
-	atomic_long_t		stat[MEMCG_NR_STAT];
 
-	/* events is managed by cgroup rstat */
+	/* stat and events are managed by cgroup rstat */
+	long			stat[MEMCG_NR_STAT];		/* local */
+	long			tree_stat[MEMCG_NR_STAT];	/* subtree */
+	long			pending_stat[MEMCG_NR_STAT];	/* propagation */
+
 	unsigned long long	events[MEMCG_NR_EVENTS];	/* local */
 	unsigned long long	tree_events[MEMCG_NR_EVENTS];	/* subtree */
 	unsigned long long	pending_events[MEMCG_NR_EVENTS];/* propagation */

@@ -497,11 +501,32 @@ struct mem_cgroup *lock_page_memcg(struct page *page);
 void __unlock_page_memcg(struct mem_cgroup *memcg);
 void unlock_page_memcg(struct page *page);
 
-/* idx can be of type enum memcg_stat_item or node_stat_item */
-static inline unsigned long memcg_page_state(struct mem_cgroup *memcg,
-					     int idx)
+/**
+ * memcg_stat_flush - flush stat in a memcg's subtree
+ * @memcg: target memcg
+ *
+ * Flush cgroup_rstat statistics in @memcg's subtree.  This brings @memcg's
+ * statistics up-to-date.
+ */
+static inline void memcg_stat_flush(struct mem_cgroup *memcg)
 {
-	long x = atomic_long_read(&memcg->stat[idx]);
+	if (!memcg)
+		memcg = root_mem_cgroup;
+	cgroup_rstat_flush(memcg->css.cgroup);
+}
+
+/**
+ * __memcg_page_state - read page state counter without brininging it up-to-date
+ * @memcg: target memcg
+ * @idx: page state item to read
+ *
+ * Read a memcg page state counter.  @idx can be of type enum
+ * memcg_stat_item or node_stat_item.  The caller must haved flushed by
+ * calling memcg_stat_flush() to bring the counter up-to-date.
+ */
+static inline unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+	long x = READ_ONCE(memcg->stat[idx]);
 #ifdef CONFIG_SMP
 	if (x < 0)
 		x = 0;

@@ -509,21 +534,30 @@ static inline unsigned long memcg_page_state(struct mem_cgroup *memcg,
 	return x;
 }
 
+/**
+ * memcg_page_state - read page state counter after bringing it up-to-date
+ * @memcg: target memcg
+ * @idx: page state item to read
+ *
+ * __memcg_page_state() with implied flushing.  When reading multiple
+ * counters in sequence, flushing explicitly and using __memcg_page_state()
+ * is cheaper.
+ */
+static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+	memcg_stat_flush(memcg);
+	return __memcg_page_state(memcg, idx);
+}
+
 /* idx can be of type enum memcg_stat_item or node_stat_item */
 static inline void __mod_memcg_state(struct mem_cgroup *memcg,
 				     int idx, int val)
 {
-	long x;
-
 	if (mem_cgroup_disabled())
 		return;
 
-	x = val + __this_cpu_read(memcg->stat_cpu->count[idx]);
-	if (unlikely(abs(x) > MEMCG_CHARGE_BATCH)) {
-		atomic_long_add(x, &memcg->stat[idx]);
-		x = 0;
-	}
-	__this_cpu_write(memcg->stat_cpu->count[idx], x);
+	__this_cpu_add(memcg->stat_cpu->count[idx], val);
+	cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id());
 }
 
 /* idx can be of type enum memcg_stat_item or node_stat_item */

@@ -895,8 +929,16 @@ static inline bool mem_cgroup_oom_synchronize(bool wait)
 	return false;
 }
 
-static inline unsigned long memcg_page_state(struct mem_cgroup *memcg,
-					     int idx)
+static inline void memcg_stat_flush(struct mem_cgroup *memcg)
+{
+}
+
+static inline unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+	return 0;
+}
+
+static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
 {
 	return 0;
 }

mm/memcontrol.c +49 ~~-41~~

168 lines not on this page · raw message

mm/vmscan.c +3 -1

index bee5349..29bf99f 100644
@@ -2738,13 +2738,15 @@ static void snapshot_refaults(struct mem_cgroup *root_memcg, pg_data_t *pgdat)
 {
 	struct mem_cgroup *memcg;
 
+	memcg_stat_flush(root_memcg);
+
 	memcg = mem_cgroup_iter(root_memcg, NULL, NULL);
 	do {
 		unsigned long refaults;
 		struct lruvec *lruvec;
 
 		if (memcg)
-			refaults = memcg_page_state(memcg, WORKINGSET_ACTIVATE);
+			refaults = __memcg_page_state(memcg, WORKINGSET_ACTIVATE);
 		else
 			refaults = node_page_state(pgdat, WORKINGSET_ACTIVATE);

Signature · 1 line

2.9.5

Tejun Heo

author maintainer

2018-03-24 16:09 UTC #11 lore raw

PATCH 3/3 mm: memcontrol: Remove lruvec_stat

In reply to Tejun Heo

lruvec_stat doesn't have any consumer. Remove it.

Signed-off-by Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Roman Gushchin <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/memcontrol.h | 40 ----------------------------------------
 mm/memcontrol.c            | 36 ++----------------------------------
 2 files changed, 2 insertions(+), 74 deletions(-)

include/linux/memcontrol.h +0 ~~-40~~

index 0cf6d5a..85a8f00 100644
@@ -103,19 +103,12 @@ struct mem_cgroup_reclaim_iter {
 	unsigned int generation;
 };
 
-struct lruvec_stat {
-	long count[NR_VM_NODE_STAT_ITEMS];
-};
-
 /*
  * per-zone information in memory controller.
  */
 struct mem_cgroup_per_node {
 	struct lruvec		lruvec;
 
-	struct lruvec_stat __percpu *lruvec_stat_cpu;
-	atomic_long_t		lruvec_stat[NR_VM_NODE_STAT_ITEMS];
-
 	unsigned long		lru_zone_size[MAX_NR_ZONES][NR_LRU_LISTS];
 
 	struct mem_cgroup_reclaim_iter	iter[DEF_PRIORITY + 1];

@@ -602,29 +595,10 @@ static inline void mod_memcg_page_state(struct page *page,
 		mod_memcg_state(page->mem_cgroup, idx, val);
 }
 
-static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
-					      enum node_stat_item idx)
-{
-	struct mem_cgroup_per_node *pn;
-	long x;
-
-	if (mem_cgroup_disabled())
-		return node_page_state(lruvec_pgdat(lruvec), idx);
-
-	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	x = atomic_long_read(&pn->lruvec_stat[idx]);
-#ifdef CONFIG_SMP
-	if (x < 0)
-		x = 0;
-#endif
-	return x;
-}
-
 static inline void __mod_lruvec_state(struct lruvec *lruvec,
 				      enum node_stat_item idx, int val)
 {
 	struct mem_cgroup_per_node *pn;
-	long x;
 
 	/* Update node */
 	__mod_node_page_state(lruvec_pgdat(lruvec), idx, val);

@@ -636,14 +610,6 @@ static inline void __mod_lruvec_state(struct lruvec *lruvec,
 
 	/* Update memcg */
 	__mod_memcg_state(pn->memcg, idx, val);
-
-	/* Update lruvec */
-	x = val + __this_cpu_read(pn->lruvec_stat_cpu->count[idx]);
-	if (unlikely(abs(x) > MEMCG_CHARGE_BATCH)) {
-		atomic_long_add(x, &pn->lruvec_stat[idx]);
-		x = 0;
-	}
-	__this_cpu_write(pn->lruvec_stat_cpu->count[idx], x);
 }
 
 static inline void mod_lruvec_state(struct lruvec *lruvec,

@@ -967,12 +933,6 @@ static inline void mod_memcg_page_state(struct page *page,
 {
 }
 
-static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
-					      enum node_stat_item idx)
-{
-	return node_page_state(lruvec_pgdat(lruvec), idx);
-}
-
 static inline void __mod_lruvec_state(struct lruvec *lruvec,
 				      enum node_stat_item idx, int val)
 {

mm/memcontrol.c +2 ~~-34~~

index 03d1b30..d5bf01d 100644
@@ -1815,30 +1815,7 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)
 
 static int memcg_hotplug_cpu_dead(unsigned int cpu)
 {
-	struct memcg_stock_pcp *stock;
-	struct mem_cgroup *memcg;
-
-	stock = &per_cpu(memcg_stock, cpu);
-	drain_stock(stock);
-
-	for_each_mem_cgroup(memcg) {
-		int i;
-
-		for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++) {
-			int nid;
-			long x;
-
-			for_each_node(nid) {
-				struct mem_cgroup_per_node *pn;
-
-				pn = mem_cgroup_nodeinfo(memcg, nid);
-				x = this_cpu_xchg(pn->lruvec_stat_cpu->count[i], 0);
-				if (x)
-					atomic_long_add(x, &pn->lruvec_stat[i]);
-			}
-		}
-	}
-
+	drain_stock(&per_cpu(memcg_stock, cpu));
 	return 0;
 }

@@ -4056,12 +4033,6 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
 	if (!pn)
 		return 1;
 
-	pn->lruvec_stat_cpu = alloc_percpu(struct lruvec_stat);
-	if (!pn->lruvec_stat_cpu) {
-		kfree(pn);
-		return 1;
-	}
-
 	lruvec_init(&pn->lruvec);
 	pn->usage_in_excess = 0;
 	pn->on_tree = false;

@@ -4073,10 +4044,7 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
 
 static void free_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
 {
-	struct mem_cgroup_per_node *pn = memcg->nodeinfo[node];
-
-	free_percpu(pn->lruvec_stat_cpu);
-	kfree(pn);
+	kfree(memcg->nodeinfo[node]);
 }
 
 static void __mem_cgroup_free(struct mem_cgroup *memcg)

Signature · 1 line

2.9.5

Johannes Weiner

maintainer

2018-04-04 14:12 UTC #12 lore raw

Re: [PATCH 3/3] mm: memcontrol: Remove lruvec_stat

Quoted 1 line · Tejun Heo

lruvec_stat doesn't have any consumer. Remove it.

This tracks counters at the node-memcg intersection, which is a requirement for Josef's shrinker rework.

I don't know what happened to that patch series. Josef, Andrew?