| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z8o4m5s-O4NUhz79@google.com> Date: Thu, 6 Mar 2025 16:06:51 -0800 From: Namhyung Kim <namhyung@...nel.org> To: Ian Rogers <irogers@...gle.com> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org, Athira Rajeev <atrajeev@...ux.vnet.ibm.com> Subject: Re: [PATCHSET 0/7] perf annotate: Add --code-with-type option On Wed, Mar 05, 2025 at 03:31:45PM -0800, Ian Rogers wrote: > On Wed, Mar 5, 2025 at 3:27 PM Ian Rogers <irogers@...gle.com> wrote: > > > > On Mon, Mar 3, 2025 at 9:38 AM Namhyung Kim <namhyung@...nel.org> wrote: > > > > > > Hello, > > > > > > There are roughly two modes in the perf annotate. One is normal code > > > anootation and the other is data type annotation. I'm proposing a new > > > > nit: s/anootation/annotation/ Oops. > > > > > way to combine the code and data annotation together. > > > > > > With this option, "# data-type: <name> [offset (field)]" part will be > > > adeded when it found a data type for the given instruction. This is > > > > nit: s/adeded/added/ Thanks for pointing this out. > > > > > for every instruction in the function regardless whether it has a > > > sample or not. > > > > > > This will be useful to understand function level data access patterns. > > > Currently it only works with --stdio, but I can add it to TUI later. > > > > > > Here's an example output. > > > > At some point I think we might as well have our own disassembler. The > > annotations remind me of the relocation and other annotation output of > > objdump. > > > > > $ perf annotate --code-with-type --stdio > > > ... > > > Percent | Source code & Disassembly of elf for cpu/mem-loads,ldlat=30/P (4 samples, percent: local period) > > > ---------------------------------------------------------------------------------------------------------------- > > > : 0 0xffffffff81bb7060 <__schedule>: > > > 0.00 : ffffffff81bb7060: pushq %rbp # data-type: (stack operation) > > > 0.00 : ffffffff81bb7061: movq %rsp, %rbp > > > 0.00 : ffffffff81bb7064: pushq %r15 # data-type: (stack operation) > > > 0.00 : ffffffff81bb7066: pushq %r14 # data-type: (stack operation) > > > 0.00 : ffffffff81bb7068: pushq %r13 # data-type: (stack operation) > > > 0.00 : ffffffff81bb706a: pushq %r12 # data-type: (stack operation) > > > 0.00 : ffffffff81bb706c: pushq %rbx # data-type: (stack operation) > > > > data-type: looks a bit weird here. I wonder if the > > .cfi_offset/.cfi_restore could make this information richer, maybe: > > 0.00 : ffffffff81bb7060: pushq %rbp # *(frame-16) = caller rbp > > 0.00 : ffffffff81bb7061: movq %rsp, %rbp > > 0.00 : ffffffff81bb7064: pushq %r15 # *(frame-24) = caller r15 Probably possible, but it's also obvious from the instruction. I didn't try to be smart here as we're interested in types. > > > > > 0.00 : ffffffff81bb706d: movq $0x33180, %rbx > > > 0.00 : ffffffff81bb7074: movq %rbx, %r12 > > > 0.00 : ffffffff81bb7077: subq $0x38, %rsp > > > 0.00 : ffffffff81bb707b: movl %edi, -0x5c(%rbp) # data-type: unsigned int +0 > > > 0.00 : ffffffff81bb707e: movq %gs:0x28, %rax # data-type: (stack canary) > > > 0.00 : ffffffff81bb7087: movq %rax, -0x30(%rbp) # data-type: (stack canary) > > > 0.00 : ffffffff81bb708b: xorl %eax, %eax > > > 0.00 : ffffffff81bb708d: movq $0x0, -0x58(%rbp) # data-type: struct rq_flags +0 (flags) > > > 0.00 : ffffffff81bb7095: movq $0x0, -0x50(%rbp) # data-type: struct rq_flags +0x8 (clock_update_flags) > > > 0.00 : ffffffff81bb709d: callq 0xffffffff81baa100 <debug_smp_processor_id> # data-type: (stack operation) > > > 0.00 : ffffffff81bb70a2: cltq > > > 0.00 : ffffffff81bb70a4: addq -0x7dcf0500(,%rax,8), %r12 # data-type: long unsigned int[] +0 > > > 0.00 : ffffffff81bb70ac: movq 0x960(%r12), %r13 # data-type: struct rq +0x960 (curr) > > > 0.00 : ffffffff81bb70b4: movq 0x20(%r13), %rax # data-type: struct task_struct +0x20 (stack) > > > 0.00 : ffffffff81bb70b8: cmpq $0x57ac6e9d, (%rax) > > > 0.00 : ffffffff81bb70bf: jne 0xffffffff81bb7bca <__schedule+0xb6a> > > > 0.00 : ffffffff81bb70c5: movl %gs:0x7e47b87c(%rip), %eax # 0x32948 <pcpu_hot+0x8> # data-type: struct pcpu_hot +0x8 (preempt_count) > > > > The variable name in the struct is really nice, it is a shame the same > > information gets repeated a few times on the line. I guess you meant 'pcpu_hot'. But the assembly shows the name of the global variable and the data type shows the name of struct. It happened to be the same in this case. > > Outside of the typos in the cover letter, the rest of the patches lgtm. > > Reviewed-by: Ian Rogers <irogers@...gle.com> Thanks for your review! Namhyung > > > > 0.00 : ffffffff81bb70cc: andl $0x7fffffff, %eax > > > 0.00 : ffffffff81bb70d1: cmpl $0x1, %eax > > > 0.00 : ffffffff81bb70d4: jne 0xffffffff81bb79de <__schedule+0x97e> > > > 0.00 : ffffffff81bb70da: nopl (%rax,%rax) > > > 0.00 : ffffffff81bb70df: cmpl $0x2, 0xe86b3a(%rip) # 0xffffffff82a3dc20 <prof_on> # data-type: int +0 > > > 0.00 : ffffffff81bb70e6: movq 0x8(%rbp), %rsi > > > 0.00 : ffffffff81bb70ea: je 0xffffffff81bb7a0b <__schedule+0x9ab> > > > 0.00 : ffffffff81bb70f0: nopl (%rax,%rax) > > > 0.00 : ffffffff81bb70f5: nop > > > ... > > > > > > The code is also available at 'perf/annotate-code-data-v1' branch in > > > git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git > > > > > > Thanks, > > > Namhyung > > > > > > > > > Namhyung Kim (7): > > > perf annotate-data: Add annotated_data_type__get_member_name() > > > perf annotate: Remove unused len parameter from > > > annotation_line__print() > > > perf annotate: Pass annotation_options to annotation_line__print() > > > perf annotate: Pass hist_entry to annotate functions > > > perf annotate: Factor out __hist_entry__get_data_type() > > > perf annotate: Implement code + data type annotation > > > perf annotate: Add --code-with-type option. > > > > > > tools/perf/Documentation/perf-annotate.txt | 4 + > > > tools/perf/builtin-annotate.c | 19 +- > > > tools/perf/builtin-top.c | 2 +- > > > tools/perf/util/annotate-data.c | 34 +++ > > > tools/perf/util/annotate-data.h | 3 + > > > tools/perf/util/annotate.c | 267 +++++++++++++-------- > > > tools/perf/util/annotate.h | 8 +- > > > tools/perf/util/sort.c | 39 +-- > > > 8 files changed, 236 insertions(+), 140 deletions(-) > > > > > > -- > > > 2.48.1.711.g2feabab25a-goog > > >
Powered by blists - more mailing lists