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

[Yosry Ahmed <yosryahmed@google.com>]

On Thu, Oct 24, 2024 at 6:16 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Thu, Oct 24, 2024 at 05:57:25PM GMT, Yosry Ahmed wrote:
> > On Thu, Oct 24, 2024 at 5:26 PM JP Kobryn <inwardvessel@gmail.com> wrote:
> > >
> > > Make use of the flush tracepoint within memcontrol.
> > >
> > > Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> >
> > Is the intention to use tools like bpftrace to analyze where we flush
> > the most? In this case, why can't we just attach to the fentry of
> > do_flush_stats() and use the stack trace to find the path?
> >
> > We can also attach to mem_cgroup_flush_stats(), and the difference in
> > counts between the two will be the number of skipped flushes.
> >
>
> All these functions can get inlined and then we can not really attach
> easily. We can somehow find the offset in the inlined places and try to
> use kprobe but it is prohibitive when have to do for multiple kernels
> built with fdo/bolt.
>
> Please note that tracepoints are not really API, so we can remove them
> in future if we see no usage for them.

That's fair, but can we just add two tracepoints? This seems enough to
collect necessary data, and prevent proliferation of tracepoints and
the addition of the enum.

I am thinking one in mem_cgroup_flush_stats() and one in
do_flush_stats(), e.g. trace_mem_cgroup_flush_stats() and
trace_do_flush_stats(). Although the name of the latter is too
generic, maybe we should rename the function first to add mem_cgroup_*
or memcg_*.

WDYT?