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

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

[JP Kobryn]

This series adds new capability for understanding frequency and circumstances
behind flushing memcg stats.

Changelog v2:
- remove tracepoint enum and call sites where enum was used
- rename do_flush_stats() and include forced flag
- add forced and needs_flush flags, stats_updated to tracepoint event

Changelog v3:
- split renaming and flag into separate patch
- re-order patches so that the tracepoint comes after the preliminary code
  changes

JP Kobryn (2):
  rename do_flush_stats and add force flag
  add 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 v3] memcg: rename do_flush_stats and add force flag

[JP Kobryn]

Change the name to something more consistent with others in the file and
use double unders to signify it is associated with the
mem_cgroup_flush_stats() API call. Additionally include a new flag that
call sites use to indicate a forced flush; skipping checks and flushing
unconditionally. There are no changes in functionality.

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

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 18c3f513d766..59f6f247fc13 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -588,8 +588,11 @@ 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)
 {
+	if (!force && !memcg_vmstats_needs_flush(memcg->vmstats))
+		return;
+
 	if (mem_cgroup_is_root(memcg))
 		WRITE_ONCE(flush_last_time, jiffies_64);
 
@@ -613,8 +616,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 +632,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 +5283,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 1/2 v3] memcg: rename do_flush_stats and add force flag

[Yosry Ahmed]

On Mon, Oct 28, 2024 at 7:11 PM JP Kobryn <inwardvessel@gmail.com> wrote:
>
> Change the name to something more consistent with others in the file and
> use double unders to signify it is associated with the
> mem_cgroup_flush_stats() API call. Additionally include a new flag that
> call sites use to indicate a forced flush; skipping checks and flushing
> unconditionally. There are no changes in functionality.
>
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> ---
>  mm/memcontrol.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 18c3f513d766..59f6f247fc13 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -588,8 +588,11 @@ 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)
>  {
> +       if (!force && !memcg_vmstats_needs_flush(memcg->vmstats))
> +               return;
> +
>         if (mem_cgroup_is_root(memcg))
>                 WRITE_ONCE(flush_last_time, jiffies_64);
>
> @@ -613,8 +616,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 +632,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 +5283,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 1/2 v3] memcg: rename do_flush_stats and add force flag

[Shakeel Butt]

On Mon, Oct 28, 2024 at 07:11:05PM GMT, JP Kobryn wrote:
> Change the name to something more consistent with others in the file and
> use double unders to signify it is associated with the
> mem_cgroup_flush_stats() API call. Additionally include a new flag that
> call sites use to indicate a forced flush; skipping checks and flushing
> unconditionally. There are no changes in functionality.
> 
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

[PATCH 2/2 v3] memcg: add flush tracepoint

[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.

Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
---
 include/trace/events/memcg.h | 25 +++++++++++++++++++++++++
 mm/memcontrol.c              |  7 ++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

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 */
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 59f6f247fc13..c3d6163aaa1c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -590,7 +590,12 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 
 static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
 {
-	if (!force && !memcg_vmstats_needs_flush(memcg->vmstats))
+	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))
-- 
2.47.0

Re: [PATCH 2/2 v3] memcg: add flush tracepoint

[Yosry Ahmed]

On Mon, Oct 28, 2024 at 7:11 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.
>
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> ---
>  include/trace/events/memcg.h | 25 +++++++++++++++++++++++++
>  mm/memcontrol.c              |  7 ++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> 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 */
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 59f6f247fc13..c3d6163aaa1c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -590,7 +590,12 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>
>  static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
>  {
> -       if (!force && !memcg_vmstats_needs_flush(memcg->vmstats))
> +       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))
> --
> 2.47.0
>

Re: [PATCH 2/2 v3] memcg: add flush tracepoint

[Shakeel Butt]

On Mon, Oct 28, 2024 at 07:11:06PM GMT, JP Kobryn 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.
> 
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>