Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

From: Yang Jihong
Date: Fri Mar 12 2021 - 21:24:12 EST

Hello,

On 2021/3/13 10:00, Yang Jihong wrote:
Hello, Namhyung
On 2021/3/12 18:20, Yang Jihong wrote:
Hello,

On 2021/3/12 16:39, Namhyung Kim wrote:
On Fri, Mar 12, 2021 at 4:19 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:


Hello,
On 2021/3/12 13:49, Namhyung Kim wrote:
Hi,

On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:

Hello, Namhyung

On 2021/3/11 22:42, Namhyung Kim wrote:
Hi,

On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:

Hello,

On 2021/3/6 16:28, Yang Jihong wrote:
In hist__find_annotations function, since have a hist_entry per IP for the same
symbol, we free notes->src to signal already processed this symbol in stdio mode;
when annotate, entry will skipped if notes->src is NULL to avoid repeated output.

I'm not sure it's still true that we have a hist_entry per IP.
Afaik the default sort key is comm,dso,sym which means it should have a single
hist_entry for each symbol.  It seems like an old comment..

Emm, yes, we have a hist_entry for per IP.
a member named "sym" in struct "hist_entry" points to symbol,
different IP may point to the same symbol.

Are you sure about this?  It seems like a bug then.

Yes, now each IP corresponds to a hist_entry :)

Last week I found that some sample events were missing when perf
annotate in stdio mode, so I went through the annotate code carefully.

The event handling process is as follows:
process_sample_event
    evsel_add_sample
      hists__add_entry
        __hists__add_entry
          hists__findnew_entry
            hist_entry__new                  -> here allock new hist_entry

Yeah, so this is for a symbol.


      hist_entry__inc_addr_samples
        symbol__inc_addr_samples
          symbol__hists
            annotated_source__new            -> here alloc annotate soruce
            annotated_source__alloc_histograms -> here alloc histograms

This should be for each IP (ideally it should be per instruction).


By bugs, do you mean there's something wrong?

No. I think we were saying about different things.  :)

OK, :)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index a23ba6bb99b6..a91fe45bd69f 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -374,13 +374,6 @@ static void hists__find_annotations(struct hists *hists,
                   } else {
                           hist_entry__tty_annotate(he, evsel, ann);
                           nd = rb_next(nd);
-                       /*
-                        * Since we have a hist_entry per IP for the same
-                        * symbol, free he->ms.sym->src to signal we already
-                        * processed this symbol.
-                        */
-                       zfree(&notes->src->cycles_hist);
-                       zfree(&notes->src);
                   }
           }
    }

This solution may have the following problem:
For example, if two sample events are in two different processes but in
the same symbol, repeated output may occur.
Therefore, a flag is required to indicate whether the symbol has been
processed to avoid repeated output.

Hmm.. ok.  Yeah we don't care about the processes here.
Then we should remove it from the sort key like below:

@@ -624,6 +617,7 @@ int cmd_annotate(int argc, const char **argv)
                  if (setup_sorting(annotate.session->evlist) < 0)
                          usage_with_options(annotate_usage, options);
          } else {
+               sort_order = "dso,symbol";
                  if (setup_sorting(NULL) < 0)
                          usage_with_options(annotate_usage, options);
          }


Are you referring to this solution?
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -374,13 +374,6 @@ static void hists__find_annotations(struct hists
*hists,
                  } else {
                          hist_entry__tty_annotate(he, evsel, ann);
                          nd = rb_next(nd);
-                       /*
-                        * Since we have a hist_entry per IP for the same
-                        * symbol, free he->ms.sym->src to signal we already
-                        * processed this symbol.
-                        */
-                       zfree(&notes->src->cycles_hist);
-                       zfree(&notes->src);
                  }
          }
   }
@@ -624,6 +617,7 @@ int cmd_annotate(int argc, const char **argv)
                  if (setup_sorting(annotate.session->evlist) < 0)
                          usage_with_options(annotate_usage, options);
          } else {
+               sort_order = "dso,symbol";
                  if (setup_sorting(NULL) < 0)
                          usage_with_options(annotate_usage, options);
          }
It seems to be a better solution without adding new member.
I just tested it and it works.

If we decide to use this solution, I'll resubmit a v3 patch.

I prefer changing the sort order (and removing the zfree and comments).

OK, I'll submit a v3 patch based on the "changing the sort order" solution.

I have submitted the v3 patch, look forward to your review:
https://lore.kernel.org/patchwork/patch/1394619/

The first line of comments in the v3 patch is not empty
and has been modified. For details, see v4 patch:
https://lore.kernel.org/patchwork/patch/1394623/
Please review the new patch :)
Thanks,
Yang
Thanks,
Namhyung
Thanks,
Yang
.

Thanks,
Yang