Message-ID: <CAM9d7cg5MYvLeOoBuKqp1pw7uvRfqCw1fXpLtgct0npL96JaYg@mail.gmail.com>
Date: Wed, 17 Jul 2024 23:43:01 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, irogers@...gle.com, segher@...nel.crashing.org, 
	christophe.leroy@...roup.eu, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, 
	akanksha@...ux.ibm.com, maddy@...ux.ibm.com, kjain@...ux.ibm.com, 
	disgoel@...ux.vnet.ibm.com
Subject: Re: [PATCH V7 00/18] Add data type profiling support for powerpc

On Wed, Jul 17, 2024 at 11:12 PM Athira Rajeev
<atrajeev@...ux.vnet.ibm.com> wrote:
>
>
>
> > On 18 Jul 2024, at 11:04 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hello,
> >
> > On Sat, Jul 13, 2024 at 10:25:11PM +0530, Athira Rajeev wrote:
> >> The patchset from Namhyung added support for data type profiling
> >> in perf tool. This enabled support to associate PMU samples to data
> >> types they refer using DWARF debug information. With the upstream
> >> perf, currently it possible to run perf report or perf annotate to
> >> view the data type information on x86.
> >>
> >> Initial patchset posted here had changes need to enable data type
> >> profiling support for powerpc.
> >>
> >> https://lore.kernel.org/all/6e09dc28-4a2e-49d8-a2b5-ffb3396a9952@csgroup.eu/T/
> >>
> >> Main change were:
> >> 1. powerpc instruction nmemonic table to associate load/store
> >> instructions with move_ops which is use to identify if instruction
> >> is a memory access one.
> >> 2. To get register number and access offset from the given
> >> instruction, code uses fields from "struct arch" -> objump.
> >> Added entry for powerpc here.
> >> 3. A get_arch_regnum to return register number from the
> >> register name string.
> >>
> >> But the apporach used in the initial patchset used parsing of
> >> disassembled code which the current perf tool implementation does.
> >>
> >> Example: lwz     r10,0(r9)
> >>
> >> This line "lwz r10,0(r9)" is parsed to extract instruction name,
> >> registers names and offset. Also to find whether there is a memory
> >> reference in the operands, "memory_ref_char" field of objdump is used.
> >> For x86, "(" is used as memory_ref_char to tackle instructions of the
> >> form "mov  (%rax), %rcx".
> >>
> >> In case of powerpc, not all instructions using "(" are the only memory
> >> instructions. Example, above instruction can also be of extended form (X
> >> form) "lwzx r10,0,r19". Inorder to easy identify the instruction category
> >> and extract the source/target registers, second patchset added support to use
> >> raw instruction. With raw instruction, macros are added to extract opcode
> >> and register fields.
> >> Link to second patchset:
> >> https://lore.kernel.org/all/20240506121906.76639-1-atrajeev@linux.vnet.ibm.com/
> >>
> >> Example representation using --show-raw-insn in objdump gives result:
> >>
> >> 38 01 81 e8     ld      r4,312(r1)
> >>
> >> Here "38 01 81 e8" is the raw instruction representation. In powerpc,
> >> this translates to instruction form: "ld RT,DS(RA)" and binary code
> >> as:
> >>  _____________________________________
> >>  | 58 |  RT  |  RA |      DS       | |
> >>  -------------------------------------
> >> 0    6     11    16              30 31
> >>
> >> Second patchset used "objdump" again to read the raw instruction.
> >> But since there is no need to disassemble and binary code can be read
> >> directly from the DSO, third patchset (ie this patchset) uses below
> >> apporach. The apporach preferred in powerpc to parse sample for data
> >> type profiling in V3 patchset is:
> >> - Read directly from DSO using dso__data_read_offset
> >> - If that fails for any case, fallback to using libcapstone
> >> - If libcapstone is not supported, approach will use objdump
> >>
> >> Patchset adds support to pick the opcode and reg fields from this
> >> raw/binary instruction code. This approach came in from review comment
> >> by Segher Boessenkool and Christophe for the initial patchset.
> >>
> >> Apart from that, instruction tracking is enabled for powerpc and
> >> support function is added to find variables defined as registers
> >> Example, in powerpc, below two registers are
> >> defined to represent variable:
> >> 1. r13: represents local_paca
> >> register struct paca_struct *local_paca asm("r13");
> >>
> >> 2. r1: represents stack_pointer
> >> register void *__stack_pointer asm("r1");
> >>
> >> These are handled in this patchset.
> >>
> >> - Patch 1 is to rearrange register state type structures to header file
> >> so that it can referred from other arch specific files
> >> - Patch 2 is to make instruction tracking as a callback to"struct arch"
> >> so that it can be implemented by other archs easily and defined in arch
> >> specific files
> >> - Patch 3 is to handle state type regs array size for x86 and powerpc
> >> - Patch 4 adds support to capture and parse raw instruction in powerpc
> >> using dso__data_read_offset utility
> >> - Patch 4 also adds logic to support using objdump when doing default "perf
> >> report" or "perf annotate" since it that needs disassembled instruction.
> >> - Patch 5 adds disasm_line__parse to parse raw instruction for powerpc
> >> - Patch 6 update parameters for reg extract functions to use raw
> >> instruction on powerpc
> >> - Patch 7 updates ins__find to carry raw_insn and also adds parse
> >> callback for memory instructions for powerpc
> >> - Patch 8 add support to identify memory instructions of opcode 31 in
> >> powerpc
> >> - Patch 9 adds more instructions to support instruction tracking in powerpc
> >> - Patch 10 and 11 handles instruction tracking for powerpc.
> >> - Patch 12, 13 and 14 add support to use libcapstone in powerpc
> >> - Patch 15 and patch 16 handles support to find global register variables
> >> - PAtch 17 updates data type compare functions data_type_cmp and
> >>  sort__typeoff_sort to include var_name along with type_name in
> >>  comparison.
> >> - Patch 18 handles insn-stat option for perf annotate
> >>
> >> Note:
> >> - There are remaining unknowns (25%) as seen in annotate Instruction stats
> >> below.
> >> - This patchset is not tested on powerpc32. In next step of enhancements
> >> along with handling remaining unknowns, plan to cover powerpc32 changes
> >> based on how testing goes.
> >>
> >> With the current patchset:
> >>
> >> ./perf record -a -e mem-loads sleep 1
> >> ./perf report -s type,typeoff --hierarchy --group --stdio
> >> ./perf annotate --data-type --insn-stat
> >>
> >> perf annotate logs:
> >> ==================
> >>
> >>
> >> Annotate Instruction stats
> >> total 609, ok 446 (73.2%), bad 163 (26.8%)
> >>
> >>  Name/opcode         :  Good   Bad
> >>  -----------------------------------------------------------
> >>  58                  :   323    80
> >>  32                  :    49    43
> >>  34                  :    33    11
> >>  OP_31_XOP_LDX       :     8    20
> >>  40                  :    23     0
> >>  OP_31_XOP_LWARX     :     5     1
> >>  OP_31_XOP_LWZX      :     2     3
> >>  OP_31_XOP_LDARX     :     3     0
> >>  33                  :     0     2
> >>  OP_31_XOP_LBZX      :     0     1
> >>  OP_31_XOP_LWAX      :     0     1
> >>  OP_31_XOP_LHZX      :     0     1
> >>
> >> perf report logs:
> >> =================
> >>
> >>  Total Lost Samples: 0
> >>
> >>  Samples: 1K of event 'mem-loads'
> >>  Event count (approx.): 937238
> >>
> >>  Overhead  Data Type  Data Type Offset
> >> ........  .........  ................
> >>    48.60%  (unknown)  (unknown) +0 (no field)
> >>    11.42%  long unsigned int  long unsigned int +0 (current_stack_pointer)
> >>     4.68%  struct paca_struct  struct paca_struct +2312 (__current)
> >>     4.57%  struct paca_struct  struct paca_struct +2354 (irq_soft_mask)
> >>     2.69%  struct paca_struct  struct paca_struct +2808 (canary)
> >>     2.68%  struct paca_struct  struct paca_struct +8 (paca_index)
> >>     2.24%  struct paca_struct  struct paca_struct +48 (data_offset)
> >>     1.43%  long unsigned int  long unsigned int +0 (no field)
> >>     1.41%  struct vm_fault  struct vm_fault +0 (vma)
> >>     1.29%  struct task_struct  struct task_struct +276 (flags)
> >>     1.03%  struct pt_regs  struct pt_regs +264 (user_regs.msr)
> >>     0.90%  struct security_hook_list  struct security_hook_list +0 (list.next)
> >>     0.76%  struct irq_desc  struct irq_desc +304 (irq_data.chip)
> >>     0.76%  struct rq  struct rq +2856 (cpu)
> >>     0.72%  long long unsigned int  long long unsigned int +0 (no field)
> >
> > Thanks for your work!  But I think you need to split the basic part and
> > global register support part which needs more review.
> >
> > For the patch 1 to 14:
> > Reviewed-by: Namhyung Kim <namhyung@...nel.org>
>
> Hi Namhyung
>
> Thanks for all suggestions and reviews. I will check latest comments for patches 15 and 16 (also patch 17 is dependent the global register support part). But patch 18 is not dependent on global register support patches. Along with patches 1 to 14, can you please add patch 18 also ?

Sure, feel free to add it to the patch 18.

Reviewed-by: Namhyung Kim <namhyung@...nel.org>

Thanks,
Namhyung

Powered by blists - more mailing lists