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

[Yosry Ahmed <yosryahmed@google.com>]

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
>