All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bryton Lee <brytonlee01@gmail.com>
To: jolsa@kernel.org, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: ak@linux.intel.com, likexu@tencent.com, chengdongli@tencent.com,
	mark.rutland@arm.com, mingo@redhat.com,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	alexander.shishkin@linux.intel.com, irogers@google.com,
	german.gomez@arm.com, rickyman7@gmail.com,
	Namhyung Kim <namhyung@kernel.org>,
	peterz@infradead.org, alexey.v.bayduraev@linux.intel.com
Subject: Re: [PATCH v2] perf tools: fix callstack entries and nr print message
Date: Wed, 11 May 2022 09:35:26 +0800	[thread overview]
Message-ID: <CAC2pzGdkXz+wJsiSLOV5quMugXDvbMbF-WpiJshXfMM9Qt3FRQ@mail.gmail.com> (raw)
In-Reply-To: <20220510004057.68284-1-chengdongli@tencent.com>

Hi Jiri,

Could you kindly review my patch again? I sent out my updates
yesterday. I am happy to know your guidance.

Thanks,
Chengdong

On Tue, May 10, 2022 at 8:41 AM Chengdong Li <brytonlee01@gmail.com> wrote:
>
> From: Chengdong Li <chengdongli@tencent.com>
>
> when generating callstack information from branch_stack(Intel LBR),
> the actual number of callstack entry should be bigger than the number
> of branch_stack, for example:
>
>         branch_stack records:
>                 B() -> C()
>                 A() -> B()
>         converted callstack records should be:
>                 C()
>                 B()
>                 A()
> though, the number of callstack equals
> to the number of branch stack plus 1.
>
> This patch fixes above issue in branch_stack__printf(). For example,
>
>         # echo 'scale=2000; 4*a(1)' > cmd
>         # perf record --call-graph lbr bc -l < cmd
>
> Before applying this patch, `perf script -D` output:
>
>         1220022677386876 0x2a40 [0xd8]: PERF_RECORD_SAMPLE(IP, 0x4002): 17990/17990: 0x40a6d6 period: 894172 addr: 0
>         ... LBR call chain: nr:8
>         .....  0: fffffffffffffe00
>         .....  1: 000000000040a410
>         .....  2: 000000000040573c
>         .....  3: 0000000000408650
>         .....  4: 00000000004022f2
>         .....  5: 00000000004015f5
>         .....  6: 00007f5ed6dcb553
>         .....  7: 0000000000401698
>         ... FP chain: nr:2
>         .....  0: fffffffffffffe00
>         .....  1: 000000000040a6d8
>         ... branch callstack: nr:6    # which is not consistent with LBR records.
>         .....  0: 000000000040a410
>         .....  1: 0000000000408650    # ditto
>         .....  2: 00000000004022f2
>         .....  3: 00000000004015f5
>         .....  4: 00007f5ed6dcb553
>         .....  5: 0000000000401698
>          ... thread: bc:17990
>          ...... dso: /usr/bin/bc
>         bc 17990 1220022.677386:     894172 cycles:
>                           40a410 [unknown] (/usr/bin/bc)
>                           40573c [unknown] (/usr/bin/bc)
>                           408650 [unknown] (/usr/bin/bc)
>                           4022f2 [unknown] (/usr/bin/bc)
>                           4015f5 [unknown] (/usr/bin/bc)
>                     7f5ed6dcb553 __libc_start_main+0xf3 (/usr/lib64/libc-2.17.so)
>                           401698 [unknown] (/usr/bin/bc)
>
> After applied:
>
>         1220022677386876 0x2a40 [0xd8]: PERF_RECORD_SAMPLE(IP, 0x4002): 17990/17990: 0x40a6d6 period: 894172 addr: 0
>         ... LBR call chain: nr:8
>         .....  0: fffffffffffffe00
>         .....  1: 000000000040a410
>         .....  2: 000000000040573c
>         .....  3: 0000000000408650
>         .....  4: 00000000004022f2
>         .....  5: 00000000004015f5
>         .....  6: 00007f5ed6dcb553
>         .....  7: 0000000000401698
>         ... FP chain: nr:2
>         .....  0: fffffffffffffe00
>         .....  1: 000000000040a6d8
>         ... branch callstack: nr:7
>         .....  0: 000000000040a410
>         .....  1: 000000000040573c
>         .....  2: 0000000000408650
>         .....  3: 00000000004022f2
>         .....  4: 00000000004015f5
>         .....  5: 00007f5ed6dcb553
>         .....  6: 0000000000401698
>          ... thread: bc:17990
>          ...... dso: /usr/bin/bc
>         bc 17990 1220022.677386:     894172 cycles:
>                           40a410 [unknown] (/usr/bin/bc)
>                           40573c [unknown] (/usr/bin/bc)
>                           408650 [unknown] (/usr/bin/bc)
>                           4022f2 [unknown] (/usr/bin/bc)
>                           4015f5 [unknown] (/usr/bin/bc)
>                     7f5ed6dcb553 __libc_start_main+0xf3 (/usr/lib64/libc-2.17.so)
>                           401698 [unknown] (/usr/bin/bc)
>
> Change from v1:
>         - refined code style according to Jiri's review comments.
>
> Signed-off-by: Chengdong Li <chengdongli@tencent.com>
> ---
>  tools/perf/util/session.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f9a320694b85..a7f93f5a1ac8 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1151,9 +1151,20 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
>         struct branch_entry *entries = perf_sample__branch_entries(sample);
>         uint64_t i;
>
> -       printf("%s: nr:%" PRIu64 "\n",
> -               !callstack ? "... branch stack" : "... branch callstack",
> -               sample->branch_stack->nr);
> +       if (!callstack) {
> +               printf("%s: nr:%" PRIu64 "\n", "... branch stack", sample->branch_stack->nr);
> +       } else {
> +               /* the reason of adding 1 to nr is because after expanding
> +                * branch stack it generates nr + 1 callstack records. e.g.,
> +                *         B()->C()
> +                *         A()->B()
> +                * the final callstack should be:
> +                *         C()
> +                *         B()
> +                *         A()
> +                */
> +               printf("%s: nr:%" PRIu64 "\n", "... branch callstack", sample->branch_stack->nr+1);
> +       }
>
>         for (i = 0; i < sample->branch_stack->nr; i++) {
>                 struct branch_entry *e = &entries[i];
> @@ -1169,8 +1180,13 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
>                                 (unsigned)e->flags.reserved,
>                                 e->flags.type ? branch_type_name(e->flags.type) : "");
>                 } else {
> -                       printf("..... %2"PRIu64": %016" PRIx64 "\n",
> -                               i, i > 0 ? e->from : e->to);
> +                       if (i == 0) {
> +                               printf("..... %2"PRIu64": %016" PRIx64 "\n"
> +                                      "..... %2"PRIu64": %016" PRIx64 "\n",
> +                                               i, e->to, i+1, e->from);
> +                       } else {
> +                               printf("..... %2"PRIu64": %016" PRIx64 "\n", i+1, e->from);
> +                       }
>                 }
>         }
>  }
> --
> 2.27.0
>


-- 
Best Regards

Bryton.Lee

  reply	other threads:[~2022-05-11  1:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-09 11:47 [PATCH] perf tools: fix callstack entries and nr print message Chengdong Li
2022-05-09 13:45 ` Jiri Olsa
2022-05-10  0:40   ` [PATCH v2] " Chengdong Li
2022-05-11  1:35     ` Bryton Lee [this message]
2022-05-17  1:57   ` [RESEND PATCH " Chengdong Li
2022-05-17 23:14     ` Namhyung Kim
2022-05-21 17:57       ` Arnaldo Carvalho de Melo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAC2pzGdkXz+wJsiSLOV5quMugXDvbMbF-WpiJshXfMM9Qt3FRQ@mail.gmail.com \
    --to=brytonlee01@gmail.com \
    --cc=acme@kernel.org \
    --cc=adrian.hunter@intel.com \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=alexey.v.bayduraev@linux.intel.com \
    --cc=chengdongli@tencent.com \
    --cc=german.gomez@arm.com \
    --cc=irogers@google.com \
    --cc=jolsa@kernel.org \
    --cc=likexu@tencent.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rickyman7@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.