[PATCH 0/2 v2] memcg: tracepoint for flushing stats

[PATCH 0/2 v2] memcg: tracepoint for flushing stats

[JP Kobryn]

This tracepoint gives visibility on how often the flushing of memcg stats
occurs and contains info on whether it was forced, skipped, and the value
of stats updated. It can help with understanding how readers are affected
by having to perform the flush, and the effectiveness of the flush by
inspecting the number of stats updated. Paired with the recently added
tracepoints for tracing rstat updates, it can also help show correlation
where stats exceed thresholds frequently.

JP Kobryn (2):
  add memcg flush tracepoint event
  use memcg flush tracepoint

 include/trace/events/memcg.h | 25 +++++++++++++++++++++++++
 mm/memcontrol.c              | 22 +++++++++++++---------
 2 files changed, 38 insertions(+), 9 deletions(-)

-- 
2.47.0

[PATCH 1/2 v2] memcg: add memcg flush tracepoint event

[JP Kobryn]

This is a tracepoint event that contains the memcg pointer and information
on whether the flush was forced, skipped, and the number of stats updated.

Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
---
 include/trace/events/memcg.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/trace/events/memcg.h b/include/trace/events/memcg.h
index 8667e57816d2..dfe2f51019b4 100644
--- a/include/trace/events/memcg.h
+++ b/include/trace/events/memcg.h
@@ -74,6 +74,31 @@ DEFINE_EVENT(memcg_rstat_events, count_memcg_events,
 	TP_ARGS(memcg, item, val)
 );
 
+TRACE_EVENT(memcg_flush_stats,
+
+	TP_PROTO(struct mem_cgroup *memcg, s64 stats_updates,
+		bool force, bool needs_flush),
+
+	TP_ARGS(memcg, stats_updates, force, needs_flush),
+
+	TP_STRUCT__entry(
+		__field(u64, id)
+		__field(s64, stats_updates)
+		__field(bool, force)
+		__field(bool, needs_flush)
+	),
+
+	TP_fast_assign(
+		__entry->id = cgroup_id(memcg->css.cgroup);
+		__entry->stats_updates = stats_updates;
+		__entry->force = force;
+		__entry->needs_flush = needs_flush;
+	),
+
+	TP_printk("memcg_id=%llu stats_updates=%lld force=%d needs_flush=%d",
+		__entry->id, __entry->stats_updates,
+		__entry->force, __entry->needs_flush)
+);
 
 #endif /* _TRACE_MEMCG_H */
 
-- 
2.47.0

[PATCH 2/2 v2] memcg: use memcg flush tracepoint

[JP Kobryn]

Make use of the memcg flush tracepoint within memcontrol.

Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
---
 mm/memcontrol.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 18c3f513d766..c3d6163aaa1c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -588,8 +588,16 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 	}
 }
 
-static void do_flush_stats(struct mem_cgroup *memcg)
+static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
 {
+	bool needs_flush = memcg_vmstats_needs_flush(memcg->vmstats);
+
+	trace_memcg_flush_stats(memcg, atomic64_read(&memcg->vmstats->stats_updates),
+		force, needs_flush);
+
+	if (!force && !needs_flush)
+		return;
+
 	if (mem_cgroup_is_root(memcg))
 		WRITE_ONCE(flush_last_time, jiffies_64);
 
@@ -613,8 +621,7 @@ void mem_cgroup_flush_stats(struct mem_cgroup *memcg)
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
-	if (memcg_vmstats_needs_flush(memcg->vmstats))
-		do_flush_stats(memcg);
+	__mem_cgroup_flush_stats(memcg, false);
 }
 
 void mem_cgroup_flush_stats_ratelimited(struct mem_cgroup *memcg)
@@ -630,7 +637,7 @@ static void flush_memcg_stats_dwork(struct work_struct *w)
 	 * Deliberately ignore memcg_vmstats_needs_flush() here so that flushing
 	 * in latency-sensitive paths is as cheap as possible.
 	 */
-	do_flush_stats(root_mem_cgroup);
+	__mem_cgroup_flush_stats(root_mem_cgroup, true);
 	queue_delayed_work(system_unbound_wq, &stats_flush_dwork, FLUSH_TIME);
 }
 
@@ -5281,11 +5288,8 @@ bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
 			break;
 		}
 
-		/*
-		 * mem_cgroup_flush_stats() ignores small changes. Use
-		 * do_flush_stats() directly to get accurate stats for charging.
-		 */
-		do_flush_stats(memcg);
+		/* Force flush to get accurate stats for charging */
+		__mem_cgroup_flush_stats(memcg, true);
 		pages = memcg_page_state(memcg, MEMCG_ZSWAP_B) / PAGE_SIZE;
 		if (pages < max)
 			continue;
-- 
2.47.0

Re: [PATCH 0/2 v2] memcg: tracepoint for flushing stats

[Yosry Ahmed]

On Fri, Oct 25, 2024 at 5:48 PM JP Kobryn <inwardvessel@gmail.com> wrote:
>
> This tracepoint gives visibility on how often the flushing of memcg stats
> occurs and contains info on whether it was forced, skipped, and the value
> of stats updated. It can help with understanding how readers are affected
> by having to perform the flush, and the effectiveness of the flush by
> inspecting the number of stats updated. Paired with the recently added
> tracepoints for tracing rstat updates, it can also help show correlation
> where stats exceed thresholds frequently.
>
> JP Kobryn (2):
>   add memcg flush tracepoint event
>   use memcg flush tracepoint

I think this should be re-arranged. The first patch should have the
refactoring of the flushing code with no functional changes. The
second patch would introduce the tracepoint and use it.

Also, please use more descriptive commit logs. Most of the text in the
cover letter should be in the patch adding the tracepoint. For the
refactoring patch, please describe the refactoring and its purpose,
and mention that it is expected to be functionally a noop.

>
>  include/trace/events/memcg.h | 25 +++++++++++++++++++++++++
>  mm/memcontrol.c              | 22 +++++++++++++---------
>  2 files changed, 38 insertions(+), 9 deletions(-)
>
> --
> 2.47.0
>