Re: [PATCH v2 13/13] perf machine thread: Remove exited threads by default

[Ian Rogers]

On Wed, Oct 18, 2023 at 4:30 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Wed, Oct 11, 2023 at 11:24 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > struct thread values hold onto references to mmaps, dsos, etc. When a
> > thread exits it is necessary to clean all of this memory up by
> > removing the thread from the machine's threads. Some tools require
> > this doesn't happen, such as perf report if offcpu events exist or if
> > a task list is being generated, so add a symbol_conf value to make the
> > behavior optional. When an exited thread is left in the machine's
> > threads, mark it as exited.
> >
> > This change relates to commit 40826c45eb0b ("perf thread: Remove
> > notion of dead threads"). Dead threads were removed as they had a
> > reference count of 0 and were difficult to reason about with the
> > reference count checker. Here a thread is removed from threads when it
> > exits, unless via symbol_conf the exited thread isn't remove and is
> > marked as exited. Reference counting behaves as it normally does.
>
> Maybe we can do it the other way around.  IOW tools can access
> dead threads for whatever reason if they are dealing with a data
> file.  And I guess the main concern is perf top to reduce memory
> footprint, right?  Then we can declare to remove the dead threads
> for perf top case only IMHO.

Thanks I did consider this but I think this order makes most sense.
The only tool relying on access to dead threads is perf report, but
its uses are questionable:

 - task printing - the tools wants to show all threads from a perf run
and assumes they are in threads. By replacing tool.exit it would be
easy to record dead threads for this, as the maps aren't required the
memory overhead could be improved by just recording the necessary
task's data.

 - offcpu - it would be very useful to have offcpu samples be real
samples, rather than an aggregated sample at the end of the data file
with a timestamp greater-than when the thread exited. These samples
would happen before the exit event is processed and so the reason to
keep dead threads around would be removed. I think we could do some
kind of sample aggregation using BPF, but I think we need to think it
through with exit events. Perhaps we can fix things in the short-term
by generating BPF samples with aggregation when threads exit in the
offcpu BPF code, but then again, if you have this going it seems as
easy just to keep to record all offcpu events as distinct.

Other commands like perf top and perf script don't currently have
dependencies on dead threads and I'm kind of glad, for
understandability, memory footprint, etc. Having the default be that
dead threads linger on will just encourage the kind of issues we see
currently in perf report and having to have every tool, perf script
declare it doesn't want dead threads seems burdensome.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/builtin-report.c   |  7 +++++++
> >  tools/perf/util/machine.c     | 10 +++++++---
> >  tools/perf/util/symbol_conf.h |  3 ++-
> >  tools/perf/util/thread.h      | 14 ++++++++++++++
> >  4 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index dcedfe00f04d..749246817aed 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -1411,6 +1411,13 @@ int cmd_report(int argc, const char **argv)
> >         if (ret < 0)
> >                 goto exit;
> >
> > +       /*
> > +        * tasks_mode require access to exited threads to list those that are in
> > +        * the data file. Off-cpu events are synthesized after other events and
> > +        * reference exited threads.
> > +        */
> > +       symbol_conf.keep_exited_threads = true;
> > +
> >         annotation_options__init(&report.annotation_opts);
> >
> >         ret = perf_config(report__config, &report);
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 6ca7500e2cf4..5cda47eb337d 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -2157,9 +2157,13 @@ int machine__process_exit_event(struct machine *machine, union perf_event *event
> >         if (dump_trace)
> >                 perf_event__fprintf_task(event, stdout);
> >
> > -       if (thread != NULL)
> > -               thread__put(thread);
> > -
> > +       if (thread != NULL) {
> > +               if (symbol_conf.keep_exited_threads)
> > +                       thread__set_exited(thread, /*exited=*/true);
> > +               else
> > +                       machine__remove_thread(machine, thread);
> > +       }
> > +       thread__put(thread);
> >         return 0;
> >  }
> >
> > diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> > index 2b2fb9e224b0..6040286e07a6 100644
> > --- a/tools/perf/util/symbol_conf.h
> > +++ b/tools/perf/util/symbol_conf.h
> > @@ -43,7 +43,8 @@ struct symbol_conf {
> >                         disable_add2line_warn,
> >                         buildid_mmap2,
> >                         guest_code,
> > -                       lazy_load_kernel_maps;
> > +                       lazy_load_kernel_maps,
> > +                       keep_exited_threads;
> >         const char      *vmlinux_name,
> >                         *kallsyms_name,
> >                         *source_prefix,
> > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> > index e79225a0ea46..0df775b5c110 100644
> > --- a/tools/perf/util/thread.h
> > +++ b/tools/perf/util/thread.h
> > @@ -36,13 +36,22 @@ struct thread_rb_node {
> >  };
> >
> >  DECLARE_RC_STRUCT(thread) {
> > +       /** @maps: mmaps associated with this thread. */
> >         struct maps             *maps;
> >         pid_t                   pid_; /* Not all tools update this */
> > +       /** @tid: thread ID number unique to a machine. */
> >         pid_t                   tid;
> > +       /** @ppid: parent process of the process this thread belongs to. */
> >         pid_t                   ppid;
> >         int                     cpu;
> >         int                     guest_cpu; /* For QEMU thread */
> >         refcount_t              refcnt;
> > +       /**
> > +        * @exited: Has the thread had an exit event. Such threads are usually
> > +        * removed from the machine's threads but some events/tools require
> > +        * access to dead threads.
> > +        */
> > +       bool                    exited;
> >         bool                    comm_set;
> >         int                     comm_len;
> >         struct list_head        namespaces_list;
> > @@ -189,6 +198,11 @@ static inline refcount_t *thread__refcnt(struct thread *thread)
> >         return &RC_CHK_ACCESS(thread)->refcnt;
> >  }
> >
> > +static inline void thread__set_exited(struct thread *thread, bool exited)
> > +{
> > +       RC_CHK_ACCESS(thread)->exited = exited;
> > +}
> > +
> >  static inline bool thread__comm_set(const struct thread *thread)
> >  {
> >         return RC_CHK_ACCESS(thread)->comm_set;
> > --
> > 2.42.0.609.gbb76f46606-goog
> >