All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <olsajiri@gmail.com>
To: Chengdong Li <brytonlee01@gmail.com>
Cc: alexey.v.bayduraev@linux.intel.com, peterz@infradead.org,
	mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
	alexander.shishkin@linux.intel.com, jolsa@kernel.org,
	namhyung@kernel.org, rickyman7@gmail.com,
	adrian.hunter@intel.com, irogers@google.com,
	german.gomez@arm.com, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, ak@linux.intel.com,
	likexu@tencent.com, chengdongli@tencent.com
Subject: Re: [PATCH] perf tools: fix callstack entries and nr print message
Date: Mon, 9 May 2022 15:45:06 +0200	[thread overview]
Message-ID: <Ynka4u+jCvFefgwJ@krava> (raw)
In-Reply-To: <20220509114743.22668-1-chengdongli@tencent.com>

On Mon, May 09, 2022 at 07:47:43PM +0800, Chengdong Li 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 fix 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)
> 
> Signed-off-by: Chengdong Li <chengdongli@tencent.com>
> ---
>  tools/perf/util/session.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f9a320694b85..568a1db98686 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1151,9 +1151,19 @@ 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);

please use { }

>  
>  	for (i = 0; i < sample->branch_stack->nr; i++) {
>  		struct branch_entry *e = &entries[i];
> @@ -1169,8 +1179,12 @@ 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);

same here

thanks,
jirka

  reply	other threads:[~2022-05-09 13:45 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 [this message]
2022-05-10  0:40   ` [PATCH v2] " Chengdong Li
2022-05-11  1:35     ` Bryton Lee
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=Ynka4u+jCvFefgwJ@krava \
    --to=olsajiri@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=brytonlee01@gmail.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.