Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"

[Jiri Olsa]

On Mon, Sep 05, 2022 at 10:24:40AM +0530, Athira Rajeev wrote:
> The affinity code in "affinity_set" function access array
> named "sched_cpus". The size for this array is allocated in
> affinity_setup function which is nothing but value from
> get_cpu_set_size. This is used to contain the cpumask value
> for each cpu. While setting bit for each cpu, it calls
> "set_bit" function which access index in sched_cpus array.
> If we provide a command-line option to -C which is more than
> the number of CPU's present in the system, the set_bit could
> access an array member which is out-of the array size. This
> is because currently, there is no boundary check for the CPU.
> This will result in seg fault:
> 
> <<>>
>  ./perf stat -C 12323431 ls
> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
> Segmentation fault (core dumped)
> <<>>
> 
> Fix this by adding boundary check for the array.
> 
> After the fix from powerpc system:
> 
> <<>>
> ./perf stat -C 12323431 ls 1>out
> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
> 
>  Performance counter stats for 'CPU(s) 12323431':
> 
> msec cpu-clock
>  context-switches
>  cpu-migrations
>  page-faults
>  cycles
>  instructions
>  branches
>  branch-misses
> 
>0.001192373 seconds time elapsed
> <<>>
> 
> Reported-by: Nageswara Sastry 
> Signed-off-by: Athira Rajeev 
> ---
>  tools/perf/util/affinity.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
> index 4d216c0dc425..a1dd37347abc 100644
> --- a/tools/perf/util/affinity.c
> +++ b/tools/perf/util/affinity.c
> @@ -49,8 +49,14 @@ void affinity__set(struct affinity *a, int cpu)
>  {
>   int cpu_set_size = get_cpu_set_size();
>  
> - if (cpu == -1)
> + /*
> +  * Return:
> +  * - if cpu is -1
> +  * - restrict out of bound access to sched_cpus
> +  */
> + if (cpu == -1 || ((cpu / __BITS_PER_LONG) >= (cpu_set_size / 8)))

hm, there's __BITS_PER_LONG in one case, but then there's hardcoded 8

would this be simpler:

if (cpu == -1 || ((cpu >= (cpu_set_size * 8
return;

jirka

>   return;
> +
>   a->changed = true;
>   set_bit(cpu, a->sched_cpus);
>   /*
> -- 
> 2.35.1
> 

Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass

[Jiri Olsa]

On Thu, Jan 06, 2022 at 05:15:07PM +0530, Naveen N. Rao wrote:
> These instructions are updated after the initial JIT, so redo codegen
> during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
> that this is more than just subprog calls.
> 
> Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
> Cc: sta...@vger.kernel.org # v5.15
> Signed-off-by: Naveen N. Rao 

Tested-by: Jiri Olsa 

thanks,
jirka

> ---
>  arch/powerpc/net/bpf_jit_comp.c   | 29 +++--
>  arch/powerpc/net/bpf_jit_comp32.c |  6 ++
>  arch/powerpc/net/bpf_jit_comp64.c |  7 ++-
>  3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index d6ffdd0f2309d0..56dd1f4e3e4447 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned 
> int size)
>   memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>  }
>  
> -/* Fix the branch target addresses for subprog calls */
> -static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
> -struct codegen_context *ctx, u32 *addrs)
> +/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra 
> pass */
> +static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
> +struct codegen_context *ctx, u32 *addrs)
>  {
>   const struct bpf_insn *insn = fp->insnsi;
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 tmp_idx;
> - int i, ret;
> + int i, j, ret;
>  
>   for (i = 0; i < fp->len; i++) {
>   /*
> @@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog 
> *fp, u32 *image,
>* of the JITed sequence remains unchanged.
>*/
>   ctx->idx = tmp_idx;
> + } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> + tmp_idx = ctx->idx;
> + ctx->idx = addrs[i] / 4;
> +#ifdef CONFIG_PPC32
> + PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 
> 1].imm);
> + PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
> + for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
> + EMIT(PPC_RAW_NOP());
> +#else
> + func_addr = ((u64)(u32)insn[i].imm) | 
> (((u64)(u32)insn[i + 1].imm) << 32);
> + PPC_LI64(b2p[insn[i].dst_reg], func_addr);
> + /* overwrite rest with nops */
> + for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
> + EMIT(PPC_RAW_NOP());
> +#endif
> + ctx->idx = tmp_idx;
> + i++;
>   }
>   }
>  
> @@ -200,13 +217,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog 
> *fp)
>   /*
>* Do not touch the prologue and epilogue as they will remain
>* unchanged. Only fix the branch target address for subprog
> -  * calls in the body.
> +  * calls in the body, and ldimm64 instructions.
>*
>* This does not change the offsets and lengths of the subprog
>* call instruction sequences and hence, the size of the JITed
>* image as well.
>*/
> - bpf_jit_fixup_subprog_calls(fp, code_base, , addrs);
> + bpf_jit_fixup_addresses(fp, code_base, , addrs);
>  
>   /* There is no need to perform the usual passes. */
>   goto skip_codegen_passes;
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
> b/arch/powerpc/net/bpf_jit_comp32.c
> index 997a47fa615b30..2258d3886d02ec 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -293,6 +293,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 true_cond;
> + u32 tmp_idx;
> + int j;
>  
>   /*
>* addrs[] maps a BPF bytecode address into a real offset from
> @@ -908,8 +910,12 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>* 16 byte instruction that uses two 'struct bpf_insn'
>*/
>   case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
> +

Re: [BUG] powerpc: test_progs -t for_each faults kernel

[Jiri Olsa]

On Mon, Jan 03, 2022 at 11:07:02AM +0530, Naveen N. Rao wrote:
> Hi Jiri,
> 
> Jiri Olsa wrote:
> > hi,
> > when running 'test_progs -t for_each' on powerpc we are getting
> > the fault below
> 
> This looks to be the same issue reported by Yauheni:
> http://lkml.kernel.org/r/xunylf0o872l@redhat.com
> 
> Can you please check if the patch I posted there fixes it for you?

great, yes, that fixes it for me

thanks,
jirka

[BUG] powerpc: test_progs -t for_each faults kernel

[Jiri Olsa]

hi,
when running 'test_progs -t for_each' on powerpc we are getting
the fault below

it seems that for some reason the function callback address passed
to bpf_for_each_array_elem is wrong.. I wonder it's the powerpc
function pointers magic ;-)

it's the latest bpf-next/master, I can send .config if needed

thanks,
jirka


---
[  114.362271] kernel tried to execute user page (1) - exploit attempt? 
(uid: 0)
[  114.362284] BUG: Unable to handle kernel instruction fetch
[  114.362288] Faulting instruction address: 0x1
[  114.362294] Oops: Kernel access of bad area, sig: 11 [#1]
[  114.362299] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
[  114.362309] Modules linked in: bpf_testmod(OE) bonding tls rfkill 
pseries_rng ibmveth crct10dif_vpmsum drm fuse i2c_core 
drm_panel_orientation_quirks zram ip_tables ibmvscsi scsi_transport_srp 
vmx_crypto crc32c_vpmsum
[  114.362339] CPU: 5 PID: 935 Comm: test_progs Tainted: G   OE 
5.16.0-rc7+ #3
[  114.362345] NIP:  0001 LR: c0378a24 CTR: 00010001
[  114.362350] REGS: c00015ef36e0 TRAP: 0400   Tainted: G   OE  
(5.16.0-rc7+)
[  114.362355] MSR:  80004280b033   CR: 
88008248  XER: 2004
[  114.362371] CFAR: c0378a20 IRQMASK: 0 
   GPR00: c03789bc c00015ef3980 c2890900 
ccbb0200 
   GPR04: c00015ef39ec ccbb0310 c00015ef3a48 
 
   GPR08: 0001fd9a 0003  
2000 
   GPR12: 00010001 c0001ecab080  
 
   GPR16:    
c00017a48400 
   GPR20: 0001  c00015ef3ac0 
c00015ef3b7c 
   GPR24: c00015ef3b78 c00802e4 c00802e40048 
c00015ef3a48 
   GPR28: ccbb0310 00010001 ccbb0200 
0001 
[  114.362431] NIP [0001] 0x1
[  114.362436] LR [c0378a24] bpf_for_each_array_elem+0xc4/0x1c0
[  114.362443] Call Trace:
[  114.362445] [c00015ef3980] [c03789bc] 
bpf_for_each_array_elem+0x5c/0x1c0 (unreliable)
[  114.362454] [c00015ef3a20] [c008032267dc] 
bpf_prog_21bfb2cd0ec79d94_F+0xac/0x98d0
[  114.362461] [c00015ef3a90] [c0f54308] bpf_test_run+0x208/0x420
[  114.362469] [c00015ef3b50] [c0f551b8] 
bpf_prog_test_run_skb+0x368/0x7a0
[  114.362478] [c00015ef3bf0] [c034bf40] __sys_bpf+0xc20/0x2e20
[  114.362486] [c00015ef3d90] [c034e1dc] sys_bpf+0x2c/0x40
[  114.362495] [c00015ef3db0] [c002d478] 
system_call_exception+0x188/0x360
[  114.362505] [c00015ef3e10] [c000bfe8] 
system_call_vectored_common+0xe8/0x278
[  114.362514] --- interrupt: 3000 at 0x7fff813dc9fc
[  114.362520] NIP:  7fff813dc9fc LR:  CTR: 
[  114.362526] REGS: c00015ef3e80 TRAP: 3000   Tainted: G   OE  
(5.16.0-rc7+)
[  114.362532] MSR:  8280f033   CR: 
48002848  XER: 
[  114.362552] IRQMASK: 0 
   GPR00: 0169 7fffe3429c10 7fff814e6f00 
000a 
   GPR04: 7fffe3429cb8 0090  
0008 
   GPR08: 000a   
 
   GPR12:  7fff8164b7d0  
 
   GPR16:    
 
   GPR20:    
 
   GPR24: 101baa4c 7fffe342a520 1056f5d8 
7fff8163eb68 
   GPR28: 7fffe342a6a8 7fffe342a4f8 0004 
7fffe3429c10 
[  114.362626] NIP [7fff813dc9fc] 0x7fff813dc9fc
[  114.362631] LR [] 0x0
[  114.362636] --- interrupt: 3000
[  114.362640] Instruction dump:
[  114.362646]        
 
[  114.362661]        
 
[  114.362675] ---[ end trace c044e1b381f36402 ]---

Re: [PATCH 1/2] tools/perf: Include global and local variants for p_stage_cyc sort key

[Jiri Olsa]

On Thu, Nov 25, 2021 at 08:18:50AM +0530, Athira Rajeev wrote:
> Sort key p_stage_cyc is used to present the latency
> cycles spend in pipeline stages. perf tool has local
> p_stage_cyc sort key to display this info. There is no
> global variant available for this sort key. local variant
> shows latency in a sinlge sample, whereas, global value
> will be useful to present the total latency (sum of
> latencies) in the hist entry. It represents latency
> number multiplied by the number of samples.
> 
> Add global (p_stage_cyc) and local variant
> (local_p_stage_cyc) for this sort key. Use the
> local_p_stage_cyc as default option for "mem" sort mode.
> Also add this to list of dynamic sort keys.
> 
> Signed-off-by: Athira Rajeev 
> Reported-by: Namhyung Kim 
> ---
>  tools/perf/util/hist.c |  4 +++-
>  tools/perf/util/hist.h |  3 ++-
>  tools/perf/util/sort.c | 34 +-
>  tools/perf/util/sort.h |  3 ++-
>  4 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b776465e04ef..0a8033b09e28 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -211,7 +211,9 @@ void hists__calc_col_len(struct hists *hists, struct 
> hist_entry *h)
>   hists__new_col_len(hists, HISTC_MEM_BLOCKED, 10);
>   hists__new_col_len(hists, HISTC_LOCAL_INS_LAT, 13);
>   hists__new_col_len(hists, HISTC_GLOBAL_INS_LAT, 13);
> - hists__new_col_len(hists, HISTC_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_LOCAL_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_GLOBAL_P_STAGE_CYC, 13);
> +
>   if (symbol_conf.nanosecs)
>   hists__new_col_len(hists, HISTC_TIME, 16);
>   else
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 5343b62476e6..2752ce681108 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -75,7 +75,8 @@ enum hist_column {
>   HISTC_MEM_BLOCKED,
>   HISTC_LOCAL_INS_LAT,
>   HISTC_GLOBAL_INS_LAT,
> - HISTC_P_STAGE_CYC,
> + HISTC_LOCAL_P_STAGE_CYC,
> + HISTC_GLOBAL_P_STAGE_CYC,
>   HISTC_NR_COLS, /* Last entry */
>  };
>  
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index e9216a292a04..e978f7883e07 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -37,7 +37,7 @@ const char  default_parent_pattern[] = 
> "^sys_|^do_page_fault";
>  const char   *parent_pattern = default_parent_pattern;
>  const char   *default_sort_order = "comm,dso,symbol";
>  const char   default_branch_sort_order[] = 
> "comm,dso_from,symbol_from,symbol_to,cycles";
> -const char   default_mem_sort_order[] = 
> "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,p_stage_cyc";
> +const char   default_mem_sort_order[] = 
> "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,local_p_stage_cyc";
>  const char   default_top_sort_order[] = "dso,symbol";
>  const char   default_diff_sort_order[] = "dso,symbol";
>  const char   default_tracepoint_sort_order[] = "trace";
> @@ -46,8 +46,8 @@ const char  *field_order;
>  regex_t  ignore_callees_regex;
>  int  have_ignore_callees = 0;
>  enum sort_mode   sort__mode = SORT_MODE__NORMAL;
> -const char   *dynamic_headers[] = {"local_ins_lat", "p_stage_cyc"};
> -const char   *arch_specific_sort_keys[] = {"p_stage_cyc"};
> +const char   *dynamic_headers[] = {"local_ins_lat", "ins_lat", 
> "local_p_stage_cyc", "p_stage_cyc"};

so you also add global ins_lat, right? will this change
some default behaviour?

> +const char   *arch_specific_sort_keys[] = {"local_p_stage_cyc", 
> "p_stage_cyc"};

nit.. both dynamic_headers and arch_specific_sort_keys could be static right?

thanks,
jirka

>  
>  /*
>   * Replaces all occurrences of a char used with the:
> @@ -1392,22 +1392,37 @@ struct sort_entry sort_global_ins_lat = {
>  };
>  
>  static int64_t
> -sort__global_p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry 
> *right)
> +sort__p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
>   return left->p_stage_cyc - right->p_stage_cyc;
>  }
>  
> +static int hist_entry__global_p_stage_cyc_snprintf(struct hist_entry *he, 
> char *bf,
> + size_t size, unsigned int width)
> +{
> + return repsep_snprintf(bf, size, "%-*u", width,
> + he->p_stage_cyc * he->stat.nr_events);
> +}
> +
> +
>  static int hist_entry__p_stage_cyc_snprintf(struct hist_entry *he, char *bf,
>   size_t size, unsigned int width)
>  {
>   return repsep_snprintf(bf, size, "%-*u", width, he->p_stage_cyc);
>  }
>  
> -struct sort_entry sort_p_stage_cyc = {
> - .se_header  = "Pipeline Stage Cycle",
> - .se_cmp = sort__global_p_stage_cyc_cmp,
> +struct sort_entry sort_local_p_stage_cyc = {
> + .se_header  = "Local Pipeline Stage Cycle",
> + 

Re: [PATCH 1/2] tools/perf: Include global and local variants for p_stage_cyc sort key

[Jiri Olsa]

On Mon, Nov 29, 2021 at 02:43:48PM +0530, Athira Rajeev wrote:
> 
> 
> > On 28-Nov-2021, at 10:04 PM, Jiri Olsa  wrote:
> > 
> > On Thu, Nov 25, 2021 at 08:18:50AM +0530, Athira Rajeev wrote:
> >> Sort key p_stage_cyc is used to present the latency
> >> cycles spend in pipeline stages. perf tool has local
> >> p_stage_cyc sort key to display this info. There is no
> >> global variant available for this sort key. local variant
> >> shows latency in a sinlge sample, whereas, global value
> >> will be useful to present the total latency (sum of
> >> latencies) in the hist entry. It represents latency
> >> number multiplied by the number of samples.
> >> 
> >> Add global (p_stage_cyc) and local variant
> >> (local_p_stage_cyc) for this sort key. Use the
> >> local_p_stage_cyc as default option for "mem" sort mode.
> >> Also add this to list of dynamic sort keys.
> >> 
> >> Signed-off-by: Athira Rajeev 
> >> Reported-by: Namhyung Kim 
> > 
> > I can't apply this to Arnaldo's perf/core, could you please rebase?
> > 
> > patching file util/hist.c
> > patching file util/hist.h
> > patching file util/sort.c
> > Hunk #3 FAILED at 1392.
> > Hunk #4 succeeded at 1878 (offset 20 lines).
> > 1 out of 4 hunks FAILED -- saving rejects to file util/sort.c.rej
> > patching file util/sort.h
> > 
> > thanks,
> > jirka
> 
> Hi Jiri,
> 
> Thanks for checking this patch. 
> 
> Actually these changes are on top of three other fixes from Namhyung which 
> are already part of upstream. Below are the commits.
> 
> 784e8adda4cd ("perf sort: Fix the 'weight' sort key behavior”)
> 4d03c75363ee ("perf sort: Fix the 'ins_lat' sort key behavior”)
> db4b28402909 ("perf sort: Fix the 'p_stage_cyc' sort key behavior”)
> 
> I checked in Arnaldo’s perf/core, but these commits are not there. But I 
> could see them in 'tmp.perf/urgent'
> I think perf/core is not yet updated.

ah ok, I got it applied on perf/urgent, thanks

jirka

> 
> Thanks
> Athira Rajeev
> 
> > 
> >> ---
> >> tools/perf/util/hist.c |  4 +++-
> >> tools/perf/util/hist.h |  3 ++-
> >> tools/perf/util/sort.c | 34 +-
> >> tools/perf/util/sort.h |  3 ++-
> >> 4 files changed, 32 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> >> index b776465e04ef..0a8033b09e28 100644
> >> --- a/tools/perf/util/hist.c
> >> +++ b/tools/perf/util/hist.c
> >> @@ -211,7 +211,9 @@ void hists__calc_col_len(struct hists *hists, struct 
> >> hist_entry *h)
> >>hists__new_col_len(hists, HISTC_MEM_BLOCKED, 10);
> >>hists__new_col_len(hists, HISTC_LOCAL_INS_LAT, 13);
> >>hists__new_col_len(hists, HISTC_GLOBAL_INS_LAT, 13);
> >> -  hists__new_col_len(hists, HISTC_P_STAGE_CYC, 13);
> >> +  hists__new_col_len(hists, HISTC_LOCAL_P_STAGE_CYC, 13);
> >> +  hists__new_col_len(hists, HISTC_GLOBAL_P_STAGE_CYC, 13);
> >> +
> >>if (symbol_conf.nanosecs)
> >>hists__new_col_len(hists, HISTC_TIME, 16);
> >>else
> >> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> >> index 5343b62476e6..2752ce681108 100644
> >> --- a/tools/perf/util/hist.h
> >> +++ b/tools/perf/util/hist.h
> >> @@ -75,7 +75,8 @@ enum hist_column {
> >>HISTC_MEM_BLOCKED,
> >>HISTC_LOCAL_INS_LAT,
> >>HISTC_GLOBAL_INS_LAT,
> >> -  HISTC_P_STAGE_CYC,
> >> +  HISTC_LOCAL_P_STAGE_CYC,
> >> +  HISTC_GLOBAL_P_STAGE_CYC,
> >>HISTC_NR_COLS, /* Last entry */
> >> };
> >> 
> >> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> >> index e9216a292a04..e978f7883e07 100644
> >> --- a/tools/perf/util/sort.c
> >> +++ b/tools/perf/util/sort.c
> >> @@ -37,7 +37,7 @@ const char   default_parent_pattern[] = 
> >> "^sys_|^do_page_fault";
> >> const char *parent_pattern = default_parent_pattern;
> >> const char *default_sort_order = "comm,dso,symbol";
> >> const char default_branch_sort_order[] = 
> >> "comm,dso_from,symbol_from,symbol_to,cycles";
> >> -const chardefault_mem_sort_order[] = 
> >> "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,p_stage_cyc";
> >> +const chardefault_mem_sort_order[] = 
> >> "local_w

Re: [PATCH 1/2] tools/perf: Include global and local variants for p_stage_cyc sort key

[Jiri Olsa]

On Thu, Nov 25, 2021 at 08:18:50AM +0530, Athira Rajeev wrote:
> Sort key p_stage_cyc is used to present the latency
> cycles spend in pipeline stages. perf tool has local
> p_stage_cyc sort key to display this info. There is no
> global variant available for this sort key. local variant
> shows latency in a sinlge sample, whereas, global value
> will be useful to present the total latency (sum of
> latencies) in the hist entry. It represents latency
> number multiplied by the number of samples.
> 
> Add global (p_stage_cyc) and local variant
> (local_p_stage_cyc) for this sort key. Use the
> local_p_stage_cyc as default option for "mem" sort mode.
> Also add this to list of dynamic sort keys.
> 
> Signed-off-by: Athira Rajeev 
> Reported-by: Namhyung Kim 

I can't apply this to Arnaldo's perf/core, could you please rebase?

patching file util/hist.c
patching file util/hist.h
patching file util/sort.c
Hunk #3 FAILED at 1392.
Hunk #4 succeeded at 1878 (offset 20 lines).
1 out of 4 hunks FAILED -- saving rejects to file util/sort.c.rej
patching file util/sort.h

thanks,
jirka

> ---
>  tools/perf/util/hist.c |  4 +++-
>  tools/perf/util/hist.h |  3 ++-
>  tools/perf/util/sort.c | 34 +-
>  tools/perf/util/sort.h |  3 ++-
>  4 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b776465e04ef..0a8033b09e28 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -211,7 +211,9 @@ void hists__calc_col_len(struct hists *hists, struct 
> hist_entry *h)
>   hists__new_col_len(hists, HISTC_MEM_BLOCKED, 10);
>   hists__new_col_len(hists, HISTC_LOCAL_INS_LAT, 13);
>   hists__new_col_len(hists, HISTC_GLOBAL_INS_LAT, 13);
> - hists__new_col_len(hists, HISTC_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_LOCAL_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_GLOBAL_P_STAGE_CYC, 13);
> +
>   if (symbol_conf.nanosecs)
>   hists__new_col_len(hists, HISTC_TIME, 16);
>   else
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 5343b62476e6..2752ce681108 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -75,7 +75,8 @@ enum hist_column {
>   HISTC_MEM_BLOCKED,
>   HISTC_LOCAL_INS_LAT,
>   HISTC_GLOBAL_INS_LAT,
> - HISTC_P_STAGE_CYC,
> + HISTC_LOCAL_P_STAGE_CYC,
> + HISTC_GLOBAL_P_STAGE_CYC,
>   HISTC_NR_COLS, /* Last entry */
>  };
>  
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index e9216a292a04..e978f7883e07 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -37,7 +37,7 @@ const char  default_parent_pattern[] = 
> "^sys_|^do_page_fault";
>  const char   *parent_pattern = default_parent_pattern;
>  const char   *default_sort_order = "comm,dso,symbol";
>  const char   default_branch_sort_order[] = 
> "comm,dso_from,symbol_from,symbol_to,cycles";
> -const char   default_mem_sort_order[] = 
> "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,p_stage_cyc";
> +const char   default_mem_sort_order[] = 
> "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,local_p_stage_cyc";
>  const char   default_top_sort_order[] = "dso,symbol";
>  const char   default_diff_sort_order[] = "dso,symbol";
>  const char   default_tracepoint_sort_order[] = "trace";
> @@ -46,8 +46,8 @@ const char  *field_order;
>  regex_t  ignore_callees_regex;
>  int  have_ignore_callees = 0;
>  enum sort_mode   sort__mode = SORT_MODE__NORMAL;
> -const char   *dynamic_headers[] = {"local_ins_lat", "p_stage_cyc"};
> -const char   *arch_specific_sort_keys[] = {"p_stage_cyc"};
> +const char   *dynamic_headers[] = {"local_ins_lat", "ins_lat", 
> "local_p_stage_cyc", "p_stage_cyc"};
> +const char   *arch_specific_sort_keys[] = {"local_p_stage_cyc", 
> "p_stage_cyc"};
>  
>  /*
>   * Replaces all occurrences of a char used with the:
> @@ -1392,22 +1392,37 @@ struct sort_entry sort_global_ins_lat = {
>  };
>  
>  static int64_t
> -sort__global_p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry 
> *right)
> +sort__p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
>   return left->p_stage_cyc - right->p_stage_cyc;
>  }
>  
> +static int hist_entry__global_p_stage_cyc_snprintf(struct hist_entry *he, 
> char *bf,
> + size_t size, unsigned int width)
> +{
> + return repsep_snprintf(bf, size, "%-*u", width,
> + he->p_stage_cyc * he->stat.nr_events);
> +}
> +
> +
>  static int hist_entry__p_stage_cyc_snprintf(struct hist_entry *he, char *bf,
>   size_t size, unsigned int width)
>  {
>   return repsep_snprintf(bf, size, "%-*u", width, he->p_stage_cyc);
>  }
>  
> -struct sort_entry sort_p_stage_cyc = {
> - .se_header  = "Pipeline Stage Cycle",
> - .se_cmp  

Re: [PATCH 1/2] powerpc/bpf: Fix detecting BPF atomic instructions

[Jiri Olsa]

On Thu, Jul 01, 2021 at 08:38:58PM +0530, Naveen N. Rao wrote:
> Commit 91c960b0056672 ("bpf: Rename BPF_XADD and prepare to encode other
> atomics in .imm") converted BPF_XADD to BPF_ATOMIC and added a way to
> distinguish instructions based on the immediate field. Existing JIT
> implementations were updated to check for the immediate field and to
> reject programs utilizing anything more than BPF_ADD (such as BPF_FETCH)
> in the immediate field.
> 
> However, the check added to powerpc64 JIT did not look at the correct
> BPF instruction. Due to this, such programs would be accepted and
> incorrectly JIT'ed resulting in soft lockups, as seen with the atomic
> bounds test. Fix this by looking at the correct immediate value.
> 
> Fixes: 91c960b0056672 ("bpf: Rename BPF_XADD and prepare to encode other 
> atomics in .imm")
> Reported-by: Jiri Olsa 
> Tested-by: Jiri Olsa 
> Signed-off-by: Naveen N. Rao 
> ---
> Hi Jiri,
> FYI: I made a small change in this patch -- using 'imm' directly, rather 
> than insn[i].imm. I've still added your Tested-by since this shouldn't 
> impact the fix in any way.

yep, it works nicely

thanks
jirka

> 
> - Naveen
> 
> 
>  arch/powerpc/net/bpf_jit_comp64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c 
> b/arch/powerpc/net/bpf_jit_comp64.c
> index 5cad5b5a7e9774..de8595880feec6 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -667,7 +667,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>* BPF_STX ATOMIC (atomic ops)
>*/
>   case BPF_STX | BPF_ATOMIC | BPF_W:
> - if (insn->imm != BPF_ADD) {
> + if (imm != BPF_ADD) {
>   pr_err_ratelimited(
>   "eBPF filter atomic op code %02x (@%d) 
> unsupported\n",
>   code, i);
> @@ -689,7 +689,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>   PPC_BCC_SHORT(COND_NE, tmp_idx);
>   break;
>   case BPF_STX | BPF_ATOMIC | BPF_DW:
> - if (insn->imm != BPF_ADD) {
> + if (imm != BPF_ADD) {
>   pr_err_ratelimited(
>   "eBPF filter atomic op code %02x (@%d) 
> unsupported\n",
>   code, i);
> -- 
> 2.31.1
> 

Re: [PATCH v2] lockdown,selinux: avoid bogus SELinux lockdown permission checks

[Jiri Olsa]

.880800]  _raw_spin_lock_irqsave+0x4d/0x90
>   [  730.881656]  ? __wake_up_common_lock+0x52/0x90
>   [  730.882532]  __wake_up_common_lock+0x52/0x90
>   [  730.883375]  audit_log_end+0x5b/0x100
>   [  730.884104]  slow_avc_audit+0x69/0x90
>   [  730.884836]  avc_has_perm+0x8b/0xb0
>   [  730.885532]  selinux_lockdown+0xa5/0xd0
>   [  730.886297]  security_locked_down+0x20/0x40
>   [  730.887133]  bpf_probe_read_compat+0x66/0xd0
>   [  730.887983]  bpf_prog_250599c5469ac7b5+0x10f/0x820
>   [  730.888917]  trace_call_bpf+0xe9/0x240
>   [  730.889672]  perf_trace_run_bpf_submit+0x4d/0xc0
>   [  730.890579]  perf_trace_sched_switch+0x142/0x180
>   [  730.891485]  ? __schedule+0x6d8/0xb20
>   [  730.892209]  __schedule+0x6d8/0xb20
>   [  730.892899]  schedule+0x5b/0xc0
>   [  730.893522]  exit_to_user_mode_prepare+0x11d/0x240
>   [  730.894457]  syscall_exit_to_user_mode+0x27/0x70
>   [  730.895361]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>   [...]
> 
> Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> Reported-by: Ondrej Mosnacek 
> Reported-by: Jakub Hrozek 
> Reported-by: Serhei Makarov 
> Signed-off-by: Daniel Borkmann 
> Cc: Stephen Smalley 
> Cc: Jerome Marchand 
> Cc: Frank Eigler 
> Cc: Jiri Olsa 
> Cc: Paul Moore 

found the original server and reproduced.. this patch fixes it for me 

Tested-by: Jiri Olsa 

thanks,
jirka

> ---
>  kernel/bpf/helpers.c |  6 --
>  kernel/trace/bpf_trace.c | 36 +---
>  2 files changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 73443498d88f..6f6e090c5310 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1069,11 +1069,13 @@ bpf_base_func_proto(enum bpf_func_id func_id)
>   case BPF_FUNC_probe_read_user:
>   return _probe_read_user_proto;
>   case BPF_FUNC_probe_read_kernel:
> - return _probe_read_kernel_proto;
> + return security_locked_down(LOCKDOWN_BPF_READ) < 0 ?
> +NULL : _probe_read_kernel_proto;
>   case BPF_FUNC_probe_read_user_str:
>   return _probe_read_user_str_proto;
>   case BPF_FUNC_probe_read_kernel_str:
> - return _probe_read_kernel_str_proto;
> + return security_locked_down(LOCKDOWN_BPF_READ) < 0 ?
> +NULL : _probe_read_kernel_str_proto;
>   case BPF_FUNC_snprintf_btf:
>   return _snprintf_btf_proto;
>   case BPF_FUNC_snprintf:
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d2d7cf6cfe83..3df43d89d642 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -215,16 +215,10 @@ const struct bpf_func_proto 
> bpf_probe_read_user_str_proto = {
>  static __always_inline int
>  bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr)
>  {
> - int ret = security_locked_down(LOCKDOWN_BPF_READ);
> + int ret = copy_from_kernel_nofault(dst, unsafe_ptr, size);
>  
>   if (unlikely(ret < 0))
> - goto fail;
> - ret = copy_from_kernel_nofault(dst, unsafe_ptr, size);
> - if (unlikely(ret < 0))
> - goto fail;
> - return ret;
> -fail:
> - memset(dst, 0, size);
> + memset(dst, 0, size);
>   return ret;
>  }
>  
> @@ -246,11 +240,6 @@ const struct bpf_func_proto bpf_probe_read_kernel_proto 
> = {
>  static __always_inline int
>  bpf_probe_read_kernel_str_common(void *dst, u32 size, const void *unsafe_ptr)
>  {
> - int ret = security_locked_down(LOCKDOWN_BPF_READ);
> -
> - if (unlikely(ret < 0))
> - goto fail;
> -
>   /*
>* The strncpy_from_kernel_nofault() call will likely not fill the
>* entire buffer, but that's okay in this circumstance as we're probing
> @@ -260,13 +249,10 @@ bpf_probe_read_kernel_str_common(void *dst, u32 size, 
> const void *unsafe_ptr)
>* code altogether don't copy garbage; otherwise length of string
>* is returned that can be used for bpf_perf_event_output() et al.
>*/
> - ret = strncpy_from_kernel_nofault(dst, unsafe_ptr, size);
> - if (unlikely(ret < 0))
> - goto fail;
> + int ret = strncpy_from_kernel_nofault(dst, unsafe_ptr, size);
>  
> - return ret;
> -fail:
> - memset(dst, 0, size);
> + if (unlikely(ret < 0))
> + memset(dst, 0, size);
>   return ret;
>  }
>  
> @@ -1011,16 +997,20 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const 
> struct bpf_prog *prog)
>   case BPF_FUNC_probe_read_user:
>   return _probe_read_user_proto;

Re: [PATCH v2] lockdown,selinux: avoid bogus SELinux lockdown permission checks

[Jiri Olsa]

On Fri, May 28, 2021 at 11:56:02AM +0200, Daniel Borkmann wrote:

SNIP

> 
> Ondrej / Paul / Jiri: at least for the BPF tracing case specifically (I 
> haven't looked
> at the rest but it's also kind of independent), the attached fix should 
> address both
> reported issues, please take a look & test.
> 
> Thanks a lot,
> Daniel

> From 5893ad528dc0a0a68933b8f2a81b18d3f539660d Mon Sep 17 00:00:00 2001
> From: Daniel Borkmann 
> Date: Fri, 28 May 2021 09:16:31 +
> Subject: [PATCH bpf] bpf, audit, lockdown: Fix bogus SELinux lockdown 
> permission checks
> 
> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> added an implementation of the locked_down LSM hook to SELinux, with the aim
> to restrict which domains are allowed to perform operations that would breach
> lockdown. This is indirectly also getting audit subsystem involved to report
> events. The latter is problematic, as reported by Ondrej and Serhei, since it
> can bring down the whole system via audit:
> 
>   i) The audit events that are triggered due to calls to 
> security_locked_down()
>  can OOM kill a machine, see below details [0].
> 
>  ii) It seems to be causing a deadlock via slow_avc_audit() -> audit_log_end()
>  when presumingly trying to wake up kauditd [1].
> 
> Fix both at the same time by taking a completely different approach, that is,
> move the check into the program verification phase where we actually retrieve
> the func proto. This also reliably gets the task (current) that is trying to
> install the tracing program, e.g. bpftrace/bcc/perf/systemtap/etc, and it also
> fixes the OOM since we're moving this out of the BPF helpers which can be 
> called
> millions of times per second.

nice idea.. I'll try to reproduce and test

jirka

Re: [PATCH v2] lockdown,selinux: avoid bogus SELinux lockdown permission checks

[Jiri Olsa]

On Fri, May 28, 2021 at 09:09:57AM +0200, Daniel Borkmann wrote:
> On 5/28/21 3:37 AM, Paul Moore wrote:
> > On Mon, May 17, 2021 at 5:22 AM Ondrej Mosnacek  wrote:
> > > 
> > > Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> > > lockdown") added an implementation of the locked_down LSM hook to
> > > SELinux, with the aim to restrict which domains are allowed to perform
> > > operations that would breach lockdown.
> > > 
> > > However, in several places the security_locked_down() hook is called in
> > > situations where the current task isn't doing any action that would
> > > directly breach lockdown, leading to SELinux checks that are basically
> > > bogus.
> > > 
> > > Since in most of these situations converting the callers such that
> > > security_locked_down() is called in a context where the current task
> > > would be meaningful for SELinux is impossible or very non-trivial (and
> > > could lead to TOCTOU issues for the classic Lockdown LSM
> > > implementation), fix this by modifying the hook to accept a struct cred
> > > pointer as argument, where NULL will be interpreted as a request for a
> > > "global", task-independent lockdown decision only. Then modify SELinux
> > > to ignore calls with cred == NULL.
> > 
> > I'm not overly excited about skipping the access check when cred is
> > NULL.  Based on the description and the little bit that I've dug into
> > thus far it looks like using SECINITSID_KERNEL as the subject would be
> > much more appropriate.  *Something* (the kernel in most of the
> > relevant cases it looks like) is requesting that a potentially
> > sensitive disclosure be made, and ignoring it seems like the wrong
> > thing to do.  Leaving the access control intact also provides a nice
> > avenue to audit these requests should users want to do that.
> 
> I think the rationale/workaround for ignoring calls with cred == NULL (or the 
> previous
> patch with the unimplemented hook) from Ondrej was two-fold, at least 
> speaking for his
> seen tracing cases:
> 
>   i) The audit events that are triggered due to calls to 
> security_locked_down()
>  can OOM kill a machine, see below details [0].
> 
>  ii) It seems to be causing a deadlock via slow_avc_audit() -> audit_log_end()
>  when presumingly trying to wake up kauditd [1].

hi,
I saw the same deadlock, ended up with this sequence:

  rq_lock(rq) -> trace_sched_switch -> bpf_prog -> selinux_lockdown -> 
audit_log_end -> wake_up_interruptible -> try_to_wake_up -> rq_lock(rq)

problem is that trace_sched_switch already holds rq_lock

I had powerpc server where I could reproduce this easily,
but now for some reason I can't hit the issue anymore

jirka

> 
> How would your suggestion above solve both i) and ii)?
> 
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1955585 :
> 
>   I starting seeing this with F-34. When I run a container that is traced 
> with eBPF
>   to record the syscalls it is doing, auditd is flooded with messages like:
> 
>   type=AVC msg=audit(1619784520.593:282387): avc:  denied  { confidentiality 
> } for
>pid=476 comm="auditd" lockdown_reason="use of bpf to read kernel RAM"
> scontext=system_u:system_r:auditd_t:s0 
> tcontext=system_u:system_r:auditd_t:s0 tclass=lockdown permissive=0
> 
>   This seems to be leading to auditd running out of space in the backlog 
> buffer and
>   eventually OOMs the machine.
> 
>   auditd running at 99% CPU presumably processing all the messages, 
> eventually I get:
>   Apr 30 12:20:42 fedora kernel: audit: backlog limit exceeded
>   Apr 30 12:20:42 fedora kernel: audit: backlog limit exceeded
>   Apr 30 12:20:42 fedora kernel: audit: audit_backlog=2152579 > 
> audit_backlog_limit=64
>   Apr 30 12:20:42 fedora kernel: audit: audit_backlog=2152626 > 
> audit_backlog_limit=64
>   Apr 30 12:20:42 fedora kernel: audit: audit_backlog=2152694 > 
> audit_backlog_limit=64
>   Apr 30 12:20:42 fedora kernel: audit: audit_lost=6878426 audit_rate_limit=0 
> audit_backlog_limit=64
>   Apr 30 12:20:45 fedora kernel: oci-seccomp-bpf invoked oom-killer: 
> gfp_mask=0x100cca(GFP_HIGHUSER_MOVABLE), order=0, oom_score_adj=-1000
>   Apr 30 12:20:45 fedora kernel: CPU: 0 PID: 13284 Comm: oci-seccomp-bpf Not 
> tainted 5.11.12-300.fc34.x86_64 #1
>   Apr 30 12:20:45 fedora kernel: Hardware name: QEMU Standard PC (i440FX + 
> PIIX, 1996), BIOS 1.13.0-2.fc32 04/01/2014
> 
> [1] 
> https://lore.kernel.org/linux-audit/canyvdqn7h5tvp47fbycrasv4xf07eubsdwt_edchxjuj43j...@mail.gmail.com/
>  :
> 
>   Upstream kernel 5.11.0-rc7 and later was found to deadlock during a 
> bpf_probe_read_compat()
>   call within a sched_switch tracepoint. The problem is reproducible with the 
> reg_alloc3
>   testcase from SystemTap's BPF backend testsuite on x86_64 as well as the 
> runqlat,runqslower
>   tools from bcc on ppc64le. Example stack trace from [1]:
> 
>   [  730.868702] stack backtrace:
>   [  730.869590] CPU: 1 PID: 701 Comm: in:imjournal Not tainted, 
> 

Re: [PATCH V2 3/5] tools/perf: Add powerpc support for PERF_SAMPLE_WEIGHT_STRUCT

[Jiri Olsa]

On Mon, Mar 22, 2021 at 10:57:25AM -0400, Athira Rajeev wrote:
> Add arch specific arch_evsel__set_sample_weight() to set the new
> sample type for powerpc.
> 
> Add arch specific arch_perf_parse_sample_weight() to store the
> sample->weight values depending on the sample type applied.
> if the new sample type (PERF_SAMPLE_WEIGHT_STRUCT) is applied,
> store only the lower 32 bits to sample->weight. If sample type
> is 'PERF_SAMPLE_WEIGHT', store the full 64-bit to sample->weight.
> 
> Signed-off-by: Athira Rajeev 
> ---
>  tools/perf/arch/powerpc/util/Build   |  2 ++
>  tools/perf/arch/powerpc/util/event.c | 32 
>  tools/perf/arch/powerpc/util/evsel.c |  8 
>  3 files changed, 42 insertions(+)
>  create mode 100644 tools/perf/arch/powerpc/util/event.c
>  create mode 100644 tools/perf/arch/powerpc/util/evsel.c
> 
> diff --git a/tools/perf/arch/powerpc/util/Build 
> b/tools/perf/arch/powerpc/util/Build
> index b7945e5a543b..8a79c4126e5b 100644
> --- a/tools/perf/arch/powerpc/util/Build
> +++ b/tools/perf/arch/powerpc/util/Build
> @@ -4,6 +4,8 @@ perf-y += kvm-stat.o
>  perf-y += perf_regs.o
>  perf-y += mem-events.o
>  perf-y += sym-handling.o
> +perf-y += evsel.o
> +perf-y += event.o
>  
>  perf-$(CONFIG_DWARF) += dwarf-regs.o
>  perf-$(CONFIG_DWARF) += skip-callchain-idx.o
> diff --git a/tools/perf/arch/powerpc/util/event.c 
> b/tools/perf/arch/powerpc/util/event.c
> new file mode 100644
> index ..f49d32c2c8ae
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/event.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include 
> +#include 
> +#include 
> +
> +#include "../../../util/event.h"
> +#include "../../../util/synthetic-events.h"
> +#include "../../../util/machine.h"
> +#include "../../../util/tool.h"
> +#include "../../../util/map.h"
> +#include "../../../util/debug.h"

nit, just #include "utils/...h" should work no?

other than that, the patchset looks ok to me

Acked-by: Jiri Olsa 

thanks,
jirka

> +
> +void arch_perf_parse_sample_weight(struct perf_sample *data,
> +const __u64 *array, u64 type)
> +{
> + union perf_sample_weight weight;
> +
> + weight.full = *array;
> + if (type & PERF_SAMPLE_WEIGHT)
> + data->weight = weight.full;
> + else
> + data->weight = weight.var1_dw;
> +}
> +
> +void arch_perf_synthesize_sample_weight(const struct perf_sample *data,
> + __u64 *array, u64 type)
> +{
> + *array = data->weight;
> +
> + if (type & PERF_SAMPLE_WEIGHT_STRUCT)
> + *array &= 0x;
> +}
> diff --git a/tools/perf/arch/powerpc/util/evsel.c 
> b/tools/perf/arch/powerpc/util/evsel.c
> new file mode 100644
> index ..2f733cdc8dbb
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/evsel.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include 
> +#include "util/evsel.h"
> +
> +void arch_evsel__set_sample_weight(struct evsel *evsel)
> +{
> + evsel__set_sample_bit(evsel, WEIGHT_STRUCT);
> +}
> -- 
> 1.8.3.1
> 

Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc

[Jiri Olsa]

On Wed, Mar 17, 2021 at 05:01:27PM +0530, Athira Rajeev wrote:
>  class="ApplePlainTextBody"> class="ApplePlainTextBody">On 16-Mar-2021, at 
> 4:48 AM, Jiri Olsa jo...@redhat.com wrote:On Mon, Mar 15, 
> 2021 at 01:22:09PM +0530, Athira Rajeev wrote:SNIP type="cite">++static char *setup_dynamic_sort_keys(char 
> *str)+{+   
> unsigned int j;++ style="white-space:pre">   if (sort__mode == 
> SORT_MODE__MEMORY)+  
>  for 
> (j = 0; j  ARRAY_SIZE(dynamic_sort_keys_mem); j++)+ class="Apple-tab-span" style="white-space:pre">  class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre">if 
> (arch_support_dynamic_key(dynamic_sort_keys_mem[j])) {+ class="Apple-tab-span" style="white-space:pre">   class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre">str = 
> suffix_if_not_in(dynamic_sort_keys_mem[j], str);+ class="Apple-tab-span" style="white-space:pre">  class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre">if (str == 
> NULL)+   
>  class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre"> class="Apple-tab-span" style="white-space:pre">return 
> str;+
>  class="Apple-tab-span" style="white-space:pre">}++ class="Apple-tab-span" style="white-space:pre"> return 
> str;+}+static int __setup_sorting(struct evlist 
> *evlist){
> char *str;@@ -3050,6 +3085,12 @@ static int __setup_sorting(struct 
> evlist *evlist)  
> 
> }   
> }+  
> str = setup_dynamic_sort_keys(str);+ style="white-space:pre">if (str == NULL) {+ class="Apple-tab-span" style="white-space:pre">  class="Apple-tab-span" style="white-space:pre">pr_err("Not enough 
> memory to setup dynamic sort keys");+ style="white-space:pre"> style="white-space:pre">return -ENOMEM;+ class="Apple-tab-span" style="white-space:pre">
> }hum, so this is basicaly overloading the 
> default_mem_sort_order forarchitecture, right?then I think it'd 
> be easier just overload default_mem_sort_order directlyI was thinking 
> more about adding extra (arch specific) loop tosort_dimension__add or 
> somehow add arch's specific stuff 
> tomemory_sort_dimensionsHi Jiri,Above patch 
> was to append additional sort keys to sort order based onsort mode and 
> architecture support. I had initially thought of defining twoorders ( say 
> default_mem_sort_order plus mem_sort_order_pstage ). But ifnew sort keys 
> gets added for mem mode in future, we will need to keepupdating both 
> orders. So preferred the approach of "appending" supported sortkeys to 
> default order.Following your thought on using "sort_dimension__add", 
> I tried below approachwhich is easier. The new sort dimension 
> "p_stage_cyc" is presently only supportedon powerpc. For unsupported 
> platforms, we don't want to display itin the perf report output columns. 
> Hence added check in sort_dimension__add()and skip the sort key incase 
> its not applicable for particular arch.Please help to check if below 
> approach looks fine.diff --git 
> a/tools/perf/arch/powerpc/util/event.c 
> b/tools/perf/arch/powerpc/util/event.cindex b80fbee83b6e..7205767d75eb 
> 100644--- a/tools/perf/arch/powerpc/util/event.c+++ 
> b/tools/perf/arch/powerpc/util/event.c@@ -44,3 +44,10 @@ const char 
> *arch_perf_header_entry__add(const char *se_header)  class="Apple-tab-span" style="white-space:pre">   class="Apple-tab-span" style="white-space:pre">return "Dispatch 
> Cyc";  return 
> se_header; }++int arch_support_sort_key(const char 
> *sort_key)+{+ 
> if (!strcmp(sort_key, "p_stage_cyc"))+ style="white-space:pre">   style="white-space:pre">return 1;+ style="white-space:pre">  return 0;+}diff --git 
> a/tools/perf/util/event.h b/tools/perf/util/event.hindex 
> 65f89e80916f..612a92aaaefb 100644--- a/tools/perf/util/event.h+++ 
> b/tools/perf/util/event.h@@ -429,5 +429,6 @@ char *get_page_size_name(u64 
> size, char *str); void a

Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc

[Jiri Olsa]

On Mon, Mar 15, 2021 at 01:22:09PM +0530, Athira Rajeev wrote:

SNIP

> +
> +static char *setup_dynamic_sort_keys(char *str)
> +{
> + unsigned int j;
> +
> + if (sort__mode == SORT_MODE__MEMORY)
> + for (j = 0; j < ARRAY_SIZE(dynamic_sort_keys_mem); j++)
> + if (arch_support_dynamic_key(dynamic_sort_keys_mem[j])) 
> {
> + str = 
> suffix_if_not_in(dynamic_sort_keys_mem[j], str);
> + if (str == NULL)
> + return str;
> + }
> +
> + return str;
> +}
> +
>  static int __setup_sorting(struct evlist *evlist)
>  {
>   char *str;
> @@ -3050,6 +3085,12 @@ static int __setup_sorting(struct evlist *evlist)
>   }
>   }
>  
> + str = setup_dynamic_sort_keys(str);
> + if (str == NULL) {
> + pr_err("Not enough memory to setup dynamic sort keys");
> + return -ENOMEM;
> + }

hum, so this is basicaly overloading the default_mem_sort_order for
architecture, right?

then I think it'd be easier just overload default_mem_sort_order directly

I was thinking more about adding extra (arch specific) loop to
sort_dimension__add or somehow add arch's specific stuff to
memory_sort_dimensions

jirka

Re: [PATCH 2/4] tools/perf: Add dynamic headers for perf report columns

[Jiri Olsa]

On Tue, Mar 09, 2021 at 09:03:58AM -0500, Athira Rajeev wrote:
> Currently the header string for different columns in perf report
> is fixed. Some fields of perf sample could have different meaning
> for different architectures than the meaning conveyed by the header
> string. An example is the new field 'var2_w' of perf_sample_weight
> structure. This is presently captured as 'Local INSTR Latency' in
> perf mem report. But this could be used to denote a different latency
> cycle in another architecture.
> 
> Introduce a weak function arch_perf_header_entry__add() to set
> the arch specific header string for the fields which can contain dynamic
> header. If the architecture do not have this function, fall back to the
> default header string value.
> 
> Signed-off-by: Athira Rajeev 
> ---
>  tools/perf/util/event.h |  1 +
>  tools/perf/util/sort.c  | 19 ++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index f603edbbbc6f..89b149e2e70a 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -427,5 +427,6 @@ void  cpu_map_data__synthesize(struct 
> perf_record_cpu_map_data *data, struct per
>  
>  void arch_perf_parse_sample_weight(struct perf_sample *data, const __u64 
> *array, u64 type);
>  void arch_perf_synthesize_sample_weight(const struct perf_sample *data, 
> __u64 *array, u64 type);
> +const char *arch_perf_header_entry__add(const char *se_header);
>  
>  #endif /* __PERF_RECORD_H */
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 0d5ad42812b9..741a6df29fa0 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include "mem-events.h"
>  #include "annotate.h"
> +#include "event.h"
>  #include "time-utils.h"
>  #include "cgroup.h"
>  #include "machine.h"
> @@ -45,6 +46,7 @@
>  regex_t  ignore_callees_regex;
>  int  have_ignore_callees = 0;
>  enum sort_mode   sort__mode = SORT_MODE__NORMAL;
> +const char   *dynamic_headers[] = {"local_ins_lat"};
>  
>  /*
>   * Replaces all occurrences of a char used with the:
> @@ -1816,6 +1818,16 @@ struct sort_dimension {
>   int taken;
>  };
>  
> +const char * __weak arch_perf_header_entry__add(const char *se_header)

no need for the __add suffix in here

jirka

> +{
> + return se_header;
> +}
> +
> +static void sort_dimension_add_dynamic_header(struct sort_dimension *sd)
> +{
> + sd->entry->se_header = 
> arch_perf_header_entry__add(sd->entry->se_header);
> +}
> +
>  #define DIM(d, n, func) [d] = { .name = n, .entry = &(func) }
>  
>  static struct sort_dimension common_sort_dimensions[] = {
> @@ -2739,11 +2751,16 @@ int sort_dimension__add(struct perf_hpp_list *list, 
> const char *tok,
>   struct evlist *evlist,
>   int level)
>  {
> - unsigned int i;
> + unsigned int i, j;
>  
>   for (i = 0; i < ARRAY_SIZE(common_sort_dimensions); i++) {
>   struct sort_dimension *sd = _sort_dimensions[i];
>  
> + for (j = 0; j < ARRAY_SIZE(dynamic_headers); j++) {
> + if (!strcmp(dynamic_headers[j], sd->name))
> + sort_dimension_add_dynamic_header(sd);
> + }
> +
>   if (strncasecmp(tok, sd->name, strlen(tok)))
>   continue;
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc

[Jiri Olsa]

On Tue, Mar 09, 2021 at 09:04:00AM -0500, Athira Rajeev wrote:
> The pipeline stage cycles details can be recorded on powerpc from
> the contents of Performance Monitor Unit (PMU) registers. On
> ISA v3.1 platform, sampling registers exposes the cycles spent in
> different pipeline stages. Patch adds perf tools support to present
> two of the cycle counter information along with memory latency (weight).
> 
> Re-use the field 'ins_lat' for storing the first pipeline stage cycle.
> This is stored in 'var2_w' field of 'perf_sample_weight'.
> 
> Add a new field 'p_stage_cyc' to store the second pipeline stage cycle
> which is stored in 'var3_w' field of perf_sample_weight.
> 
> Add new sort function 'Pipeline Stage Cycle' and include this in
> default_mem_sort_order[]. This new sort function may be used to denote
> some other pipeline stage in another architecture. So add this to
> list of sort entries that can have dynamic header string.
> 
> Signed-off-by: Athira Rajeev 
> ---
>  tools/perf/Documentation/perf-report.txt |  1 +
>  tools/perf/arch/powerpc/util/event.c | 18 --
>  tools/perf/util/event.h  |  1 +
>  tools/perf/util/hist.c   | 11 ---
>  tools/perf/util/hist.h   |  1 +
>  tools/perf/util/session.c|  4 +++-
>  tools/perf/util/sort.c   | 24 ++--
>  tools/perf/util/sort.h   |  2 ++
>  8 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-report.txt 
> b/tools/perf/Documentation/perf-report.txt
> index f546b5e9db05..9691d9c227ba 100644
> --- a/tools/perf/Documentation/perf-report.txt
> +++ b/tools/perf/Documentation/perf-report.txt
> @@ -112,6 +112,7 @@ OPTIONS
>   - ins_lat: Instruction latency in core cycles. This is the global 
> instruction
> latency
>   - local_ins_lat: Local instruction latency version
> + - p_stage_cyc: Number of cycles spent in a pipeline stage.

please specify in here that it's ppc only

SNIP

> +struct sort_entry sort_p_stage_cyc = {
> + .se_header  = "Pipeline Stage Cycle",
> + .se_cmp = sort__global_p_stage_cyc_cmp,
> + .se_snprintf= hist_entry__p_stage_cyc_snprintf,
> + .se_width_idx   = HISTC_P_STAGE_CYC,
> +};
> +
>  struct sort_entry sort_mem_daddr_sym = {
>   .se_header  = "Data Symbol",
>   .se_cmp = sort__daddr_cmp,
> @@ -1853,6 +1872,7 @@ static void sort_dimension_add_dynamic_header(struct 
> sort_dimension *sd)
>   DIM(SORT_CODE_PAGE_SIZE, "code_page_size", sort_code_page_size),
>   DIM(SORT_LOCAL_INS_LAT, "local_ins_lat", sort_local_ins_lat),
>   DIM(SORT_GLOBAL_INS_LAT, "ins_lat", sort_global_ins_lat),
> + DIM(SORT_P_STAGE_CYC, "p_stage_cyc", sort_p_stage_cyc),

this might be out of scope for this patch, but would it make sense
to add arch specific sort dimension? so the specific column is
not even visible on arch that it's not supported on


>  };
>  
>  #undef DIM
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 63f67a3f3630..23b20cbbc846 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -51,6 +51,7 @@ struct he_stat {
>   u64 period_guest_us;
>   u64 weight;
>   u64 ins_lat;
> + u64 p_stage_cyc;
>   u32 nr_events;
>  };
>  
> @@ -234,6 +235,7 @@ enum sort_type {
>   SORT_CODE_PAGE_SIZE,
>   SORT_LOCAL_INS_LAT,
>   SORT_GLOBAL_INS_LAT,
> + SORT_P_STAGE_CYC,

we could have the whole 'SORT_PEPELINE_STAGE_CYC',
so it's more obvious

thanks,
jirka

Re: [PATCH] powerpc: Fix instruction encoding for lis in ppc_function_entry()

[Jiri Olsa]

On Thu, Mar 04, 2021 at 07:34:11AM +0530, Naveen N. Rao wrote:
> 'lis r2,N' is 'addis r2,0,N' and the instruction encoding in the macro
> LIS_R2 is incorrect (it currently maps to 'addis 0,r2,N'). Fix the same.
> 
> Fixes: c71b7eff426fa7 ("powerpc: Add ABIv2 support to ppc_function_entry")
> Reported-by: Jiri Olsa 
> Signed-off-by: Naveen N. Rao 
> ---
>  arch/powerpc/include/asm/code-patching.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h 
> b/arch/powerpc/include/asm/code-patching.h
> index eacc9102c2515c..d5b3c3bb95b400 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -73,7 +73,7 @@ void __patch_exception(int exc, unsigned long addr);
>  #endif
>  
>  #define OP_RT_RA_MASK0xUL
> -#define LIS_R2   0x3c02UL
> +#define LIS_R2   0x3c40UL
>  #define ADDIS_R2_R12 0x3c4cUL
>  #define ADDI_R2_R2   0x3842UL
>  
> 
> base-commit: 91966823812efbd175f904599e5cf2a854b39809
> -- 
> 2.25.1
> 

I tested the new define value with uprobe issue we were dealing with
and now it matches the instruction, so it works for me

thanks a lot for the fix,
jirka

Re: [PATCH] tools/perf: Fix powerpc gap between kernel end and module start

[Jiri Olsa]

On Mon, Dec 28, 2020 at 09:14:14PM -0500, Athira Rajeev wrote:

SNIP

> c2799370 b backtrace_flag
> c2799378 B radix_tree_node_cachep
> c2799380 B __bss_stop
> c27a B _end
> c0080389 t icmp_checkentry  [ip_tables]
> c00803890038 t ipt_alloc_initial_table  [ip_tables]
> c00803890468 T ipt_do_table [ip_tables]
> c00803890de8 T ipt_unregister_table_pre_exit[ip_tables]
> ...
> 
> Perf calls function symbols__fixup_end() which sets the end of symbol
> to 0xc0080389, which is the next address and this is the start
> address of first module (icmp_checkentry in above) which will make the
> huge symbol size of 0x8010f.
> 
> After symbols__fixup_end:
> symbols__fixup_end: sym->name: _end, sym->start: 0xc27a,
> sym->end: 0xc0080389
> 
> On powerpc, kernel text segment is located at 0xc000
> whereas the modules are located at very high memory addresses,
> 0xc0080xxx. Since the gap between end of kernel text segment
> and beginning of first module's address is high, histogram allocation
> using calloc fails.
> 
> Fix this by detecting the kernel's last symbol and limiting
> the range of last kernel symbol to pagesize.
> 
> Signed-off-by: Athira Rajeev

I can't test, but since the same approach works for arm and s390,
this also looks ok

Acked-by: Jiri Olsa 

thanks,
jirka

> ---
>  tools/perf/arch/powerpc/util/Build |  1 +
>  tools/perf/arch/powerpc/util/machine.c | 24 
>  2 files changed, 25 insertions(+)
>  create mode 100644 tools/perf/arch/powerpc/util/machine.c
> 
> diff --git a/tools/perf/arch/powerpc/util/Build 
> b/tools/perf/arch/powerpc/util/Build
> index e86e210bf514..b7945e5a543b 100644
> --- a/tools/perf/arch/powerpc/util/Build
> +++ b/tools/perf/arch/powerpc/util/Build
> @@ -1,4 +1,5 @@
>  perf-y += header.o
> +perf-y += machine.o
>  perf-y += kvm-stat.o
>  perf-y += perf_regs.o
>  perf-y += mem-events.o
> diff --git a/tools/perf/arch/powerpc/util/machine.c 
> b/tools/perf/arch/powerpc/util/machine.c
> new file mode 100644
> index ..c30e5cc88c16
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/machine.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +#include 
> +#include  // page_size
> +#include "debug.h"
> +#include "symbol.h"
> +
> +/* On powerpc kernel text segment start at memory addresses, 
> 0xc000
> + * whereas the modules are located at very high memory addresses,
> + * for example 0xc0080xxx. The gap between end of kernel text segment
> + * and beginning of first module's text segment is very high.
> + * Therefore do not fill this gap and do not assign it to the kernel dso map.
> + */
> +
> +void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
> +{
> + if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
> + /* Limit the range of last kernel symbol */
> + p->end += page_size;
> + else
> + p->end = c->start;
> + pr_debug4("%s sym:%s end:%#lx\n", __func__, p->name, p->end);
> +}
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 0/4] powerpc/perf: Add support for perf extended regs in powerpc

[Jiri Olsa]

On Thu, Jul 30, 2020 at 01:24:40PM +0530, Athira Rajeev wrote:
> 
> 
> > On 27-Jul-2020, at 10:46 PM, Athira Rajeev  
> > wrote:
> > 
> > Patch set to add support for perf extended register capability in
> > powerpc. The capability flag PERF_PMU_CAP_EXTENDED_REGS, is used to
> > indicate the PMU which support extended registers. The generic code
> > define the mask of extended registers as 0 for non supported architectures.
> > 
> > Patches 1 and 2 are the kernel side changes needed to include
> > base support for extended regs in powerpc and in power10.
> > Patches 3 and 4 are the perf tools side changes needed to support the
> > extended registers.
> > 
> 
> Hi Arnaldo, Jiri
> 
> please let me know if you have any comments/suggestions on this patch series 
> to add support for perf extended regs.

hi,
can't really tell for powerpc, but in general
perf tool changes look ok

jirka

[tip: perf/core] perf expr: Add expr_scanner_ctx object

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 871f9f599db8d9d2387c0717e712af405290edea
Gitweb:
https://git.kernel.org/tip/871f9f599db8d9d2387c0717e712af405290edea
Author:Jiri Olsa 
AuthorDate:Thu, 02 Apr 2020 02:03:35 +05:30
Committer: Arnaldo Carvalho de Melo 
CommitterDate: Thu, 16 Apr 2020 12:19:13 -03:00

perf expr: Add expr_scanner_ctx object

Add the expr_scanner_ctx object to hold user data for the expr scanner.
Currently it holds only start_token, Kajol Jain will use it to hold 24x7
runtime param.

Signed-off-by: Jiri Olsa 
Cc: Alexander Shishkin 
Cc: Andi Kleen 
Cc: Anju T Sudhakar 
Cc: Benjamin Herrenschmidt 
Cc: Greg Kroah-Hartman 
Cc: Jin Yao 
Cc: Joe Mario 
Cc: Kajol Jain 
Cc: Kan Liang 
Cc: Madhavan Srinivasan 
Cc: Mamatha Inamdar 
Cc: Mark Rutland 
Cc: Michael Ellerman 
Cc: Michael Petlan 
Cc: Namhyung Kim 
Cc: Paul Mackerras 
Cc: Peter Zijlstra 
Cc: Ravi Bangoria 
Cc: Sukadev Bhattiprolu 
Cc: Thomas Gleixner 
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lore.kernel.org/lkml/20200401203340.31402-3-kj...@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/expr.c |  6 --
 tools/perf/util/expr.h |  4 
 tools/perf/util/expr.l | 10 +-
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index c8ccc54..c3382d5 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -3,7 +3,6 @@
 #include 
 #include "expr.h"
 #include "expr-bison.h"
-#define YY_EXTRA_TYPE int
 #include "expr-flex.h"
 
 #ifdef PARSER_DEBUG
@@ -30,11 +29,14 @@ static int
 __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
  int start)
 {
+   struct expr_scanner_ctx scanner_ctx = {
+   .start_token = start,
+   };
YY_BUFFER_STATE buffer;
void *scanner;
int ret;
 
-   ret = expr_lex_init_extra(start, );
+   ret = expr_lex_init_extra(_ctx, );
if (ret)
return ret;
 
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index b9e53f2..0938ad1 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -15,6 +15,10 @@ struct expr_parse_ctx {
struct expr_parse_id ids[MAX_PARSE_ID];
 };
 
+struct expr_scanner_ctx {
+   int start_token;
+};
+
 void expr__ctx_init(struct expr_parse_ctx *ctx);
 void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char 
*expr);
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index eaad292..2582c24 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -76,13 +76,13 @@ sym [0-9a-zA-Z_\.:@]+
 symbol {spec}*{sym}*{spec}*{sym}*
 
 %%
-   {
-   int start_token;
+   struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
 
-   start_token = expr_get_extra(yyscanner);
+   {
+   int start_token = sctx->start_token;
 
-   if (start_token) {
-   expr_set_extra(NULL, yyscanner);
+   if (sctx->start_token) {
+   sctx->start_token = 0;
return start_token;
}
}

[tip: perf/core] perf expr: Add expr_ prefix for parse_ctx and parse_id

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: aecce63e2b98f28606b063949cca06facf215d6c
Gitweb:
https://git.kernel.org/tip/aecce63e2b98f28606b063949cca06facf215d6c
Author:Jiri Olsa 
AuthorDate:Thu, 02 Apr 2020 02:03:34 +05:30
Committer: Arnaldo Carvalho de Melo 
CommitterDate: Thu, 16 Apr 2020 12:19:13 -03:00

perf expr: Add expr_ prefix for parse_ctx and parse_id

Adding expr_ prefix for parse_ctx and parse_id, to straighten out the
expr* namespace.

There's no functional change.

Signed-off-by: Jiri Olsa 
Cc: Alexander Shishkin 
Cc: Andi Kleen 
Cc: Anju T Sudhakar 
Cc: Benjamin Herrenschmidt 
Cc: Greg Kroah-Hartman 
Cc: Jin Yao 
Cc: Joe Mario 
Cc: Kajol Jain 
Cc: Kan Liang 
Cc: Madhavan Srinivasan 
Cc: Mamatha Inamdar 
Cc: Mark Rutland 
Cc: Michael Ellerman 
Cc: Michael Petlan 
Cc: Namhyung Kim 
Cc: Paul Mackerras 
Cc: Peter Zijlstra 
Cc: Ravi Bangoria 
Cc: Sukadev Bhattiprolu 
Cc: Thomas Gleixner 
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lore.kernel.org/lkml/20200401203340.31402-2-kj...@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/tests/expr.c   |  4 ++--
 tools/perf/util/expr.c| 10 +-
 tools/perf/util/expr.h| 12 ++--
 tools/perf/util/expr.y|  6 +++---
 tools/perf/util/stat-shadow.c |  2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 28313e5..ea10fc4 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -6,7 +6,7 @@
 #include 
 #include 
 
-static int test(struct parse_ctx *ctx, const char *e, double val2)
+static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
 {
double val;
 
@@ -22,7 +22,7 @@ int test__expr(struct test *t __maybe_unused, int subtest 
__maybe_unused)
const char **other;
double val;
int i, ret;
-   struct parse_ctx ctx;
+   struct expr_parse_ctx ctx;
int num_other;
 
expr__ctx_init();
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index fd192dd..c8ccc54 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -11,7 +11,7 @@ extern int expr_debug;
 #endif
 
 /* Caller must make sure id is allocated */
-void expr__add_id(struct parse_ctx *ctx, const char *name, double val)
+void expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
 {
int idx;
 
@@ -21,13 +21,13 @@ void expr__add_id(struct parse_ctx *ctx, const char *name, 
double val)
ctx->ids[idx].val = val;
 }
 
-void expr__ctx_init(struct parse_ctx *ctx)
+void expr__ctx_init(struct expr_parse_ctx *ctx)
 {
ctx->num_ids = 0;
 }
 
 static int
-__expr__parse(double *val, struct parse_ctx *ctx, const char *expr,
+__expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
  int start)
 {
YY_BUFFER_STATE buffer;
@@ -52,7 +52,7 @@ __expr__parse(double *val, struct parse_ctx *ctx, const char 
*expr,
return ret;
 }
 
-int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr)
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char 
*expr)
 {
return __expr__parse(final_val, ctx, expr, EXPR_PARSE) ? -1 : 0;
 }
@@ -75,7 +75,7 @@ int expr__find_other(const char *expr, const char *one, const 
char ***other,
 int *num_other)
 {
int err, i = 0, j = 0;
-   struct parse_ctx ctx;
+   struct expr_parse_ctx ctx;
 
expr__ctx_init();
err = __expr__parse(NULL, , expr, EXPR_OTHER);
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 9377538..b9e53f2 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -5,19 +5,19 @@
 #define EXPR_MAX_OTHER 20
 #define MAX_PARSE_ID EXPR_MAX_OTHER
 
-struct parse_id {
+struct expr_parse_id {
const char *name;
double val;
 };
 
-struct parse_ctx {
+struct expr_parse_ctx {
int num_ids;
-   struct parse_id ids[MAX_PARSE_ID];
+   struct expr_parse_id ids[MAX_PARSE_ID];
 };
 
-void expr__ctx_init(struct parse_ctx *ctx);
-void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
-int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
+void expr__ctx_init(struct expr_parse_ctx *ctx);
+void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char 
*expr);
 int expr__find_other(const char *expr, const char *one, const char ***other,
int *num_other);
 
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 4720cbe..cd17486 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -15,7 +15,7 @@
 %define api.pure full
 
 %parse-param { double *final_val }
-%parse-param { struct parse_ctx *ctx }
+%parse-param { struct expr_parse_ctx *ctx }
 %parse-param {void *scanner}
 %lex-param {void* scanner}
 
@@ -39,14 

Re: [PATCH v8 0/7] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Jiri Olsa]

On Thu, Apr 02, 2020 at 02:03:33AM +0530, Kajol Jain wrote:
> Patchset adds json file metric support for the hv_24x7 socket/chip level
> events. "hv_24x7" pmu interface events needs system dependent parameter
> like socket/chip/core. For example, hv_24x7 chip level events needs
> specific chip-id to which the data is requested should be added as part
> of pmu events.
> 
> So to enable JSON file support to "hv_24x7" interface, patchset reads
> total number of sockets details in sysfs under 
> "/sys/devices/hv_24x7/interface/".
> 
> Second patch of the patchset adds expr_scanner_ctx object to hold user
> data for the expr scanner, which can be used to hold runtime parameter.
> 
> Patch 4 & 6 of the patchset handles perf tool plumbing needed to replace
> the "?" character in the metric expression to proper value and hv_24x7
> json metric file for different Socket/chip resources.
> 
> Patch set also enable Hz/hz prinitg for --metric-only option to print
> metric data for bus frequency.
> 
> Applied and tested all these patches cleanly on top of jiri's flex changes
> with the changes done by Kan Liang for "Support metric group constraint"
> patchset and made required changes.
> 
> Also apply this patch on top of the fix patch send earlier
> for printing metric name incase overlapping events.
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core=37cd7f65bf71a48f25eeb6d9be5dacb20d008ea6
> 
> Changelog:
> v7 -> v8
> - Add test case for testing parsing of "?" in metric expression
> - Reaname variables name to runtime

Acked-by: Jiri Olsa 

thanks,
jirka

Re: [PATCH v8 6/7] tools/perf: Enable Hz/hz prinitg for --metric-only option

[Jiri Olsa]

On Thu, Apr 02, 2020 at 02:03:39AM +0530, Kajol Jain wrote:
> Commit 54b5091606c18 ("perf stat: Implement --metric-only mode")
> added function 'valid_only_metric()' which drops "Hz" or "hz",
> if it is part of "ScaleUnit". This patch enable it since hv_24x7
> supports couple of frequency events.
> 
> Signed-off-by: Kajol Jain 
> ---
>  tools/perf/util/stat-display.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 9e757d18d713..679aaa655824 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -237,8 +237,6 @@ static bool valid_only_metric(const char *unit)
>   if (!unit)
>   return false;
>   if (strstr(unit, "/sec") ||
> - strstr(unit, "hz") ||
> - strstr(unit, "Hz") ||

will this change output of --metric-only for some setups then?

Andi, are you ok with this?

other than this, the patchset looks ok to me

thanks,
jirka

>   strstr(unit, "CPUs utilized"))
>   return false;
>   return true;
> -- 
> 2.21.0
> 

Re: [PATCH v7 3/6] perf/tools: Refactoring metricgroup__add_metric function

[Jiri Olsa]

On Fri, Mar 27, 2020 at 03:55:25PM +0530, Kajol Jain wrote:

SNIP

> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> +
> + return 0;
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  struct list_head *group_list)
>  {
> @@ -504,35 +538,12 @@ static int metricgroup__add_metric(const char *metric, 
> struct strbuf *events,
>   continue;
>   if (match_metric(pe->metric_group, metric) ||
>   match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
>  
>   pr_debug("metric expr %s for %s\n", pe->metric_expr, 
> pe->metric_name);
>  
> - if (expr__find_other(pe->metric_expr,
> -  NULL, , ) < 0)
> + ret = __metricgroup__add_metric(events, group_list, pe);
> + if (ret == -EINVAL || !ret)
>   continue;

what's the point of continue in here? it's end of the loop..

jirka

Re: [PATCH v7 3/6] perf/tools: Refactoring metricgroup__add_metric function

[Jiri Olsa]

On Fri, Mar 27, 2020 at 03:55:25PM +0530, Kajol Jain wrote:
> This patch refactor metricgroup__add_metric function where
> some part of it move to function metricgroup__add_metric_param.
> No logic change.
> 
> Signed-off-by: Kajol Jain 
> ---
>  tools/perf/util/metricgroup.c | 61 +--
>  1 file changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 926449a7cdbf..b905eaa907a7 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -485,6 +485,40 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +static int __metricgroup__add_metric(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr, NULL, , ) < 0)
> + return ret;

no need for ret variable, return -EINVAL directly

jirka

Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:

SNIP

>  
>  %%
>   struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> @@ -93,7 +106,7 @@ if { return IF; }
>  else { return ELSE; }
>  #smt_on  { return SMT_ON; }
>  {number} { return value(yyscanner, 10); }
> -{symbol} { return str(yyscanner, ID); }
> +{symbol} { return str(yyscanner, ID, sctx->runtime_param); }
>  "|"  { return '|'; }
>  "^"  { return '^'; }
>  "&"  { return '&'; }
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b905eaa907a7..66695b4a358d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -90,6 +90,7 @@ struct egroup {
>   const char *metric_name;
>   const char *metric_expr;
>   const char *metric_unit;
> + int param;

could you please use runtime_param everywhere? ... s/param/runtime_param/
or maybe just 'runtime' to keep it short..? it's param in any case ;-)

other than that it looks ok

jirka

Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index ea10fc4412c4..516504cf0ea5 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -10,7 +10,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e, 
> double val2)
>  {
>   double val;
>  
> - if (expr__parse(, ctx, e))
> + if (expr__parse(, ctx, e, 1))
>   TEST_ASSERT_VAL("parse test failed", 0);
>   TEST_ASSERT_VAL("unexpected value", val == val2);
>   return 0;
> @@ -44,15 +44,15 @@ int test__expr(struct test *t __maybe_unused, int subtest 
> __maybe_unused)
>   return ret;
>  
>   p = "FOO/0";
> - ret = expr__parse(, , p);
> + ret = expr__parse(, , p, 1);
>   TEST_ASSERT_VAL("division by zero", ret == -1);
>  
>   p = "BAR/";
> - ret = expr__parse(, , p);
> + ret = expr__parse(, , p, 1);
>   TEST_ASSERT_VAL("missing operand", ret == -1);
>  
>   TEST_ASSERT_VAL("find other",
> - expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", 
> , _other) == 0);
> + expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", 
> , _other, 1) == 0);
>   TEST_ASSERT_VAL("find other", num_other == 3);
>   TEST_ASSERT_VAL("find other", !strcmp(other[0], "BAR"));
>   TEST_ASSERT_VAL("find other", !strcmp(other[1], "BAZ"));

could we add test case for runtime param > 1 in here?
expr_parse should return value that would depend on this value..

jirka

Re: [PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Fri, Mar 20, 2020 at 06:24:04PM +0530, Kajol Jain wrote:
> Patch enhances current metric infrastructure to handle "?" in the metric
> expression. The "?" can be use for parameters whose value not known while
> creating metric events and which can be replace later at runtime to
> the proper value. It also add flexibility to create multiple events out
> of single metric event added in json file.
> 
> Patch adds function 'arch_get_runtimeparam' which is a arch specific
> function, returns the count of metric events need to be created.
> By default it return 1.
> 
> This infrastructure needed for hv_24x7 socket/chip level events.
> "hv_24x7" chip level events needs specific chip-id to which the
> data is requested. Function 'arch_get_runtimeparam' implemented
> in header.c which extract number of sockets from sysfs file
> "sockets" under "/sys/devices/hv_24x7/interface/".
> 
> 
> With this patch basically we are trying to create as many metric events
> as define by runtime_param.
> 
> For that one loop is added in function 'metricgroup__add_metric',
> which create multiple events at run time depend on return value of
> 'arch_get_runtimeparam' and merge that event in 'group_list'.
> 
> To achieve that we are actually passing this parameter value as part of
> `expr__find_other` function and changing "?" present in metric expression
> with this value.
> 
> As in our json file, there gonna be single metric event, and out of
> which we are creating multiple events, I am also merging this value
> to the original metric name to specify parameter value.
> 
> For example,
> command:# ./perf stat  -M PowerBUS_Frequency -C 0 -I 1000
> #   time counts unit events
>  1.000101867  9,356,933  hv_24x7/pm_pb_cyc,chip=0/ #  2.3 
> GHz  PowerBUS_Frequency_0
>  1.000101867  9,366,134  hv_24x7/pm_pb_cyc,chip=1/ #  2.3 
> GHz  PowerBUS_Frequency_1
>  2.000314878  9,365,868  hv_24x7/pm_pb_cyc,chip=0/ #  2.3 
> GHz  PowerBUS_Frequency_0
>  2.000314878  9,366,092  hv_24x7/pm_pb_cyc,chip=1/ #  2.3 
> GHz  PowerBUS_Frequency_1
> 
> So, here _0 and _1 after PowerBUS_Frequency specify parameter value.
> 
> As after adding this to group_list, again we call expr__parse
> in 'generic_metric' function present in util/stat-display.c.
> By this time again we need to pass this parameter value. So, now to get this 
> value
> actually I am trying to extract it from metric name itself. Because
> otherwise it gonna point to last updated value present in runtime_param.
> And gonna match for that value only.

so why can't we pass that param as integer value through the metric objects?

it get's created in metricgroup__add_metric_param:
  - as struct egroup *eg
  - we can add egroup::param and store the param value there

then in metricgroup__setup_events it moves to:
  - struct metric_expr *expr
  - we can add metric_expr::param to keep the param

then in perf_stat__print_shadow_stats there's:
  - struct metric_expr *mexp loop
  - calling generic_metric metric - we could call it with mexp::param
  - and pass the param to expr__parse

jirka

Re: [PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Fri, Mar 20, 2020 at 06:24:04PM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 52fb119d25c8..b4b91d8ad5be 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,8 +474,13 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
>  static int metricgroup__add_metric_param(struct strbuf *events,
> - struct list_head *group_list, struct pmu_event *pe)
> + struct list_head *group_list, struct pmu_event *pe, int param)
>  {
>  
>   const char **ids;
> @@ -483,7 +488,7 @@ static int metricgroup__add_metric_param(struct strbuf 
> *events,

could you please call this function __metricgroup__add_metric instead?

>   struct egroup *eg;
>   int ret = -EINVAL;
>  
> - if (expr__find_other(pe->metric_expr, NULL, , ) < 0)
> + if (expr__find_other(pe->metric_expr, NULL, , , param) < 0)
>   return ret;
>  
>   if (events->len > 0)
> @@ -502,11 +507,21 @@ static int metricgroup__add_metric_param(struct strbuf 
> *events,
>  
>   eg->ids = ids;
>   eg->idnum = idnum;
> - eg->metric_name = pe->metric_name;
> + if (strstr(pe->metric_expr, "?")) {
> + char value[PATH_MAX];
> +
> + sprintf(value, "%s%c%d", pe->metric_name, '_', param);
> + eg->metric_name = strdup(value);

how is eg->metric_name getting released?

> + if (!eg->metric_name) {
> + ret = -ENOMEM;
> + return ret;

return -ENOMEM; ??

> + }
> + }
> + else
> + eg->metric_name = pe->metric_name;
>   eg->metric_expr = pe->metric_expr;
>   eg->metric_unit = pe->unit;
>   list_add_tail(>nd, group_list);
> -
>   return 0;
>  }
>  

SNIP

thanks,
jirka

Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:

SBIP

> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> + int i, count;
> + int ret = -EINVAL;
> +
> + count = arch_get_runtimeparam();
> +
> + /* This loop is added to create multiple
> +  * events depend on count value and add
> +  * those events to group_list.
> +  */
> +
> + for (i = 0; i < count; i++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + char value[PATH_MAX];
> +
> + if (expr__find_other(pe->metric_expr, NULL, , , i) < 
> 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(*eg));
> + if (!eg) {
> + ret = -ENOMEM;
> + return ret;
> + }
> +
> + sprintf(value, "%s%c%d", pe->metric_name, '_', i);
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = strdup(value);
> + if (!eg->metric_name) {
> + ret = -ENOMEM;
> + return ret;
> + }
> +
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + if (ret != 0)
> + break;

again, this is part of metricgroup__add_metric_param no? why not use it?

jirka

Re: [PATCH v5 08/11] perf/tools: Refactoring metricgroup__add_metric function

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:30AM +0530, Kajol Jain wrote:
> This patch refactor metricgroup__add_metric function where
> some part of it move to function metricgroup__add_metric_param.
> No logic change.
> 
> Signed-off-by: Kajol Jain 
> ---
>  tools/perf/util/metricgroup.c | 63 +--
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..b4919bcfbd8b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,41 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +static int metricgroup__add_metric_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr, NULL, , , 1) < 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(*eg));
> + if (!eg)
> + ret = -ENOMEM;
> +
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + return ret;

return 0;

jirka

Re: [PATCH v5 08/11] perf/tools: Refactoring metricgroup__add_metric function

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:30AM +0530, Kajol Jain wrote:
> This patch refactor metricgroup__add_metric function where
> some part of it move to function metricgroup__add_metric_param.
> No logic change.
> 
> Signed-off-by: Kajol Jain 
> ---
>  tools/perf/util/metricgroup.c | 63 +--
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..b4919bcfbd8b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,41 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +static int metricgroup__add_metric_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr, NULL, , , 1) < 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(*eg));
> + if (!eg)
> + ret = -ENOMEM;

??? you need to return in here, eg is NULL

> +
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + return ret;
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  struct list_head *group_list)
>  {
> @@ -493,35 +528,13 @@ static int metricgroup__add_metric(const char *metric, 
> struct strbuf *events,
>   continue;
>   if (match_metric(pe->metric_group, metric) ||
>   match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
>  
>   pr_debug("metric expr %s for %s\n", pe->metric_expr, 
> pe->metric_name);
>  
> - if (expr__find_other(pe->metric_expr,
> -  NULL, , ) < 0)
> + ret = metricgroup__add_metric_param(events,
> + group_list, pe);
> + if (ret == -EINVAL)
>   continue;

previous code did 'continue' on ret < 0, why just -EINVAL now?

jirka

Re: [PATCH v5 08/11] perf/tools: Refactoring metricgroup__add_metric function

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:30AM +0530, Kajol Jain wrote:
> This patch refactor metricgroup__add_metric function where
> some part of it move to function metricgroup__add_metric_param.
> No logic change.

can't compile this change:

[jolsa@krava perf]$ make JOBS=1
  BUILD:   Doing 'make -j1' parallel build
  CC   util/metricgroup.o
util/metricgroup.c: In function ‘metricgroup__add_metric_param’:
util/metricgroup.c:486:6: error: too many arguments to function 
‘expr__find_other’
  486 |  if (expr__find_other(pe->metric_expr, NULL, , , 1) < 0)
  |  ^~~~
In file included from util/metricgroup.c:14:
util/expr.h:25:5: note: declared here
   25 | int expr__find_other(const char *expr, const char *one, const char 
***other,
  | ^~~~
mv: cannot stat 'util/.metricgroup.o.tmp': No such file or directory
make[4]: *** [/home/jolsa/kernel/linux-perf/tools/build/Makefile.build:97: 
util/metricgroup.o] Error 1
make[3]: *** [/home/jolsa/kernel/linux-perf/tools/build/Makefile.build:139: 
util] Error 2
make[2]: *** [Makefile.perf:616: perf-in.o] Error 2
make[1]: *** [Makefile.perf:225: sub-make] Error 2
make: *** [Makefile:70: all] Error 2

jirka

> 
> Signed-off-by: Kajol Jain 
> ---
>  tools/perf/util/metricgroup.c | 63 +--
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..b4919bcfbd8b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,41 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +static int metricgroup__add_metric_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr, NULL, , , 1) < 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(*eg));
> + if (!eg)
> + ret = -ENOMEM;
> +
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + return ret;
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  struct list_head *group_list)
>  {
> @@ -493,35 +528,13 @@ static int metricgroup__add_metric(const char *metric, 
> struct strbuf *events,
>   continue;
>   if (match_metric(pe->metric_group, metric) ||
>   match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
>  
>   pr_debug("metric expr %s for %s\n", pe->metric_expr, 
> pe->metric_name);
>  
> - if (expr__find_other(pe->metric_expr,
> -  NULL, , ) < 0)
> + ret = metricgroup__add_metric_param(events,
> + group_list, pe);
> + if (ret == -EINVAL)
>   continue;
> - if (events->len > 0)
> - strbuf_addf(events, ",");
> -
> - if (metricgroup__has_constraint(pe))
> - metricgroup__add_metric_non_group(events, ids, 
> idnum);
> - else
> - metricgroup__add_metric_weak_group(events, ids, 
> idnum);
> -
> - eg = malloc(sizeof(struct egroup));
> - if (!eg) {
> - ret = -ENOMEM;
> - break;
> - }
> - eg->ids = ids;
> - eg->idnum = idnum;
> - eg->metric_name = pe->metric_name;
> - eg->metric_expr = pe->metric_expr;
> - eg->metric_unit = pe->unit;
> - list_add_tail(>nd, group_list);
> - ret = 0;
>   }
>   }
>   return ret;
> -- 
> 2.18.1
> 

Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/arch/powerpc/util/header.c 
> b/tools/perf/arch/powerpc/util/header.c
> index 3b4cdfc5efd6..dcc3c6ab2e67 100644
> --- a/tools/perf/arch/powerpc/util/header.c
> +++ b/tools/perf/arch/powerpc/util/header.c
> @@ -7,6 +7,8 @@
>  #include 
>  #include 
>  #include "header.h"
> +#include "metricgroup.h"
> +#include 
>  
>  #define mfspr(rn)   ({unsigned long rval; \
>asm volatile("mfspr %0," __stringify(rn) \
> @@ -16,6 +18,8 @@
>  #define PVR_VER(pvr)(((pvr) >>  16) & 0x) /* Version field */
>  #define PVR_REV(pvr)(((pvr) >>   0) & 0x) /* Revison field */
>  
> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/sockets"
> +
>  int
>  get_cpuid(char *buffer, size_t sz)
>  {
> @@ -44,3 +48,9 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>  
>   return bufp;
>  }
> +
> +int arch_get_runtimeparam(void)
> +{
> + int count;
> + return sysfs__read_int(SOCKETS_INFO_FILE_PATH, ) < 0 ? 1 : count;

is that SOCKETS_INFO_FILE_PATH define used later? if not please
put the path directly as an argument to sysfs__read_int

jirka

Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 0938ad166ece..7786829b048b 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -17,12 +17,13 @@ struct expr_parse_ctx {
>  
>  struct expr_scanner_ctx {
>   int start_token;
> + int expr__runtimeparam;

no need for expr__ prefix in here.. jsut runtime_param

> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 402af3e8d287..85ac6d913782 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -336,7 +336,7 @@ void perf_stat__collect_metric_expr(struct evlist 
> *evsel_list)
>   metric_events = counter->metric_events;
>   if (!metric_events) {
>   if (expr__find_other(counter->metric_expr, 
> counter->name,
> - _names, 
> _metric_names) < 0)
> + _names, 
> _metric_names, 1) < 0)
>   continue;
>  
>   metric_events = calloc(sizeof(struct evsel *),
> @@ -777,7 +777,15 @@ static void generic_metric(struct perf_stat_config 
> *config,
>   }
>  
>   if (!metric_events[i]) {
> - if (expr__parse(, , metric_expr) == 0) {
> + int param = 1;
> + if (strstr(metric_expr, "?")) {
> + char *tmp = strrchr(metric_name, '_');
> +
> + tmp++;
> + param = strtol(tmp, , 10);
> + }

so, if metric_expr contains '?' you go and search metric_name for '_'
and use the number after '_' ... ugh.. what's the logic? 

I understand you create as many metrics as the magic runtime param
tells you.. but what's the connection to this?

could you please outline in the changelog or comment the whole scheme
of how this all should work? like step by step on some simple example?

thanks,
jirka

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 9377538f4097..d17664e628db 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,7 @@ struct parse_ctx {
>   struct parse_id ids[MAX_PARSE_ID];
>  };
>  
> +int expr__runtimeparam;
>  void expr__ctx_init(struct parse_ctx *ctx);
>  void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>  int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>   *dst++ = '/';
>   else if (*str == '\\')
>   *dst++ = *++str;
> +else if (*str == '?') {
> +
> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> + char * paramval = (char *)malloc(size);

can't we agree that any reasonable number in here
wouldn't cross 20 bytes in string or so and use
buffer for that instead of that malloc exercise?

thanks,
jirka

> + int i = 0;
> +
> + if(!paramval)
> + *dst++ = '0';
> + else {
> + sprintf(paramval, "%d", expr__runtimeparam);
> + while(i < size)
> + *dst++ = paramval[i++];
> + free(paramval);
> + }
> + }

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> + int i, count;
> + int ret = -EINVAL;
> +
> + count = arch_get_runtimeparam();
> +
> + /* This loop is added to create multiple
> +  * events depend on count value and add
> +  * those events to group_list.
> +  */
> +
> + for (i = 0; i < count; i++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + char value[PATH_MAX];
> +
> + expr__runtimeparam = i;
> +
> + if (expr__find_other(pe->metric_expr,
> + NULL, , ) < 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(struct egroup));
> + if (!eg) {
> + ret = -ENOMEM;
> + return ret;
> + }
> + sprintf(value, "%s%c%d", pe->metric_name, '_', i);
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = strdup(value);
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + if (ret != 0)
> + break;

the inside loop is essentialy what you factor out to
metricgroup__add_metric_param right? please nove
addition of metricgroup__add_metric_param function
into separate patch

jirka


> + }
> + return ret;
> +}
> +static int metricgroup__add_metric_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr,
> +  NULL, , ) < 0)
> + return ret;
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(struct egroup));
> + if (!eg)
> + ret = -ENOMEM;
> +
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + return ret;
> +}

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Tue, Mar 10, 2020 at 03:34:55PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > diff --git a/tools/perf/arch/powerpc/util/header.c 
> > b/tools/perf/arch/powerpc/util/header.c
> > index 3b4cdfc5efd6..036f6b2ce202 100644
> > --- a/tools/perf/arch/powerpc/util/header.c
> > +++ b/tools/perf/arch/powerpc/util/header.c
> > @@ -7,6 +7,11 @@
> >  #include 
> >  #include 
> >  #include "header.h"
> > +#include "metricgroup.h"
> > +#include "evlist.h"
> > +#include 
> > +#include "pmu.h"
> > +#include 
> >  
> >  #define mfspr(rn)   ({unsigned long rval; \
> >  asm volatile("mfspr %0," __stringify(rn) \
> > @@ -16,6 +21,8 @@
> >  #define PVR_VER(pvr)(((pvr) >>  16) & 0x) /* Version field */
> >  #define PVR_REV(pvr)(((pvr) >>   0) & 0x) /* Revison field */
> >  
> > +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> > +
> >  int
> >  get_cpuid(char *buffer, size_t sz)
> >  {
> > @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> >  
> > return bufp;
> >  }
> > +
> > +int arch_get_runtimeparam(void)
> > +{
> > +   int count;
> > +   char path[PATH_MAX];
> > +   char filename[] = "sockets";
> > +
> > +   snprintf(path, PATH_MAX,
> > +SOCKETS_INFO_FILE_PATH "%s", filename);

also, what's the point of using snprintf in here?

jirka

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>   *dst++ = '/';
>   else if (*str == '\\')
>   *dst++ = *++str;
> +else if (*str == '?') {
> +

extra line ^^^

jirka

> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> + char * paramval = (char *)malloc(size);
> + int i = 0;
> +
> + if(!paramval)

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..11eeeb929b91 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event 
> *pe)
>   return false;
>  }
>  
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> + int i, count;
> + int ret = -EINVAL;
> +
> + count = arch_get_runtimeparam();
> +
> + /* This loop is added to create multiple
> +  * events depend on count value and add
> +  * those events to group_list.
> +  */
> +
> + for (i = 0; i < count; i++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + char value[PATH_MAX];
> +
> + expr__runtimeparam = i;

so the expr__runtimeparam is always set before we call the
expr parsing function - wither expr__find_other or expr__parse,

and it's used inside the normalize flexer function, which has
access to the passed context.. so I don't see a reason why
expr__runtimeparam couldn't be added in struct parse_ctx
and used from there..

while in this, perhaps we should rename parse_ctx to expr_ctx,
to keep the namespace straight (in separate patch)

thanks,
jirka

Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Jiri Olsa]

On Tue, Mar 10, 2020 at 03:18:36PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 09, 2020 at 10:35:06AM +0100, Jiri Olsa escreveu:
> > On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> > > First patch of the patchset fix inconsistent results we are getting when
> > > we run multiple 24x7 events.
> > > 
> > > Patchset adds json file metric support for the hv_24x7 socket/chip level
> > > events. "hv_24x7" pmu interface events needs system dependent parameter
> > > like socket/chip/core. For example, hv_24x7 chip level events needs
> > > specific chip-id to which the data is requested should be added as part
> > > of pmu events.
> > > 
> > > So to enable JSON file support to "hv_24x7" interface, patchset expose
> > > total number of sockets and chips per-socket details in sysfs
> > > files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
> > > 
> > > To get sockets and number of chips per sockets, patchset adds a rtas call
> > > with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> > > handles partition migration case to re-init these system depended
> > > parameters by adding proper calls in post_mobility_fixup() (mobility.c).
> > > 
> > > Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> > > the "?" character in the metric expression to proper value and hv_24x7
> > > json metric file for different Socket/chip resources.
> > > 
> > > Patch set also enable Hz/hz prinitg for --metric-only option to print
> > > metric data for bus frequency.
> > > 
> > > Applied and tested all these patches cleanly on top of jiri's flex changes
> > > with the changes done by Kan Liang for "Support metric group constraint"
> > > patchset and made required changes.
> > > 
> > > Changelog:
> > > v3 -> v4
> > > - Made changes suggested by jiri.
> > 
> > could you please mention them next time? ;-)
> > 
> > > - Apply these patch on top of Kan liang changes.
> > 
> > Arnaldo, could you please pull the expr flex changes and Kan's
> > metric group constraint changes? it's both prereq of this patchset
> 
> Both are now in my perf/core branch, will go upstream soon, should I go
> and pickup the perf tooling bits in this patchkit?

Kajol mentioned there are still some issues with the
global variable, I plan to check on it this week

thanks,
jirka

Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> First patch of the patchset fix inconsistent results we are getting when
> we run multiple 24x7 events.
> 
> Patchset adds json file metric support for the hv_24x7 socket/chip level
> events. "hv_24x7" pmu interface events needs system dependent parameter
> like socket/chip/core. For example, hv_24x7 chip level events needs
> specific chip-id to which the data is requested should be added as part
> of pmu events.
> 
> So to enable JSON file support to "hv_24x7" interface, patchset expose
> total number of sockets and chips per-socket details in sysfs
> files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
> 
> To get sockets and number of chips per sockets, patchset adds a rtas call
> with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> handles partition migration case to re-init these system depended
> parameters by adding proper calls in post_mobility_fixup() (mobility.c).
> 
> Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> the "?" character in the metric expression to proper value and hv_24x7
> json metric file for different Socket/chip resources.
> 
> Patch set also enable Hz/hz prinitg for --metric-only option to print
> metric data for bus frequency.
> 
> Applied and tested all these patches cleanly on top of jiri's flex changes
> with the changes done by Kan Liang for "Support metric group constraint"
> patchset and made required changes.
> 
> Changelog:
> v3 -> v4
> - Made changes suggested by jiri.

could you please mention them next time? ;-)

> - Apply these patch on top of Kan liang changes.

Arnaldo, could you please pull the expr flex changes and Kan's
metric group constraint changes? it's both prereq of this patchset

thanks,
jirka

Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:

SNIP

>  #define PVR_VER(pvr)(((pvr) >>  16) & 0x) /* Version field */
>  #define PVR_REV(pvr)(((pvr) >>   0) & 0x) /* Revison field */
>  
> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> +
>  int
>  get_cpuid(char *buffer, size_t sz)
>  {
> @@ -44,3 +51,43 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>  
>   return bufp;
>  }
> +
> +int arch_get_runtimeparam(void)
> +{
> + int count = 0;
> + DIR *dir;
> + char path[PATH_MAX];
> + const char *sysfs = sysfs__mountpoint();
> + char filename[] = "sockets";
> + FILE *file;
> + char buf[16], *num;
> + int data;
> +
> + if (!sysfs)
> + goto out;
> +
> + snprintf(path, PATH_MAX,
> +  "%s" SOCKETS_INFO_FILE_PATH, sysfs);
> + dir = opendir(path);
> +
> + if (!dir)
> + goto out;
> +
> + strcat(path, filename);
> + file = fopen(path, "r");
> +
> + if (!file)
> + goto out;
> +
> + data = fread(buf, 1, sizeof(buf), file);
> +
> + if (data == 0)
> + goto out;
> +
> + count = strtol(buf, , 10);
> +out:
> + if (!count)
> + count = 1;
> +
> + return count;

we have sysfs__read_ull for this

jirka

Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..f629828cc0de 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -399,6 +399,11 @@ void metricgroup__print(bool metrics, bool metricgroups, 
> char *filter,
>   strlist__delete(metriclist);
>  }
>  
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  struct list_head *group_list)
>  {
> @@ -419,52 +424,77 @@ static int metricgroup__add_metric(const char *metric, 
> struct strbuf *events,
>   continue;
>   if (match_metric(pe->metric_group, metric) ||
>   match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
> - bool no_group = false;
> + int k, count;

two things in here.. there's already ack-ed patchset from Kan Liang:
  Support metric group constraint
>[PATCH V2 2/5] perf metricgroup: Factor out 
metricgroup__add_metric_weak_group()

that's changing this place, so you might want to synchronize with that


> +
> + if (strstr(pe->metric_expr, "?"))
> + count = arch_get_runtimeparam();
> + else
> + count = 1;
> +
> + /* This loop is added to create multiple
> +  * events depend on count value and add
> +  * those events to group_list.
> +  */
> + for (k = 0; k < count; k++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + bool no_group = false;
> + char value[PATH_MAX];
> +
> + pr_debug("metric expr %s for %s\n",
> +  pe->metric_expr, pe->metric_name);
> + expr__runtimeparam = k;

the other thing is that I don't really follow what's going on in here

you're setting expr__runtimeparam to the loop index,
which you get from some arch related file

we should do this in arch-specific way.. I think that Kan's change is
already moving some bits into separate function and that should make
all this more readable, but perhaps we might need more, so all the
'repeating' code will be in a function

please either separate this to arch code, or make it understandable
for people from other archs ;-)

jirka

> + if (expr__find_other(pe->metric_expr, NULL,
> +  , ) < 0)
> + continue;
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> + for (j = 0; j < idnum; j++) {
> + pr_debug("found event %s\n", ids[j]);
> + /*
> +  * Duration time maps to a software
> +  * event and can make groups not count.
> +  * Always use it outside a group.
> +  */
> + if (!strcmp(ids[j], "duration_time")) {
> + if (j > 0)
> + strbuf_addf(events,
> + "}:W,");
> + strbuf_addf(events,
> + "duration_time");
> + no_group = true;
> + continue;
> + }
> + strbuf_addf(events, "%s%s",
> + j == 0 || no_group ? "{" :
> + ",", ids[j]);
> + no_group = false;
> + }
> + if (!no_group)
> + strbuf_addf(events, "}:W");
>  
> - pr_debug("metric expr %s for %s\n", pe->metric_expr, 
> pe->metric_name);
> + eg = malloc(sizeof(struct egroup));
> + if (!eg) {
> + ret = -ENOMEM;
> + break;
> + }
> + eg->ids = ids;
> +   

Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:

SNIP

> + *dst++ = paramval[i++];
> + free(paramval);
> + }
> + }
>   else
>   *dst++ = *str;
>   str++;
> @@ -72,8 +86,8 @@ number  [0-9]+
>  
>  sch  [-,=]
>  spec \\{sch}
> -sym  [0-9a-zA-Z_\.:@]+
> -symbol   {spec}*{sym}*{spec}*{sym}*
> +sym[0-9a-zA-Z_\.:@?]+
> +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym}
>  
>  %%
>   {
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 4720cbe79357..0f3ef0f37bf4 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -38,6 +38,8 @@
>  %type  expr if_expr
>  
>  %{
> +int expr__runtimeparam;

we don't like global variables.. could this be part of the
contaxt struct?

jirka

Re: [PATCH v2] perf mem/c2c: Fix perf_mem_events to support powerpc

[Jiri Olsa]

On Tue, Jan 29, 2019 at 06:54:12PM +0530, Ravi Bangoria wrote:
> Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
> event and, perf_mem_events by default includes ldlat=30 which is
> causing failure on powerpc. Refactor code to support perf mem/c2c on
> powerpc.
> 
> This patch depends on kernel side changes done my Madhavan:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
> 
> Signed-off-by: Ravi Bangoria 
> ---

Acked-by: Jiri Olsa 

thanks,
jirka

>  tools/perf/Documentation/perf-c2c.txt | 16 
>  tools/perf/Documentation/perf-mem.txt |  2 +-
>  tools/perf/arch/powerpc/util/Build|  1 +
>  tools/perf/arch/powerpc/util/mem-events.c | 11 +++
>  tools/perf/util/mem-events.c  |  2 +-
>  5 files changed, 26 insertions(+), 6 deletions(-)
>  create mode 100644 tools/perf/arch/powerpc/util/mem-events.c
> 
> diff --git a/tools/perf/Documentation/perf-c2c.txt 
> b/tools/perf/Documentation/perf-c2c.txt
> index 095aebd..e6150f2 100644
> --- a/tools/perf/Documentation/perf-c2c.txt
> +++ b/tools/perf/Documentation/perf-c2c.txt
> @@ -19,8 +19,11 @@ C2C stands for Cache To Cache.
>  The perf c2c tool provides means for Shared Data C2C/HITM analysis. It allows
>  you to track down the cacheline contentions.
>  
> -The tool is based on x86's load latency and precise store facility events
> -provided by Intel CPUs. These events provide:
> +On x86, the tool is based on load latency and precise store facility events
> +provided by Intel CPUs. On PowerPC, the tool uses random instruction sampling
> +with thresholding feature.
> +
> +These events provide:
>- memory address of the access
>- type of the access (load and store details)
>- latency (in cycles) of the load access
> @@ -46,7 +49,7 @@ RECORD OPTIONS
>  
>  -l::
>  --ldlat::
> - Configure mem-loads latency.
> + Configure mem-loads latency. (x86 only)
>  
>  -k::
>  --all-kernel::
> @@ -119,11 +122,16 @@ Following perf record options are configured by default:
>-W,-d,--phys-data,--sample-cpu
>  
>  Unless specified otherwise with '-e' option, following events are monitored 
> by
> -default:
> +default on x86:
>  
>cpu/mem-loads,ldlat=30/P
>cpu/mem-stores/P
>  
> +and following on PowerPC:
> +
> +  cpu/mem-loads/
> +  cpu/mem-stores/
> +
>  User can pass any 'perf record' option behind '--' mark, like (to enable
>  callchains and system wide monitoring):
>  
> diff --git a/tools/perf/Documentation/perf-mem.txt 
> b/tools/perf/Documentation/perf-mem.txt
> index f8d2167..199ea0f 100644
> --- a/tools/perf/Documentation/perf-mem.txt
> +++ b/tools/perf/Documentation/perf-mem.txt
> @@ -82,7 +82,7 @@ RECORD OPTIONS
>   Be more verbose (show counter open errors, etc)
>  
>  --ldlat ::
> - Specify desired latency for loads event.
> + Specify desired latency for loads event. (x86 only)
>  
>  In addition, for report all perf report options are valid, and for record
>  all perf record options.
> diff --git a/tools/perf/arch/powerpc/util/Build 
> b/tools/perf/arch/powerpc/util/Build
> index 2e659531..ba98bd0 100644
> --- a/tools/perf/arch/powerpc/util/Build
> +++ b/tools/perf/arch/powerpc/util/Build
> @@ -2,6 +2,7 @@ libperf-y += header.o
>  libperf-y += sym-handling.o
>  libperf-y += kvm-stat.o
>  libperf-y += perf_regs.o
> +libperf-y += mem-events.o
>  
>  libperf-$(CONFIG_DWARF) += dwarf-regs.o
>  libperf-$(CONFIG_DWARF) += skip-callchain-idx.o
> diff --git a/tools/perf/arch/powerpc/util/mem-events.c 
> b/tools/perf/arch/powerpc/util/mem-events.c
> new file mode 100644
> index 000..f1194fc
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/mem-events.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "mem-events.h"
> +
> +/* PowerPC does not support 'ldlat' parameter. */
> +char *perf_mem_events__name(int i)
> +{
> + if (i == PERF_MEM_EVENTS__LOAD)
> + return (char *) "cpu/mem-loads/";
> +
> + return (char *) "cpu/mem-stores/";
> +}
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 93f74d8..42c3e5a 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -28,7 +28,7 @@ struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] 
> = {
>  static char mem_loads_name[100];
>  static bool mem_loads_name__init;
>  
> -char *perf_mem_events__name(int i)
> +char * __weak perf_mem_events__name(int i)
>  {
>   if (i == PERF_MEM_EVENTS__LOAD) {
>   if (!mem_loads_name__init) {
> -- 
> 1.8.3.1
> 

Re: [PATCH] perf mem/c2c: Fix perf_mem_events to support powerpc

[Jiri Olsa]

On Mon, Jan 14, 2019 at 09:44:02AM +0530, Ravi Bangoria wrote:

SNIP

> diff --git a/tools/perf/arch/x86/util/mem-events.c 
> b/tools/perf/arch/x86/util/mem-events.c
> new file mode 100644
> index 000..5b4dcfe
> --- /dev/null
> +++ b/tools/perf/arch/x86/util/mem-events.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "mem-events.h"
> +
> +struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> + PERF_MEM_EVENT("ldlat-loads", "cpu/mem-loads,ldlat=%u/P", "mem-loads"),
> + PERF_MEM_EVENT("ldlat-stores", "cpu/mem-stores/P", "mem-stores"),
> +};
> +
> +static char mem_loads_name[100];
> +static bool mem_loads_name__init;
> +
> +char *perf_mem_events__name(int i)
> +{
> + if (i == PERF_MEM_EVENTS__LOAD) {
> + if (!mem_loads_name__init) {
> + mem_loads_name__init = true;
> + scnprintf(mem_loads_name, sizeof(mem_loads_name),
> +   perf_mem_events[i].name,
> +   perf_mem_events__loads_ldlat);
> + }
> + return mem_loads_name;
> + }
> +
> + return (char *)perf_mem_events[i].name;
> +}
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 93f74d8..1ffefd3 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -15,31 +15,13 @@
>  
>  unsigned int perf_mem_events__loads_ldlat = 30;
>  
> -#define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s }
> -
> -struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> - E("ldlat-loads","cpu/mem-loads,ldlat=%u/P", "mem-loads"),
> - E("ldlat-stores",   "cpu/mem-stores/P", "mem-stores"),
> +struct perf_mem_event __weak perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> + PERF_MEM_EVENT("ldlat-loads", "cpu/mem-loads/P", "mem-loads"),
> + PERF_MEM_EVENT("ldlat-stores", "cpu/mem-stores/P", "mem-stores"),
>  };

I dont think perf_mem_events array needs to be overloaded as well,
the perf_mem_events__name function should be enough no?

thanks,
jirka

Re: [GIT PULL] Please pull JSON files for Power9 PMU events

[Jiri Olsa]

On Thu, Jul 13, 2017 at 01:43:41PM +1000, Michael Ellerman wrote:
> Sukadev Bhattiprolu  writes:
> ...
> >
> >  tools/perf/pmu-events/arch/powerpc/mapfile.csv |   3 +
> >  .../perf/pmu-events/arch/powerpc/power9/cache.json | 176 +
> >  .../arch/powerpc/power9/floating-point.json|  44 ++
> >  .../pmu-events/arch/powerpc/power9/frontend.json   | 446 +++
> >  .../pmu-events/arch/powerpc/power9/marked.json | 782 
> > +++
> >  .../pmu-events/arch/powerpc/power9/memory.json | 158 
> 
> In the map file we have "power9.json", but the files are power9/x.json.
> How does that work?

hum, we take just the base name from that string and use it
as a map to the directory/struct with events.. so no harm done

I guess it's the leftover from when we switched from single file
to per topic event files.. x86 arch seems to have it right

I'll send the fix

thanks,
jirka

Re: [PATCH v6 0/7] perf report: Show branch type

[Jiri Olsa]

On Thu, Apr 20, 2017 at 08:07:48PM +0800, Jin Yao wrote:
> v6:
>Update according to the review comments from
>Jiri Olsa <jo...@redhat.com>. Major modifications are: 
> 
>1. Move that multiline conditional code inside {} brackets.
> 
>2. Move branch_type_stat_display() from builtin-report.c to
>   branch.c. Move branch_type_str() from callchain.c to
>   branch.c.
> 
>3. Keep the original branch info display order, that is:
>   predicted, abort, cycles, iterations

Peter,
are you ok with the kernel side of this?

thanks,
jirka

> 
> v5:
> ---
>Mainly the v5 patch series are updated according to
>comments from Jiri Olsa <jo...@redhat.com>.
> 
>The kernel part doesn't have functional change. It just
>solve the merge issue.
> 
>In userspace, the functions of branch type counting and
>branch type name resolving are moved to the new files: 
>util/branch.c, util/branch.h.
> 
>And refactor the branch info printing code for better
>maintenance.
> 
> Not changed (or just fix merge issue):
>   perf/core: Define the common branch type classification
>   perf/x86/intel: Record branch type
>   perf record: Create a new option save_type in --branch-filter
> 
> New patches:
>   perf report: Refactor the branch info printing code
>   perf util: Create branch.c/.h for common branch functions
> 
> Changed:
>   perf report: Show branch type statistics for stdio mode
>   perf report: Show branch type in callchain entry
> 
> v4:
> ---
> 1. Describe the major changes in patch description.
>Thanks for Peter Zijlstra's reminding. 
> 
> 2. Initialize branch type to 0 in intel_pmu_lbr_read_32 and
>intel_pmu_lbr_read_64. Remove the invalid else code in
>intel_pmu_lbr_filter. 
> 
> v3:
> ---
> 1. Move the JCC forward/backward and cross page computing from
>kernel to userspace.
> 
> 2. Use lookup table to replace original switch/case processing.
> 
> Changed:
>   perf/core: Define the common branch type classification
>   perf/x86/intel: Record branch type
>   perf report: Show branch type statistics for stdio mode
>   perf report: Show branch type in callchain entry
> 
> Not changed:
>   perf record: Create a new option save_type in --branch-filter
> 
> v2:
> ---
> 1. Use 4 bits in perf_branch_entry to record branch type.
> 
> 2. Pull out some common branch types from FAR_BRANCH. Now the branch
>types defined in perf_event.h:
> 
> Jin Yao (7):
>   perf/core: Define the common branch type classification
>   perf/x86/intel: Record branch type
>   perf record: Create a new option save_type in --branch-filter
>   perf report: Refactor the branch info printing code
>   perf util: Create branch.c/.h for common branch functions
>   perf report: Show branch type statistics for stdio mode
>   perf report: Show branch type in callchain entry
> 
>  arch/x86/events/intel/lbr.c  |  53 +-
>  include/uapi/linux/perf_event.h  |  29 +-
>  tools/include/uapi/linux/perf_event.h|  29 +-
>  tools/perf/Documentation/perf-record.txt |   1 +
>  tools/perf/builtin-report.c  |  25 +
>  tools/perf/util/Build|   1 +
>  tools/perf/util/branch.c | 168 
> +++
>  tools/perf/util/branch.h |  25 +
>  tools/perf/util/callchain.c  | 140 ++
>  tools/perf/util/callchain.h  |   5 +-
>  tools/perf/util/event.h  |   3 +-
>  tools/perf/util/hist.c   |   5 +-
>  tools/perf/util/machine.c|  26 +++--
>  tools/perf/util/parse-branch-options.c   |   1 +
>  14 files changed, 427 insertions(+), 84 deletions(-)
>  create mode 100644 tools/perf/util/branch.c
>  create mode 100644 tools/perf/util/branch.h
> 
> -- 
> 2.7.4
> 

Re: [PATCH v2] perf: libdw support for powerpc

[Jiri Olsa]

On Thu, Jun 01, 2017 at 12:24:41PM +0200, Paolo Bonzini wrote:
> Porting PPC to libdw only needs an architecture-specific hook to move
> the register state from perf to libdw.
> 
> The ARM and x86 architectures already use libdw, and it is useful to
> have as much common code for the unwinder as possible.  Mark Wielaard
> has contributed a frame-based unwinder to libdw, so that unwinding works
> even for binaries that do not have CFI information.  In addition,
> libunwind is always preferred to libdw by the build machinery so this
> cannot introduce regressions on machines that have both libunwind and
> libdw installed.
> 
> Cc: a...@kernel.org
> Cc: Naveen N. Rao <naveen.n@linux.vnet.ibm.com>
> Cc: Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>

looks good and got possitive feedback from our QE testing this

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Tue, May 09, 2017 at 07:57:11PM +0800, Jin, Yao wrote:

SNIP

> > > > > +
> > > > > + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> > > > > + mask = ~(~0 << 1);
> > > > is that a fancy way to get 1 into the mask? what do I miss?
> > you did not comment on this one
> 
> Sorry, I misunderstood that this comment and the next comment had the same
> meaning.
> 
> In the previous version, I used the switch/case to convert from X86_BR to
> PERF_BR. I got a comment from community that it'd better use a lookup table
> for conversion.
> 
> Since each bit in type represents a X86_BR type so I use a mask (0x1) to
> filter the bit. Yes, it looks I can also directly set 0x1 to mask.
> 
> I write the code "mask = ~(~0 << 1)" according to my coding habits. If you
> think I should change the code to "mask = 0x1", that's OK  :)

im ok with that.. was just wondering for the reason
I guess compiler will make it single constant assignment anyway

> 
> > > > > +
> > > > > + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> > > > > + if (type & mask)
> > > > > + return branch_map[i];
> > > > I wonder some bit search would be faster in here, but maybe not big deal
> > > > 
> > > > jirka
> > > I just think the branch_map[] doesn't contain many entries (16 entries
> > > here), so maybe checking 1 bit one time should be acceptable. I just want 
> > > to
> > > keep the code simple.
> > > 
> > > But if the number of entries is more (e.g. 64), maybe it'd better check 2 
> > > or
> > > 4 bits one time.
> > ook
> > 
> > jirka
> Sorry, what's the meaning of ook? Does it mean "OK"?

just means ok ;-)

thanks,
jirka

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Mon, Apr 24, 2017 at 08:47:14AM +0800, Jin, Yao wrote:
> 
> 
> On 4/23/2017 9:55 PM, Jiri Olsa wrote:
> > On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > +#define X86_BR_TYPE_MAP_MAX  16
> > > +
> > > +static int
> > > +common_branch_type(int type)
> > > +{
> > > + int i, mask;
> > > + const int branch_map[X86_BR_TYPE_MAP_MAX] = {
> > > + PERF_BR_CALL,   /* X86_BR_CALL */
> > > + PERF_BR_RET,/* X86_BR_RET */
> > > + PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
> > > + PERF_BR_SYSRET, /* X86_BR_SYSRET */
> > > + PERF_BR_INT,/* X86_BR_INT */
> > > + PERF_BR_IRET,   /* X86_BR_IRET */
> > > + PERF_BR_JCC,/* X86_BR_JCC */
> > > + PERF_BR_JMP,/* X86_BR_JMP */
> > > + PERF_BR_IRQ,/* X86_BR_IRQ */
> > > + PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
> > > + PERF_BR_NONE,   /* X86_BR_ABORT */
> > > + PERF_BR_NONE,   /* X86_BR_IN_TX */
> > > + PERF_BR_NONE,   /* X86_BR_NO_TX */
> > > + PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
> > > + PERF_BR_NONE,   /* X86_BR_CALL_STACK */
> > > + PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
> > > + };
> > > +
> > > + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> > > + mask = ~(~0 << 1);
> > is that a fancy way to get 1 into the mask? what do I miss?

you did not comment on this one

> > 
> > > +
> > > + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> > > + if (type & mask)
> > > + return branch_map[i];
> > I wonder some bit search would be faster in here, but maybe not big deal
> > 
> > jirka
> 
> I just think the branch_map[] doesn't contain many entries (16 entries
> here), so maybe checking 1 bit one time should be acceptable. I just want to
> keep the code simple.
> 
> But if the number of entries is more (e.g. 64), maybe it'd better check 2 or
> 4 bits one time.

ook

jirka

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:

SNIP

>  
> +#define X86_BR_TYPE_MAP_MAX  16
> +
> +static int
> +common_branch_type(int type)
> +{
> + int i, mask;
> + const int branch_map[X86_BR_TYPE_MAP_MAX] = {
> + PERF_BR_CALL,   /* X86_BR_CALL */
> + PERF_BR_RET,/* X86_BR_RET */
> + PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
> + PERF_BR_SYSRET, /* X86_BR_SYSRET */
> + PERF_BR_INT,/* X86_BR_INT */
> + PERF_BR_IRET,   /* X86_BR_IRET */
> + PERF_BR_JCC,/* X86_BR_JCC */
> + PERF_BR_JMP,/* X86_BR_JMP */
> + PERF_BR_IRQ,/* X86_BR_IRQ */
> + PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
> + PERF_BR_NONE,   /* X86_BR_ABORT */
> + PERF_BR_NONE,   /* X86_BR_IN_TX */
> + PERF_BR_NONE,   /* X86_BR_NO_TX */
> + PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
> + PERF_BR_NONE,   /* X86_BR_CALL_STACK */
> + PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
> + };
> +
> + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> + mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?

> +
> + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> + if (type & mask)
> + return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka

> +
> + type >>= 1;
> + }
> +
> + return PERF_BR_NONE;
> +}
> +
>  /*

Re: [PATCH v6 0/7] perf report: Show branch type

[Jiri Olsa]

On Thu, Apr 20, 2017 at 08:07:48PM +0800, Jin Yao wrote:
> v6:
>Update according to the review comments from
>Jiri Olsa <jo...@redhat.com>. Major modifications are: 
> 
>1. Move that multiline conditional code inside {} brackets.
> 
>2. Move branch_type_stat_display() from builtin-report.c to
>   branch.c. Move branch_type_str() from callchain.c to
>   branch.c.
> 
>3. Keep the original branch info display order, that is:
>   predicted, abort, cycles, iterations

for the tools part

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

Re: [PATCH v5 6/7] perf report: Show branch type statistics for stdio mode

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:13PM +0800, Jin Yao wrote:

SNIP

> +static void branch_type_stat_display(FILE *fp, struct branch_type_stat *stat)
> +{
> + u64 total = 0;
> + int i;
> +
> + for (i = 0; i < PERF_BR_MAX; i++)
> + total += stat->counts[i];
> +
> + if (total == 0)
> + return;
> +
> + fprintf(fp, "\n#");
> + fprintf(fp, "\n# Branch Statistics:");
> + fprintf(fp, "\n#");
> +
> + if (stat->jcc_fwd > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "JCC forward",
> + 100.0 * (double)stat->jcc_fwd / (double)total);
> +
> + if (stat->jcc_bwd > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "JCC backward",
> + 100.0 * (double)stat->jcc_bwd / (double)total);
> +
> + if (stat->cross_4k > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "CROSS_4K",
> + 100.0 * (double)stat->cross_4k / (double)total);
> +
> + if (stat->cross_2m > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "CROSS_2M",
> + 100.0 * (double)stat->cross_2m / (double)total);
> +
> + for (i = 0; i < PERF_BR_MAX; i++) {
> + if (stat->counts[i] > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + branch_type_name(i),
> + 100.0 *
> + (double)stat->counts[i] / (double)total);
> + }
> +}

could you please also move this one to that new branch.c file

thanks,
jirka

Re: [PATCH v5 7/7] perf report: Show branch type in callchain entry

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:14PM +0800, Jin Yao wrote:

SNIP

> +static int count_str_printf(int index, const char *str,
> + char *bf, int bfsize)
> +{
> + int printed;
> +
> + printed = scnprintf(bf, bfsize,
> + "%s%s",
> + (index) ? " " : " (", str);
> +
> + return printed;
> +}
> +
> +static int branch_type_str(struct branch_type_stat *stat,
> +char *bf, int bfsize)
> +{
> + int i, j = 0, printed = 0;
> + u64 total = 0;
> +
> + for (i = 0; i < PERF_BR_MAX; i++)
> + total += stat->counts[i];
> +
> + if (total == 0)
> + return 0;
> +
> + if (stat->jcc_fwd > 0)
> + printed += count_str_printf(j++, "JCC forward",
> + bf + printed, bfsize - printed);
> +
> + if (stat->jcc_bwd > 0)
> + printed += count_str_printf(j++, "JCC backward",
> + bf + printed, bfsize - printed);
> +
> + for (i = 0; i < PERF_BR_MAX; i++) {
> + if (i == PERF_BR_JCC)
> + continue;
> +
> + if (stat->counts[i] > 0)
> + printed += count_str_printf(j++, branch_type_name(i),
> + bf + printed, bfsize - printed);
> + }
> +
> + if (stat->cross_4k > 0)
> + printed += count_str_printf(j++, "CROSS_4K",
> + bf + printed, bfsize - printed);
> +
> + if (stat->cross_2m > 0)
> + printed += count_str_printf(j++, "CROSS_2M",
> + bf + printed, bfsize - printed);
> +
> + return printed;
> +}

could you please also move this one to that new branch.c file

thanks,
jirka

Re: [PATCH v5 4/7] perf report: Refactor the branch info printing code

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:11PM +0800, Jin Yao wrote:

SNIP

>  static int counts_str_build(char *bf, int bfsize,
>u64 branch_count, u64 predicted_count,
>u64 abort_count, u64 cycles_count,
>u64 iter_count, u64 samples_count)
>  {
> - double predicted_percent = 0.0;
> - const char *null_str = "";
> - char iter_str[32];
> - char cycle_str[32];
> - char *istr, *cstr;
>   u64 cycles;
> + int printed = 0, i = 0;
>  
>   if (branch_count == 0)
>   return scnprintf(bf, bfsize, " (calltrace)");
>  
>   cycles = cycles_count / branch_count;
> + if (cycles)
> + printed += count_pri64_printf(i++, "cycles",
> + cycles,
> + bf + printed, bfsize - printed);
>  
> - if (iter_count && samples_count) {
> - if (cycles > 0)
> - scnprintf(iter_str, sizeof(iter_str),
> -  " iterations:%" PRId64 "",
> -  iter_count / samples_count);
> - else
> - scnprintf(iter_str, sizeof(iter_str),
> -  "iterations:%" PRId64 "",
> -  iter_count / samples_count);
> - istr = iter_str;
> - } else
> - istr = (char *)null_str;
> -
> - if (cycles > 0) {
> - scnprintf(cycle_str, sizeof(cycle_str),
> -   "cycles:%" PRId64 "", cycles);
> - cstr = cycle_str;
> - } else
> - cstr = (char *)null_str;
> + if (iter_count && samples_count)
> + printed += count_pri64_printf(i++, "iterations",
> + iter_count / samples_count,
> + bf + printed, bfsize - printed);

please put the multiline condition code into {} brackets
for all of the cases in this function

thanks,
jirka

Re: [PATCH v5 6/7] perf report: Show branch type statistics for stdio mode

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:13PM +0800, Jin Yao wrote:

SNIP

> +static void branch_type_stat_display(FILE *fp, struct branch_type_stat *stat)
> +{
> + u64 total = 0;
> + int i;
> +
> + for (i = 0; i < PERF_BR_MAX; i++)
> + total += stat->counts[i];
> +
> + if (total == 0)
> + return;
> +
> + fprintf(fp, "\n#");
> + fprintf(fp, "\n# Branch Statistics:");
> + fprintf(fp, "\n#");
> +
> + if (stat->jcc_fwd > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "JCC forward",
> + 100.0 * (double)stat->jcc_fwd / (double)total);
> +
> + if (stat->jcc_bwd > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "JCC backward",
> + 100.0 * (double)stat->jcc_bwd / (double)total);
> +
> + if (stat->cross_4k > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "CROSS_4K",
> + 100.0 * (double)stat->cross_4k / (double)total);
> +
> + if (stat->cross_2m > 0)
> + fprintf(fp, "\n%12s: %5.1f%%",
> + "CROSS_2M",
> + 100.0 * (double)stat->cross_2m / (double)total);

{} brackets here as well

thanks,
jirka

Re: [PATCH v5 4/7] perf report: Refactor the branch info printing code

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:11PM +0800, Jin Yao wrote:

SNIP

> +
>  static int counts_str_build(char *bf, int bfsize,
>u64 branch_count, u64 predicted_count,
>u64 abort_count, u64 cycles_count,
>u64 iter_count, u64 samples_count)
>  {
> - double predicted_percent = 0.0;
> - const char *null_str = "";
> - char iter_str[32];
> - char cycle_str[32];
> - char *istr, *cstr;
>   u64 cycles;
> + int printed = 0, i = 0;

I like it, but it looks like the previous code displayed those
bits in another order.. I managed to catch this one:

  1337c1337
  <  --0.53%--menu_select menu.c:218 (iterations:6 predicted:0.0%)
  ---
  >  --0.53%--menu_select menu.c:218 (predicted:0.0% iterations:6)

I think we better keep the current order, which seems
to be the goal of the original code as well

this function is perfect candidate for automated test ;-)
(something like we did in tests/kmod-path.c)

thanks,
jirka

Re: [PATCH v5 7/7] perf report: Show branch type in callchain entry

[Jiri Olsa]

On Wed, Apr 19, 2017 at 11:48:14PM +0800, Jin Yao wrote:

SNIP

> +static int branch_type_str(struct branch_type_stat *stat,
> +char *bf, int bfsize)
> +{
> + int i, j = 0, printed = 0;
> + u64 total = 0;
> +
> + for (i = 0; i < PERF_BR_MAX; i++)
> + total += stat->counts[i];
> +
> + if (total == 0)
> + return 0;
> +
> + if (stat->jcc_fwd > 0)
> + printed += count_str_printf(j++, "JCC forward",
> + bf + printed, bfsize - printed);
> +
> + if (stat->jcc_bwd > 0)
> + printed += count_str_printf(j++, "JCC backward",
> + bf + printed, bfsize - printed);

please move that multiline conditional code inside {} brackets

thanks,
jirka

Re: [PATCH v4 4/5] perf report: Show branch type statistics for stdio mode

[Jiri Olsa]

On Wed, Apr 12, 2017 at 06:21:05AM +0800, Jin Yao wrote:

SNIP

> +static int hist_iter__branch_callback(struct hist_entry_iter *iter,
> +   struct addr_location *al __maybe_unused,
> +   bool single __maybe_unused,
> +   void *arg)
> +{
> + struct hist_entry *he = iter->he;
> + struct report *rep = arg;
> + struct branch_info *bi;
> +
> + if (sort__mode == SORT_MODE__BRANCH) {

is this check necessary? the hist_iter__branch_callback
was set based on this check

jirka

Re: [PATCH v4 4/5] perf report: Show branch type statistics for stdio mode

[Jiri Olsa]

On Wed, Apr 12, 2017 at 06:21:05AM +0800, Jin Yao wrote:

SNIP

> +const char *branch_type_name(int type)
> +{
> + const char *branch_names[PERF_BR_MAX] = {
> + "N/A",
> + "JCC",
> + "JMP",
> + "IND_JMP",
> + "CALL",
> + "IND_CALL",
> + "RET",
> + "SYSCALL",
> + "SYSRET",
> + "IRQ",
> + "INT",
> + "IRET",
> + "FAR_BRANCH",
> + };
> +
> + if ((type >= 0) && (type < PERF_BR_MAX))
> + return branch_names[type];
> +
> + return NULL;

looks like we should add util/branch.c with above functions
and merge it with util/parse-branch-options.c

we create new file even for less code ;-)

thanks,
jirka

Re: [PATCH v4 5/5] perf report: Show branch type in callchain entry

[Jiri Olsa]

On Wed, Apr 12, 2017 at 06:21:06AM +0800, Jin Yao wrote:

SNIP

>  static int counts_str_build(char *bf, int bfsize,
>u64 branch_count, u64 predicted_count,
>u64 abort_count, u64 cycles_count,
> -  u64 iter_count, u64 samples_count)
> +  u64 iter_count, u64 samples_count,
> +  struct branch_type_stat *brtype_stat)
>  {
> - double predicted_percent = 0.0;
> - const char *null_str = "";
> - char iter_str[32];
> - char cycle_str[32];
> - char *istr, *cstr;
>   u64 cycles;
> + int printed, i = 0;
>  
>   if (branch_count == 0)
>   return scnprintf(bf, bfsize, " (calltrace)");
>  
> + printed = branch_type_str(brtype_stat, bf, bfsize);
> + if (printed)
> + i++;
> +
>   cycles = cycles_count / branch_count;
> + if (cycles) {
> + if (i++)
> + printed += scnprintf(bf + printed, bfsize - printed,
> + " cycles:%" PRId64 "", cycles);
> + else
> + printed += scnprintf(bf + printed, bfsize - printed,
> + " (cycles:%" PRId64 "", cycles);
> + }
>  
>   if (iter_count && samples_count) {
> - if (cycles > 0)
> - scnprintf(iter_str, sizeof(iter_str),
> -  " iterations:%" PRId64 "",
> -  iter_count / samples_count);
> + if (i++)
> + printed += scnprintf(bf + printed, bfsize - printed,
> + " iterations:%" PRId64 "",
> + iter_count / samples_count);
>   else
> - scnprintf(iter_str, sizeof(iter_str),
> -  "iterations:%" PRId64 "",
> -  iter_count / samples_count);
> - istr = iter_str;

could you please put the change from using iter_str
to bf into separate patch before the actual branch
display change?

it's hard to see if anything is broken ;-)

thanks,
jirka

Re: [PATCH v4 5/5] perf report: Show branch type in callchain entry

[Jiri Olsa]

On Wed, Apr 12, 2017 at 06:21:06AM +0800, Jin Yao wrote:

SNIP

> +static int branch_type_str(struct branch_type_stat *stat,
> +char *bf, int bfsize)
> +{
> + int i, j = 0, printed = 0;
> + u64 total = 0;
> +
> + for (i = 0; i < PERF_BR_MAX; i++)
> + total += stat->counts[i];
> +
> + if (total == 0)
> + return 0;
> +
> + printed += scnprintf(bf + printed, bfsize - printed, " (");
> +
> + if (stat->jcc_fwd > 0) {
> + j++;
> + printed += scnprintf(bf + printed, bfsize - printed,
> +  "JCC forward");
> + }
> +
> + if (stat->jcc_bwd > 0) {
> + if (j++)
> + printed += scnprintf(bf + printed, bfsize - printed,
> +  " JCC backward");
> + else
> + printed += scnprintf(bf + printed, bfsize - printed,
> +  "JCC backward");
> + }
> +
> + if (stat->cross_4k > 0) {
> + if (j++)
> + printed += scnprintf(bf + printed, bfsize - printed,
> +  " CROSS_4K");
> + else
> + printed += scnprintf(bf + printed, bfsize - printed,
> +  "CROSS_4K");
> + }

could that 2 legs if be shortened to just one scnprintf like (untested):

   printed += scnprintf(bf + printed, bfsize - printed, "%s%s", j++ ? " " : "", 
"CROSS_4K");

I'd also probably use some kind of macro or function
with all that similar code, but I dont insist ;-)

thanks,
jirka

Re: [PATCH v4 0/5] perf report: Show branch type

[Jiri Olsa]

On Thu, Apr 13, 2017 at 11:25:39AM +0800, Jin, Yao wrote:

SNIP

> > 
> > Now it works without my patch and it runs with latest perf/core branch.
> > So it looks like a regression issue.
> > 
> > Thanks
> > Jin Yao
> > 
> > 
> 
> I have tested, the regression issue is happened after this commit:
> 
> bdd97ca perf tools: Refactor the code to strip command name with {l,r}trim()
> 
> CC to the author for double checking.

cool, thanks

jirka

Re: [PATCH v4 0/5] perf report: Show branch type

[Jiri Olsa]

On Wed, Apr 12, 2017 at 11:42:44PM +0800, Jin, Yao wrote:
> 
> 
> On 4/12/2017 10:26 PM, Jiri Olsa wrote:
> > On Wed, Apr 12, 2017 at 08:25:34PM +0800, Jin, Yao wrote:
> > 
> > SNIP
> > 
> > > > # Overhead  Command  Source Shared Object  Source Symbol
> > > > Target SymbolBasic Block Cycles
> > > > #   ...    
> > > > ...  
> > > > ...  ..
> > > > #
> > > >8.30%  perf
> > > > Um  [kernel.vmlinux]  [k] __intel_pmu_enable_all.constprop.17  [k] 
> > > > native_write_msr -
> > > >7.91%  perf
> > > > Um  [kernel.vmlinux]  [k] intel_pmu_lbr_enable_all [k] 
> > > > __intel_pmu_enable_all.constprop.17  -
> > > >7.91%  perf
> > > > Um  [kernel.vmlinux]  [k] native_write_msr [k] 
> > > > intel_pmu_lbr_enable_all -
> > > >6.32%  kill libc-2.24.so  [.] _dl_addr   
> > > >   [.] _dl_addr -
> > > >5.93%  perf
> > > > Um  [kernel.vmlinux]  [k] perf_iterate_ctx [k] 
> > > > perf_iterate_ctx -
> > > >2.77%  kill libc-2.24.so  [.] malloc 
> > > >   [.] malloc   -
> > > >1.98%  kill libc-2.24.so  [.] _int_malloc
> > > >   [.] _int_malloc  -
> > > >1.58%  kill [kernel.vmlinux]  [k] __rb_insert_augmented  
> > > >   [k] __rb_insert_augmented-
> > > >1.58%  perf
> > > > Um  [kernel.vmlinux]  [k] perf_event_exec  [k] 
> > > > perf_event_exec  -
> > > >1.19%  kill [kernel.vmlinux]  [k] 
> > > > anon_vma_interval_tree_insert[k] anon_vma_interval_tree_insert  
> > > >   -
> > > >1.19%  kill [kernel.vmlinux]  [k] free_pgd_range 
> > > >   [k] free_pgd_range   -
> > > >1.19%  kill [kernel.vmlinux]  [k] n_tty_write
> > > >   [k] n_tty_write  -
> > > >1.19%  perf
> > > > Um  [kernel.vmlinux]  [k] native_sched_clock   [k] 
> > > > sched_clock  -
> > > > ...
> > > > SNIP
> > > > 
> > > > 
> > > > jirka
> > > Hi,
> > > 
> > > Thanks so much for trying this patch.
> > > 
> > > The branch statistics is printed at the end of perf report --stdio.
> > yep, but for some reason with your changes the head report
> > got changed as well, I haven't checked the details yet..
> > 
> > jirka
> 
> The kill returns immediately with no parameter error. Could you try an
> application which can run for a while?
> 
> For example:
> perf record -j any,save_type top

sure, but it does not change the fact that the report output is broken,
we need to fix it even for the 'kill' record case

jirka

Re: [PATCH v4 0/5] perf report: Show branch type

[Jiri Olsa]

On Wed, Apr 12, 2017 at 08:25:34PM +0800, Jin, Yao wrote:

SNIP

> > # Overhead  Command  Source Shared Object  Source Symbol
> > Target SymbolBasic Block Cycles
> > #   ...    
> > ...  
> > ...  ..
> > #
> >   8.30%  perf
> > Um  [kernel.vmlinux]  [k] __intel_pmu_enable_all.constprop.17  [k] 
> > native_write_msr -
> >   7.91%  perf
> > Um  [kernel.vmlinux]  [k] intel_pmu_lbr_enable_all [k] 
> > __intel_pmu_enable_all.constprop.17  -
> >   7.91%  perf
> > Um  [kernel.vmlinux]  [k] native_write_msr [k] 
> > intel_pmu_lbr_enable_all -
> >   6.32%  kill libc-2.24.so  [.] _dl_addr
> >  [.] _dl_addr -
> >   5.93%  perf
> > Um  [kernel.vmlinux]  [k] perf_iterate_ctx [k] 
> > perf_iterate_ctx -
> >   2.77%  kill libc-2.24.so  [.] malloc  
> >  [.] malloc   -
> >   1.98%  kill libc-2.24.so  [.] _int_malloc 
> >  [.] _int_malloc  -
> >   1.58%  kill [kernel.vmlinux]  [k] __rb_insert_augmented   
> >  [k] __rb_insert_augmented-
> >   1.58%  perf
> > Um  [kernel.vmlinux]  [k] perf_event_exec  [k] 
> > perf_event_exec  -
> >   1.19%  kill [kernel.vmlinux]  [k] 
> > anon_vma_interval_tree_insert[k] anon_vma_interval_tree_insert  
> >   -
> >   1.19%  kill [kernel.vmlinux]  [k] free_pgd_range  
> >  [k] free_pgd_range   -
> >   1.19%  kill [kernel.vmlinux]  [k] n_tty_write 
> >  [k] n_tty_write  -
> >   1.19%  perf
> > Um  [kernel.vmlinux]  [k] native_sched_clock   [k] 
> > sched_clock  -
> > ...
> > SNIP
> > 
> > 
> > jirka
> 
> Hi,
> 
> Thanks so much for trying this patch.
> 
> The branch statistics is printed at the end of perf report --stdio.

yep, but for some reason with your changes the head report
got changed as well, I haven't checked the details yet..

jirka

Re: [PATCH v4 0/5] perf report: Show branch type

[Jiri Olsa]

On Wed, Apr 12, 2017 at 06:21:01AM +0800, Jin Yao wrote:

SNIP

> 
> 3. Use 2 bits in perf_branch_entry for a "cross" metrics checking
>for branch cross 4K or 2M area. It's an approximate computing
>for checking if the branch cross 4K page or 2MB page.
> 
> For example:
> 
> perf record -g --branch-filter any,save_type 
> 
> perf report --stdio
> 
>  JCC forward:  27.7%
> JCC backward:   9.8%
>  JMP:   0.0%
>  IND_JMP:   6.5%
> CALL:  26.6%
> IND_CALL:   0.0%
>  RET:  29.3%
> IRET:   0.0%
> CROSS_4K:   0.0%
> CROSS_2M:  14.3%

got mangled perf report --stdio output for:


[root@ibm-x3650m4-02 perf]# ./perf record -j any,save_type kill
kill: not enough arguments
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.013 MB perf.data (18 samples) ]

[root@ibm-x3650m4-02 perf]# ./perf report --stdio -f | head -30
# To display the perf.data header info, please use --header/--header-only 
options.
#
#
# Total Lost Samples: 0
#
# Samples: 253  of event 'cycles'
# Event count (approx.): 253
#
# Overhead  Command  Source Shared Object  Source Symbol
Target SymbolBasic Block Cycles
#   ...    
...  
...  ..
#
 8.30%  perf
Um  [kernel.vmlinux]  [k] __intel_pmu_enable_all.constprop.17  [k] 
native_write_msr - 
 7.91%  perf
Um  [kernel.vmlinux]  [k] intel_pmu_lbr_enable_all [k] 
__intel_pmu_enable_all.constprop.17  - 
 7.91%  perf
Um  [kernel.vmlinux]  [k] native_write_msr [k] 
intel_pmu_lbr_enable_all - 
 6.32%  kill libc-2.24.so  [.] _dl_addr 
[.] _dl_addr - 
 5.93%  perf
Um  [kernel.vmlinux]  [k] perf_iterate_ctx [k] 
perf_iterate_ctx - 
 2.77%  kill libc-2.24.so  [.] malloc   
[.] malloc   - 
 1.98%  kill libc-2.24.so  [.] _int_malloc  
[.] _int_malloc  - 
 1.58%  kill [kernel.vmlinux]  [k] __rb_insert_augmented
[k] __rb_insert_augmented- 
 1.58%  perf
Um  [kernel.vmlinux]  [k] perf_event_exec  [k] 
perf_event_exec  - 
 1.19%  kill [kernel.vmlinux]  [k] anon_vma_interval_tree_insert
[k] anon_vma_interval_tree_insert- 
 1.19%  kill [kernel.vmlinux]  [k] free_pgd_range   
[k] free_pgd_range   - 
 1.19%  kill [kernel.vmlinux]  [k] n_tty_write  
[k] n_tty_write  - 
 1.19%  perf
Um  [kernel.vmlinux]  [k] native_sched_clock   [k] 
sched_clock  - 
...
SNIP


jirka

Re: [PATCH v21 16/19] perf, tools: Make alias matching case-insensitive

[Jiri Olsa]

On Mon, Oct 03, 2016 at 09:54:48PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 03, 2016 at 09:47:06PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Sep 15, 2016 at 03:24:53PM -0700, Sukadev Bhattiprolu escreveu:
> > > From: Andi Kleen 
> > > 
> > > Make alias matching the events parser case-insensitive. This is useful
> > > with the JSON events. perf uses lower case events, but the CPU manuals
> > > generally use upper case event names. The JSON files use lower
> > > case by default too. But if we search case insensitively then
> > > users can cut-n-paste the upper case event names.
> > > 
> > > So the following works:
> > > 
> > > % perf stat -e BR_INST_EXEC.TAKEN_INDIRECT_NEAR_CALL true
> > > 
> > >  Performance counter stats for 'true':
> > > 
> > >305  BR_INST_EXEC.TAKEN_INDIRECT_NEAR_CALL
> > > 
> > >0.000492799 seconds time elapsed
> > 
> > So now trying to figure this out:
> 
> Ok, so this is just another case of bad patch ordering: this uses 'perf
> stat' as the example, but it doesn't work at this point, one needs to go
> to the last patch, apply it and then test it again, when it will work.
> So just moved the last patch to before this one, sigh.

ok, you found it.. sry ;-)

jirka

Re: [PATCH v21 16/19] perf, tools: Make alias matching case-insensitive

[Jiri Olsa]

On Mon, Oct 03, 2016 at 09:47:06PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 15, 2016 at 03:24:53PM -0700, Sukadev Bhattiprolu escreveu:
> > From: Andi Kleen 
> > 
> > Make alias matching the events parser case-insensitive. This is useful
> > with the JSON events. perf uses lower case events, but the CPU manuals
> > generally use upper case event names. The JSON files use lower
> > case by default too. But if we search case insensitively then
> > users can cut-n-paste the upper case event names.
> > 
> > So the following works:
> > 
> > % perf stat -e BR_INST_EXEC.TAKEN_INDIRECT_NEAR_CALL true
> > 
> >  Performance counter stats for 'true':
> > 
> >305  BR_INST_EXEC.TAKEN_INDIRECT_NEAR_CALL
> > 
> >0.000492799 seconds time elapsed
> 
> So now trying to figure this out:
> 
> [acme@jouet linux]$ perf stat -e br_inst_exec.all_direct_near_call true
> event syntax error: 'br_inst_exec.all_direct_near_call'
>  \___ 'period' is not usable in 'perf stat'
> Run 'perf list' for a list of valid events

looks like you need also this one:
  155128eaa62f Allow period= in perf stat CPU event descriptions.

jirka

Re: [PATCH v21 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Thu, Sep 29, 2016 at 07:19:48PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 27, 2016 at 04:18:46PM +0200, Jiri Olsa escreveu:
> > On Mon, Sep 26, 2016 at 09:59:54AM -0700, Andi Kleen wrote:
> > > On Mon, Sep 26, 2016 at 12:03:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Mon, Sep 26, 2016 at 10:35:33AM +0200, Jiri Olsa escreveu:
> > > > > ping.. is that working for you? IMO we can include this
> > > > > as additional patch to the set..
> > > > 
> > > > No, it doesn't fails to build on the first cross env I tried, fixing it
> > > > now, resulting patch:
> > > 
> > > Yes it shouldn't be difficult to fix cross building. I don't think
> > > there are any fundamental problems.
> > 
> > right, how about attached patch
> > 
> > Arnaldo,
> > could you please try it on cross build.. I still dont have setup for that 
> > :-\
> > 
> > thanks,
> > jirka
> 
> So, this makes it work for me in one of the cross build envs I have (all
> in https://hub.docker.com/r/acmel/) if I apply this patch on top:
> 
> diff --git a/tools/build/Makefile b/tools/build/Makefile
> index 653faee2a055..8332959fbca4 100644
> --- a/tools/build/Makefile
> +++ b/tools/build/Makefile
> @@ -42,7 +42,7 @@ $(OUTPUT)fixdep-in.o: FORCE
>   $(Q)$(MAKE) $(build)=fixdep
>  
>  $(OUTPUT)fixdep: $(OUTPUT)fixdep-in.o
> - $(QUIET_LINK)$(CC) $(LDFLAGS) -o $@ $<
> + $(QUIET_LINK)$(HOSTCC) $(LDFLAGS) -o $@ $<
>  
>  FORCE:
>  
> ---
> 
> I've broken up the patch into multiple ones, to get first fixdep
> working, then to move to jevents, I'm putting this on a
> tmp.perf/hostprog branch till I've tested it all.

looks great, thanks

jirka

Re: [PATCH v21 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Mon, Sep 26, 2016 at 09:59:54AM -0700, Andi Kleen wrote:
> On Mon, Sep 26, 2016 at 12:03:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Sep 26, 2016 at 10:35:33AM +0200, Jiri Olsa escreveu:
> > > ping.. is that working for you? IMO we can include this
> > > as additional patch to the set..
> > 
> > No, it doesn't fails to build on the first cross env I tried, fixing it
> > now, resulting patch:
> 
> Yes it shouldn't be difficult to fix cross building. I don't think
> there are any fundamental problems.

right, how about attached patch

Arnaldo,
could you please try it on cross build.. I still dont have setup for that :-\

thanks,
jirka


---
diff --git a/tools/build/Build b/tools/build/Build
index 63a6c34c0c88..76d1a4960973 100644
--- a/tools/build/Build
+++ b/tools/build/Build
@@ -1 +1,3 @@
+hostprogs := fixdep
+
 fixdep-y := fixdep.o
diff --git a/tools/build/Makefile b/tools/build/Makefile
index 0d5a0e3a8fa9..653faee2a055 100644
--- a/tools/build/Makefile
+++ b/tools/build/Makefile
@@ -14,6 +14,12 @@ endef
 $(call allow-override,CC,$(CROSS_COMPILE)gcc)
 $(call allow-override,LD,$(CROSS_COMPILE)ld)
 
+HOSTCC ?= gcc
+HOSTLD ?= ld
+HOSTAR ?= ar
+
+export HOSTCC HOSTLD HOSTAR
+
 ifeq ($(V),1)
   Q =
 else
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build
index 27f3583193e6..031c5631cc21 100644
--- a/tools/build/Makefile.build
+++ b/tools/build/Makefile.build
@@ -58,6 +58,9 @@ quiet_cmd_mkdir = MKDIR$(dir $@)
 quiet_cmd_cc_o_c = CC   $@
   cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
 
+quiet_cmd_host_cc_o_c = HOSTCC   $@
+  cmd_host_cc_o_c = $(HOSTCC) $(c_flags) -c -o $@ $<
+
 quiet_cmd_cpp_i_c = CPP  $@
   cmd_cpp_i_c = $(CC) $(c_flags) -E -o $@ $<
 
@@ -70,16 +73,24 @@ quiet_cmd_gen = GEN  $@
 # If there's nothing to link, create empty $@ object.
 quiet_cmd_ld_multi = LD   $@
   cmd_ld_multi = $(if $(strip $(obj-y)),\
-  $(LD) -r -o $@  $(filter $(obj-y),$^),rm -f $@; $(AR) 
rcs $@)
+ $(LD) -r -o $@  $(filter $(obj-y),$^),rm -f $@; $(AR) rcs 
$@)
+
+quiet_cmd_host_ld_multi = HOSTLD   $@
+  cmd_host_ld_multi = $(if $(strip $(obj-y)),\
+  $(HOSTLD) -r -o $@  $(filter $(obj-y),$^),rm -f $@; 
$(HOSTAR) rcs $@)
+
+ifneq ($(filter $(obj),$(hostprogs)),)
+  host = host_
+endif
 
 # Build rules
 $(OUTPUT)%.o: %.c FORCE
$(call rule_mkdir)
-   $(call if_changed_dep,cc_o_c)
+   $(call if_changed_dep,$(host)cc_o_c)
 
 $(OUTPUT)%.o: %.S FORCE
$(call rule_mkdir)
-   $(call if_changed_dep,cc_o_c)
+   $(call if_changed_dep,$(host)cc_o_c)
 
 $(OUTPUT)%.i: %.c FORCE
$(call rule_mkdir)
@@ -119,7 +130,7 @@ $(sort $(subdir-obj-y)): $(subdir-y) ;
 
 $(in-target): $(obj-y) FORCE
$(call rule_mkdir)
-   $(call if_changed,ld_multi)
+   $(call if_changed,$(host)ld_multi)
 
 __build: $(in-target)
@:
diff --git a/tools/build/Makefile.include b/tools/build/Makefile.include
index be630bed66d2..ad22e4e7bc59 100644
--- a/tools/build/Makefile.include
+++ b/tools/build/Makefile.include
@@ -1,10 +1,6 @@
 build := -f $(srctree)/tools/build/Makefile.build dir=. obj
 
-ifdef CROSS_COMPILE
-fixdep:
-else
 fixdep:
$(Q)$(MAKE) -C $(srctree)/tools/build CFLAGS= LDFLAGS= $(OUTPUT)fixdep
-endif
 
 .PHONY: fixdep
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 0abebcba849f..1347b5de3669 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -144,6 +144,10 @@ $(call allow-override,LD,$(CROSS_COMPILE)ld)
 
 LD += $(EXTRA_LDFLAGS)
 
+HOSTCC  ?= gcc
+HOSTLD  ?= ld
+HOSTAR  ?= ar
+
 PKG_CONFIG = $(CROSS_COMPILE)pkg-config
 
 RM  = rm -f
@@ -345,6 +349,7 @@ strip: $(PROGRAMS) $(OUTPUT)perf
 PERF_IN := $(OUTPUT)perf-in.o
 
 export srctree OUTPUT RM CC LD AR CFLAGS V BISON FLEX AWK
+export HOSTCC HOSTLD HOSTAR
 include $(srctree)/tools/build/Makefile.include
 
 JEVENTS   := $(OUTPUT)pmu-events/jevents
diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build
index d2f34307ae79..9213a1273697 100644
--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build
@@ -1,3 +1,5 @@
+hostprogs := jevents
+
 jevents-y  += json.o jsmn.o jevents.o
 pmu-events-y   += pmu-events.o
 JDIR   =  pmu-events/arch/$(ARCH)

Re: [PATCH v21 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Thu, Sep 22, 2016 at 06:27:13PM +0200, Jiri Olsa wrote:
> On Thu, Sep 22, 2016 at 05:00:22PM +0200, Jiri Olsa wrote:
> > On Mon, Sep 19, 2016 at 09:28:20PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Sep 19, 2016 at 09:02:58PM -0300, Arnaldo Carvalho de Melo 
> > > escreveu:
> > > > Em Mon, Sep 19, 2016 at 08:37:53PM -0300, Arnaldo Carvalho de Melo 
> > > > escreveu:
> > > > > yeah, changing that typedef + true def to plain include 
> > > > > makes it progress to the next failure, which is in cross compilation
> > > > > environments, such as using fedora 24 + the Android NDK to try to 
> > > > > build
> > > > > a ARM android binary.
> > > 
> > > > 14 fedora:24-x-ARC-uClibc: FAIL
> > > >   GEN  /tmp/build/perf/pmu-events/pmu-events.c
> > > > /bin/sh: /tmp/build/perf/pmu-events/jevents: cannot execute binary 
> > > > file: Exec format error
> > > > pmu-events/Build:11: recipe for target 
> > > > '/tmp/build/perf/pmu-events/pmu-events.c' failed
> > > > make[2]: *** [/tmp/build/perf/pmu-events/pmu-events.c] Error 126
> > > > Makefile.perf:461: recipe for target 
> > > > '/tmp/build/perf/pmu-events/pmu-events-in.o' failed
> > > > make[1]: *** [/tmp/build/perf/pmu-events/pmu-events-in.o] Error 2
> > > > make[1]: *** Waiting for unfinished jobs
> > > 
> > > Jiri, we need something similar to scripts/Makefile.host :-\
> > > 
> > > Calling it a day, perhaps, for now, we should just detect that it is a
> > > corss compile env (CROSS_COMPILE is set) and exclude all this code from
> > > the build, emitting a warning.
> > > 
> > > I left what I did at the tmp.perf/core branch of my repo at
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git.
> > 
> > as discussed on irc we will disable it for cross builds now,
> > because we dont have good solution at the moment.. it's
> > similar case as for fixdep tool:
> > 
> > 3a70fcd3a4db tools build: Fix cross compile build
> > ...
> > We need to add support for host side tools build, meanwhile
> > disabling fixdep usage for cross arch builds.
> > 
> > I'll make a change to disable this for crossbuild and
> > work on common solution later
> 
> could you please give it a try with patch below?
> I tested but not with properly cross building...
> 
> also,  did you want some message during the cross build that pmu-events are 
> not included?

ping.. is that working for you? IMO we can include this
as additional patch to the set..

thanks,
jirka

Re: [PATCH v21 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Thu, Sep 22, 2016 at 05:00:22PM +0200, Jiri Olsa wrote:
> On Mon, Sep 19, 2016 at 09:28:20PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Sep 19, 2016 at 09:02:58PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Sep 19, 2016 at 08:37:53PM -0300, Arnaldo Carvalho de Melo 
> > > escreveu:
> > > > yeah, changing that typedef + true def to plain include 
> > > > makes it progress to the next failure, which is in cross compilation
> > > > environments, such as using fedora 24 + the Android NDK to try to build
> > > > a ARM android binary.
> > 
> > > 14 fedora:24-x-ARC-uClibc: FAIL
> > >   GEN  /tmp/build/perf/pmu-events/pmu-events.c
> > > /bin/sh: /tmp/build/perf/pmu-events/jevents: cannot execute binary file: 
> > > Exec format error
> > > pmu-events/Build:11: recipe for target 
> > > '/tmp/build/perf/pmu-events/pmu-events.c' failed
> > > make[2]: *** [/tmp/build/perf/pmu-events/pmu-events.c] Error 126
> > > Makefile.perf:461: recipe for target 
> > > '/tmp/build/perf/pmu-events/pmu-events-in.o' failed
> > > make[1]: *** [/tmp/build/perf/pmu-events/pmu-events-in.o] Error 2
> > > make[1]: *** Waiting for unfinished jobs
> > 
> > Jiri, we need something similar to scripts/Makefile.host :-\
> > 
> > Calling it a day, perhaps, for now, we should just detect that it is a
> > corss compile env (CROSS_COMPILE is set) and exclude all this code from
> > the build, emitting a warning.
> > 
> > I left what I did at the tmp.perf/core branch of my repo at
> > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git.
> 
> as discussed on irc we will disable it for cross builds now,
> because we dont have good solution at the moment.. it's
> similar case as for fixdep tool:
> 
> 3a70fcd3a4db tools build: Fix cross compile build
> ...
> We need to add support for host side tools build, meanwhile
> disabling fixdep usage for cross arch builds.
> 
> I'll make a change to disable this for crossbuild and
> work on common solution later

could you please give it a try with patch below?
I tested but not with properly cross building...

also,  did you want some message during the cross build that pmu-events are not 
included?

thanks,
jirka


---
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 24803c58049a..ec3a59d9a56b 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -755,6 +755,10 @@ ifndef NO_AUXTRACE
   endif
 endif
 
+ifndef CROSS_COMPILE
+  CFLAGS += -DHAVE_PMU_EVENTS_SUPPORT
+endif
+
 # Among the variables below, these:
 #   perfexecdir
 #   template_dir
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 0abebcba849f..85257c49d5ab 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -349,7 +349,14 @@ include $(srctree)/tools/build/Makefile.include
 
 JEVENTS   := $(OUTPUT)pmu-events/jevents
 JEVENTS_IN:= $(OUTPUT)pmu-events/jevents-in.o
+
+#
+# Disabling pmu-events for cross compile, as
+# we dont support host CC tools building yet.
+#
+ifndef CROSS_COMPILE
 PMU_EVENTS_IN := $(OUTPUT)pmu-events/pmu-events-in.o
+endif
 
 export JEVENTS
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ac097fcaba7b..21a3f21a6f6f 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -482,6 +482,8 @@ static struct cpu_map *pmu_cpumask(const char *name)
return cpus;
 }
 
+#ifdef HAVE_PMU_EVENTS_SUPPORT
+
 /*
  * Return the CPU id as a raw string.
  *
@@ -545,6 +547,13 @@ out:
return 0;
 }
 
+#else
+static int pmu_add_cpu_aliases(struct list_head *head __maybe_unused)
+{
+   return 0;
+}
+#endif /* HAVE_PMU_EVENTS_SUPPORT */
+
 struct perf_event_attr * __weak
 perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
 {

Re: [PATCH v21 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Mon, Sep 19, 2016 at 09:28:20PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 19, 2016 at 09:02:58PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Sep 19, 2016 at 08:37:53PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > yeah, changing that typedef + true def to plain include 
> > > makes it progress to the next failure, which is in cross compilation
> > > environments, such as using fedora 24 + the Android NDK to try to build
> > > a ARM android binary.
> 
> > 14 fedora:24-x-ARC-uClibc: FAIL
> >   GEN  /tmp/build/perf/pmu-events/pmu-events.c
> > /bin/sh: /tmp/build/perf/pmu-events/jevents: cannot execute binary file: 
> > Exec format error
> > pmu-events/Build:11: recipe for target 
> > '/tmp/build/perf/pmu-events/pmu-events.c' failed
> > make[2]: *** [/tmp/build/perf/pmu-events/pmu-events.c] Error 126
> > Makefile.perf:461: recipe for target 
> > '/tmp/build/perf/pmu-events/pmu-events-in.o' failed
> > make[1]: *** [/tmp/build/perf/pmu-events/pmu-events-in.o] Error 2
> > make[1]: *** Waiting for unfinished jobs
> 
> Jiri, we need something similar to scripts/Makefile.host :-\
> 
> Calling it a day, perhaps, for now, we should just detect that it is a
> corss compile env (CROSS_COMPILE is set) and exclude all this code from
> the build, emitting a warning.
> 
> I left what I did at the tmp.perf/core branch of my repo at
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git.

as discussed on irc we will disable it for cross builds now,
because we dont have good solution at the moment.. it's
similar case as for fixdep tool:

3a70fcd3a4db tools build: Fix cross compile build
...
We need to add support for host side tools build, meanwhile
disabling fixdep usage for cross arch builds.

I'll make a change to disable this for crossbuild and
work on common solution later

thanks,
jirka

Re: [PATCH] perf tests: Add dwarf unwind test for powerpc

[Jiri Olsa]

On Mon, Sep 19, 2016 at 02:38:20AM -0400, Ravi Bangoria wrote:
> User stack dump feature is recently added for powerpc. But there was no
> test case available to test it. This test works same as other on arch by
> preparing stack frame on perf test thread and comparing each frame by
> unwinding it.
> 
>   $ ./perf test 50
> 50: Test dwarf unwind: Ok
> 
> User stack dump for powerpc: https://lkml.org/lkml/2016/4/28/482
> 
> Signed-off-by: Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com>

can't test, but looks good

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

Re: [PATCH v20 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Wed, Aug 31, 2016 at 09:15:30AM -0700, Andi Kleen wrote:
> > > 
> > > > 
> > > > I've already made some changes in pmu-events/* to support
> > > > this hierarchy to see how bad the change would be.. and
> > > > it's not that bad ;-)
> > > 
> > > Everything has to be automated, please no manual changes.
> > 
> > sure
> > 
> > so, if you're ok with the layout, how do you want to proceed further?
> 
> If the split version is acceptable it's fine for me to merge it.
> 
> I'll add split-json to my scripting, so the next update would
> be split too.

ook, I'll wait for patches then

thanks,
jirka

Re: [PATCH v20 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Wed, Aug 31, 2016 at 07:42:47AM -0700, Andi Kleen wrote:
> > hi,
> > I had discussion with Ingo about the state of this patchset
> > and there's one more requirement from his side - to split
> > event files into per topic files
> 
> Thanks Jiri.
> > 
> > I made some initial changes over latest Sukadev's branch
> > and came up with something like this:
> 
> Did you just split it by the "Topic" fields?

yep

> 
> > 
> > $ find pmu-events/arch/x86/
> > pmu-events/arch/x86/
> > pmu-events/arch/x86/NehalemEX_core
> > pmu-events/arch/x86/NehalemEX_core/Memory.json
> > pmu-events/arch/x86/NehalemEX_core/Virtual-Memory.json
> > pmu-events/arch/x86/NehalemEX_core/Cache.json
> > pmu-events/arch/x86/NehalemEX_core/Pipeline.json
> > pmu-events/arch/x86/NehalemEX_core/Floating-point.json
> > pmu-events/arch/x86/NehalemEX_core/Other.json
> > pmu-events/arch/x86/mapfile.csv
> > pmu-events/arch/x86/Broadwell_core
> > pmu-events/arch/x86/Broadwell_core/Memory.json
> > pmu-events/arch/x86/Broadwell_core/Virtual-Memory.json
> > pmu-events/arch/x86/Broadwell_core/Cache.json
> > pmu-events/arch/x86/Broadwell_core/Pipeline.json
> > pmu-events/arch/x86/Broadwell_core/Floating-point.json
> > pmu-events/arch/x86/Broadwell_core/Other.json
> > pmu-events/arch/x86/Broadwell_core/Frontend.json
> > 
> > so let's have a discussion if this is acceptable for you guys
> 
> Splitting is fine for me, as long as it's scriptable.
> 
> I already have some scripts to generate the perf json files,
> can update them to split.

yep, there's split-json.py script earlier in the perf/json branch

> 
> > 
> > I've already made some changes in pmu-events/* to support
> > this hierarchy to see how bad the change would be.. and
> > it's not that bad ;-)
> 
> Everything has to be automated, please no manual changes.

sure

so, if you're ok with the layout, how do you want to proceed further?

thanks,
jirka

Re: [PATCH v20 00/20] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Mon, Jun 20, 2016 at 09:02:30PM -0700, Sukadev Bhattiprolu wrote:
> CPUs support a large number of performance monitoring events (PMU events)
> and often these events are very specific to an architecture/model of the
> CPU. To use most of these PMU events with perf, we currently have to identify
> them by their raw codes:
> 
>   perf stat -e r100f2 sleep 1
> 
> This patchset allows architectures to specify these PMU events in JSON
> files located in 'tools/perf/pmu-events/arch/' of the mainline tree.
> The events from the JSON files for the architecture are then built into
> the perf binary.
> 
> At run time, perf identifies the specific set of events for the CPU and
> creates "event aliases". These aliases allow users to specify events by
> "name" as:
> 
>   perf stat -e pm_1plus_ppc_cmpl sleep 1
> 
> The file, 'tools/perf/pmu-events/README' in [PATCH 16/16] gives more
> details.
> 
> Note:
>   - All known events tables for the architecture are included in the
> perf binary.
> 
>   - For architectures that don't have any JSON files, an empty mapping
>     table is created and they should continue to build.
> 
> Thanks to input from Andi Kleen, Jiri Olsa, Namhyung Kim and Ingo Molnar.
> 
> These patches are available from:
> 
>   https://github.com/sukadev/linux.git 
>   
>   Branch  Description
>   --
>   json-code-v20   Source Code only 
>   json-data-v20   x86 and Powerpc datafiles only
>   json-code+data-v20  Both code and data (for build/test)
>   
> NOTE: Only "source code" patches (i.e those in json-code-v20) are 
> being
>   emailed.  Please pull the "data files" from the json-data-v20 branch.
> 
> Changelog[v20]
>   - Rebase to recent perf/core
>   - Add Patch 20/20 to allow perf-stat to work with the period= field

hi,
I had discussion with Ingo about the state of this patchset
and there's one more requirement from his side - to split
event files into per topic files

I made some initial changes over latest Sukadev's branch
and came up with something like this:

$ find pmu-events/arch/x86/
pmu-events/arch/x86/
pmu-events/arch/x86/NehalemEX_core
pmu-events/arch/x86/NehalemEX_core/Memory.json
pmu-events/arch/x86/NehalemEX_core/Virtual-Memory.json
pmu-events/arch/x86/NehalemEX_core/Cache.json
pmu-events/arch/x86/NehalemEX_core/Pipeline.json
pmu-events/arch/x86/NehalemEX_core/Floating-point.json
pmu-events/arch/x86/NehalemEX_core/Other.json
pmu-events/arch/x86/mapfile.csv
pmu-events/arch/x86/Broadwell_core
pmu-events/arch/x86/Broadwell_core/Memory.json
pmu-events/arch/x86/Broadwell_core/Virtual-Memory.json
pmu-events/arch/x86/Broadwell_core/Cache.json
pmu-events/arch/x86/Broadwell_core/Pipeline.json
pmu-events/arch/x86/Broadwell_core/Floating-point.json
pmu-events/arch/x86/Broadwell_core/Other.json
pmu-events/arch/x86/Broadwell_core/Frontend.json

so let's have a discussion if this is acceptable for you guys

I've already made some changes in pmu-events/* to support
this hierarchy to see how bad the change would be.. and
it's not that bad ;-)

you can check followin patches (only 2 Intel files transformed):

1d5ffa8bb969 perf, tools: Change jevents
65919f8901e3 perf, tools: Split Broadwell_core.json
7cd309a85465 perf, tools: Add Broadwell V14 event file
e316aff2dd4e perf, tools: Split NehalemEX_core.json
e19e8de49408 perf, tools: Add NehalemEX V1 event file

It's available in:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/json

thanks,
jirka

Re: [PATCH v5] tools/perf: Fix the mask in regs_dump__printf and print_sample_iregs

[Jiri Olsa]

On Thu, Jun 23, 2016 at 11:19:27AM +0530, Madhavan Srinivasan wrote:

SNIP

> 
> Changelog v1:
> 1)updated commit message and patch subject
> 2)Add the fix to print_sample_iregs() in builtin-script.c
> 
>  tools/include/linux/bitmap.h |  2 ++
>  tools/lib/bitmap.c   | 18 ++
>  tools/perf/builtin-script.c  |  4 +++-
>  tools/perf/util/session.c|  4 +++-
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
> index 28f5493da491..5e98525387dc 100644
> --- a/tools/include/linux/bitmap.h
> +++ b/tools/include/linux/bitmap.h
> @@ -2,6 +2,7 @@
>  #define _PERF_BITOPS_H
>  
>  #include 
> +#include 

this could go in the bitmap.c file, but anyway:

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

>  #include 
>  
>  #define DECLARE_BITMAP(name,bits) \
> @@ -10,6 +11,7 @@
>  int __bitmap_weight(const unsigned long *bitmap, int bits);
>  void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
>const unsigned long *bitmap2, int bits);
> +void bitmap_from_u64(unsigned long *dst, u64 mask);
>  
>  #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 
> 1)))
>  

SNIP
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3] tools/perf: Fix the mask in regs_dump__printf and print_sample_iregs

[Jiri Olsa]

On Tue, Jun 21, 2016 at 06:35:31PM +0300, Yury Norov wrote:

SNIP

> > index 5214974e841a..1337b1c73f82 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -940,8 +940,22 @@ static void branch_stack__printf(struct perf_sample 
> > *sample)
> >  static void regs_dump__printf(u64 mask, u64 *regs)
> >  {
> > unsigned rid, i = 0;
> > +   unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
> >  
> > -   for_each_set_bit(rid, (unsigned long *) , sizeof(mask) * 8) {
> > +   /*
> > +* Since u64 is passed as 'unsigned long *', check
> > +* to see whether we need to swap words within u64.
> > +* Reason being, in 32 bit big endian userspace on a
> > +* 64bit kernel, 'unsigned long' is 32 bits.
> > +* When reading u64 using (u32 *)()[0] and (u32 *)()[1],
> > +* we will get wrong value for the mask. This is what
> > +* find_first_bit() and find_next_bit() is doing.
> > +* Issue here is "(u32 *)()[0]" gets upper 32 bits of u64,
> > +* but perf assumes it gets lower 32bits of u64. Hence the check
> > +* and swap.
> > +*/
> 
> Identical comments... I'd prefer to see it in commit message, or
> better in function description. For me it's pretty straightforward in
> understanding what happens here in-place without comments.

yep, please use this just once as the fucntion description

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] tools/perf: Fix the mask in regs_dump__printf and

[Jiri Olsa]

On Mon, Jun 20, 2016 at 05:27:25PM +0800, Wangnan (F) wrote:
> 
> 
> On 2016/6/20 17:18, Jiri Olsa wrote:
> > On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote:
> > > When decoding the perf_regs mask in regs_dump__printf(),
> > > we loop through the mask using find_first_bit and find_next_bit functions.
> > > "mask" is of type "u64", but sent as a "unsigned long *" to
> > > lib functions along with sizeof(). While the exisitng code works fine in
> > > most of the case, the logic is broken when using a 32bit perf on a
> > > 64bit kernel (Big Endian). We end up reading the wrong word of the u64
> > > first in the lib functions.
> > hum, I still don't see why this happens.. why do we read the
> > wrong word in this case?
> 
> If you read a u64 using (u32 *)()[0] and (u32 *)()[1]
> you can get wrong value. This is what _find_next_bit() is doing.
> 
> In a big endian environment where 'unsigned long' is 32 bits
> long, "(u32 *)()[0]" gets upper 32 bits, but without this patch
> perf assumes it gets lower 32 bits. The root cause is wrongly convert
> u64 value to bitmap.

i see, could you please put this into comment in the code?

also we could have common function for that, to keep it on
one place only, like bitmap_from_u64 or so

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] tools/perf: Fix the mask in regs_dump__printf and

[Jiri Olsa]

On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote:

SNIP

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index e3ce2f34d3ad..76d5006ebcc3 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -412,11 +412,16 @@ static void print_sample_iregs(struct perf_sample 
> *sample,
>   struct regs_dump *regs = >intr_regs;
>   uint64_t mask = attr->sample_regs_intr;
>   unsigned i = 0, r;
> + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
>  
>   if (!regs)
>   return;
>  
> - for_each_set_bit(r, (unsigned long *) , sizeof(mask) * 8) {
> + _mask[0] = mask & ULONG_MAX;
> + if (sizeof(mask) > sizeof(unsigned long))
> + _mask[1] = mask >> 32;
> +
> + for_each_set_bit(r, _mask, sizeof(mask) * 8) {
>   u64 val = regs->regs[i++];
>   printf("%5s:0x%"PRIx64" ", perf_reg_name(r), val);
>   }
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 5214974e841a..2eaa42a4832a 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample 
> *sample)
>  static void regs_dump__printf(u64 mask, u64 *regs)
>  {
>   unsigned rid, i = 0;
> + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
>  
> - for_each_set_bit(rid, (unsigned long *) , sizeof(mask) * 8) {
> + _mask[0] = mask & ULONG_MAX;
> + if (sizeof(mask) > sizeof(unsigned long))
> + _mask[1] = mask >> 32;
> +
> + for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
>   u64 val = regs[i++];

could you please move the common code into the function?

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] tools/perf: Fix the mask in regs_dump__printf and

[Jiri Olsa]

On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote:
> When decoding the perf_regs mask in regs_dump__printf(),
> we loop through the mask using find_first_bit and find_next_bit functions.
> "mask" is of type "u64", but sent as a "unsigned long *" to
> lib functions along with sizeof(). While the exisitng code works fine in
> most of the case, the logic is broken when using a 32bit perf on a
> 64bit kernel (Big Endian). We end up reading the wrong word of the u64
> first in the lib functions.

hum, I still don't see why this happens.. why do we read the
wrong word in this case?

thanks,
jirka

> first in the lib functions. Proposed fix is to swap the words of the
> u64 to handle this case. This is not endianess swap.
> 

SNIP


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf

[Jiri Olsa]

On Fri, Jun 17, 2016 at 02:43:31PM +0530, Madhavan Srinivasan wrote:

SNIP

> 
>  
> if (data->user_regs.abi) {
> -   u64 mask = evsel->attr.sample_regs_user;
> +   u.val64 = evsel->attr.sample_regs_user;
>  
> -   sz = hweight_long(mask) * sizeof(u64);
> +   if (sizeof(u64) > sizeof(unsigned long)) {
> +   u64 mask = u.val64;
> +   u.val32[1] = mask >> 32;
> +   u.val32[0] = mask & ULONG_MAX;
> +   }
> +
> +   sz = hweight_long(u.val64) * sizeof(u64);
> OVERFLOW_CHECK(array, sz, max_size);
> -   data->user_regs.mask = mask;
> +   data->user_regs.mask = u.val64;
> data->user_regs.regs = (u64 *)array;
> 
> Issue I see is when printing the mask value in a 32bit perf
> on a 64bit kernel (big endian).
> 
> 442044948492 0xdc0 [0x188]: PERF_RECORD_SAMPLE(IP, 0x1): 7299/7299:
> 0xc0059200 period: 12599 addr: 0
> ... intr regs: mask 0x07ff ABI 32-bit
> ^^^ shld have been 0x7ff
> 
> I agree it is better to fix this when reading, but
> we need to swap again when printing?

hum, sample data should be swap at this point already..
see perf_session__process_event

jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf

[Jiri Olsa]

On Fri, Jun 17, 2016 at 10:52:38AM +0530, Madhavan Srinivasan wrote:
> When decoding the perf_regs mask in regs_dump__printf(),
> we loop through the mask using find_first_bit and find_next_bit functions.
> And mask is of type "u64". But "u64" is send as a "unsigned long *" to
> lib functions along with sizeof().
> 
> While the exisitng code works fine in most of the case, when using a
> 32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
> in the u64 mask. Patch to fix the mask in regs_dump__printf().
> 
> Suggested-by: Yury Norov <yno...@caviumnetworks.com>
> Cc: Yury Norov <yno...@caviumnetworks.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: Arnaldo Carvalho de Melo <a...@kernel.org>
> Cc: Alexander Shishkin <alexander.shish...@linux.intel.com>
> Cc: Jiri Olsa <jo...@kernel.org>
> Cc: Adrian Hunter <adrian.hun...@intel.com>
> Cc: Kan Liang <kan.li...@intel.com>
> Cc: Wang Nan <wangn...@huawei.com>
> Cc: Michael Ellerman <m...@ellerman.id.au>
> Signed-off-by: Madhavan Srinivasan <ma...@linux.vnet.ibm.com>
> ---
>  tools/perf/util/session.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 5214974e841a..2eaa42a4832a 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample 
> *sample)
>  static void regs_dump__printf(u64 mask, u64 *regs)
>  {
>   unsigned rid, i = 0;
> + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
>  
> - for_each_set_bit(rid, (unsigned long *) , sizeof(mask) * 8) {
> + _mask[0] = mask & ULONG_MAX;
> + if (sizeof(mask) > sizeof(unsigned long))
> + _mask[1] = mask >> 32;
> +

I think we should do this earlier when reading the mask,
not at the moment we just print it, like we do for other
types.. maybe we could do this as an extra bit for
perf_event__swap_ops[PERF_RECORD_SAMPLE] function

also there's print_sample_iregs in builtin-script.c that's
most likely affected as well

thanks,
jirka

> + for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
>   u64 val = regs[i++];
>  
>   printf(" %-5s 0x%" PRIx64 "\n",
> -- 
> 1.9.1
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 1/2] tools include: Adopt byte ordering macros from byteorder/generic.h

[Jiri Olsa]

On Thu, Jun 16, 2016 at 10:41:31AM +, He Kuang wrote:
> From: Wang Nan 
> 
> This patch adopts the macros for byte order conversion from
> "include/linux/byteorder/generic.h" to
> "tools/include/linux/byteorder/generic.h"
> 
> tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.

other than explanation for the kbuild test robot email:
  http://marc.info/?l=linux-kernel=146605819924421=2

it looks ok..

jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] tools include: Sync byteorder/generic.h

[Jiri Olsa]

On Thu, Jun 16, 2016 at 01:32:08AM +, He Kuang wrote:
> From: Wang Nan 
> 
> This patch copies "include/linux/byteorder/generic.h" to
> "tools/include/linux/byteorder/generic.h" to enable other libraries to
> use macros in it.

it's not the file copied, as the changelog suggest,
just several macros, please fix the changelog

thanks,
jirka

> 
> tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.
> 
> Signed-off-by: Wang Nan 
> Signed-off-by: He Kuang 
> ---
>  tools/include/linux/byteorder/generic.h | 48 
> +
>  tools/perf/MANIFEST |  1 +
>  2 files changed, 49 insertions(+)
>  create mode 100644 tools/include/linux/byteorder/generic.h
> 
> diff --git a/tools/include/linux/byteorder/generic.h 
> b/tools/include/linux/byteorder/generic.h
> new file mode 100644
> index 000..41b4507
> --- /dev/null
> +++ b/tools/include/linux/byteorder/generic.h
> @@ -0,0 +1,48 @@
> +#ifndef _TOOLS_LINUX_BYTEORDER_GENERIC_H
> +#define _TOOLS_LINUX_BYTEORDER_GENERIC_H
> +
> +#include 
> +#include 
> +
> +#define cpu_to_le64 __cpu_to_le64
> +#define le64_to_cpu __le64_to_cpu
> +#define cpu_to_le32 __cpu_to_le32
> +#define le32_to_cpu __le32_to_cpu
> +#define cpu_to_le16 __cpu_to_le16
> +#define le16_to_cpu __le16_to_cpu
> +#define cpu_to_be64 __cpu_to_be64
> +#define be64_to_cpu __be64_to_cpu
> +#define cpu_to_be32 __cpu_to_be32
> +#define be32_to_cpu __be32_to_cpu
> +#define cpu_to_be16 __cpu_to_be16
> +#define be16_to_cpu __be16_to_cpu
> +
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +#define __cpu_to_le16 bswap_16
> +#define __cpu_to_le32 bswap_32
> +#define __cpu_to_le64 bswap_64
> +#define __le16_to_cpu bswap_16
> +#define __le32_to_cpu bswap_32
> +#define __le64_to_cpu bswap_64
> +#define __cpu_to_be16
> +#define __cpu_to_be32
> +#define __cpu_to_be64
> +#define __be16_to_cpu
> +#define __be32_to_cpu
> +#define __be64_to_cpu
> +#else
> +#define __cpu_to_le16
> +#define __cpu_to_le32
> +#define __cpu_to_le64
> +#define __le16_to_cpu
> +#define __le32_to_cpu
> +#define __le64_to_cpu
> +#define __cpu_to_be16 bswap_16
> +#define __cpu_to_be32 bswap_32
> +#define __cpu_to_be64 bswap_64
> +#define __be16_to_cpu bswap_16
> +#define __be32_to_cpu bswap_32
> +#define __be64_to_cpu bswap_64
> +#endif
> +
> +#endif /* _TOOLS_LINUX_BYTEORDER_GENERIC_H */
> diff --git a/tools/perf/MANIFEST b/tools/perf/MANIFEST
> index 8c8c6b9..80ac3d4 100644
> --- a/tools/perf/MANIFEST
> +++ b/tools/perf/MANIFEST
> @@ -46,6 +46,7 @@ tools/include/asm-generic/bitops/hweight.h
>  tools/include/asm-generic/bitops.h
>  tools/include/linux/atomic.h
>  tools/include/linux/bitops.h
> +tools/include/linux/byteorder/generic.h
>  tools/include/linux/compiler.h
>  tools/include/linux/filter.h
>  tools/include/linux/hash.h
> -- 
> 1.8.5.2
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* for big endian

[Jiri Olsa]

On Thu, Jun 16, 2016 at 01:32:09AM +, He Kuang wrote:
> From: Wang Nan 
> 
> The cpu_to_le* macros in kernel.h are defined without considering
> endianese. This patch includes "byteoder/generic.h" instead to fix the
> bug, and removes redundant le64_to_cpu definition in intel-bts.c.
> 
> Signed-off-by: Wang Nan 
> Signed-off-by: He Kuang 
> ---
>  tools/include/linux/kernel.h | 5 ++---
>  tools/perf/util/intel-bts.c  | 5 -
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
> index 76df535..6145e41 100644
> --- a/tools/include/linux/kernel.h
> +++ b/tools/include/linux/kernel.h
> @@ -64,11 +64,10 @@
>  #endif
>  
>  /*
> - * Both need more care to handle endianness
> + * Need more care to handle endianness
>   * (Don't use bitmap_copy_le() for now)

what's the purpose of this comment now?

>   */
> -#define cpu_to_le64(x)   (x)
> -#define cpu_to_le32(x)   (x)
> +#include 
>  
>  static inline int
>  vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index 9df9960..0e632c4 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -40,11 +40,6 @@
>  #define INTEL_BTS_ERR_NOINSN  5
>  #define INTEL_BTS_ERR_LOST9
>  
> -#if __BYTE_ORDER == __BIG_ENDIAN
> -#define le64_to_cpu bswap_64
> -#else
> -#define le64_to_cpu
> -#endif

the purpose of this patchset is to unify these macros right?

there're more conversion defines in:
  util/intel-pt-decoder/intel-pt-pkt-decoder.c,

please remove them as well

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/1] perf tools: Fix build break on powerpc

[Jiri Olsa]

On Sat, Mar 26, 2016 at 11:01:47AM -0700, Sukadev Bhattiprolu wrote:
> From 502e8236082412db1d33abfad95aaf14b539502e Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu 
> Date: Sat, 26 Mar 2016 17:31:39 -0400
> Subject: [PATCH 1/1] perf tools: Fix build break on powerpc
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 'Commit 531d2410635c ("perf tools: Do not include stringify.h from the
> kernel sources")' seems to have accidentially removed the inclusion of
> "util/header.h" from "arch/powerpc/util/header.c".
> 
> "util/header.h" provides the prototype for get_cpuid() and is needed to
> build perf on Powerpc.
> 
>   arch/powerpc/util/header.c:17:1: error: no previous prototype for
>   ‘get_cpuid’ [-Werror=missing-prototypes]
> 
> Reported-by: Michael Ellerman 
> Signed-off-by: Sukadev Bhattiprolu 
> ---
>  tools/perf/arch/powerpc/util/header.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/arch/powerpc/util/header.c 
> b/tools/perf/arch/powerpc/util/header.c
> index 6138bde..5111e34 100644
> --- a/tools/perf/arch/powerpc/util/header.c
> +++ b/tools/perf/arch/powerpc/util/header.c
> @@ -4,6 +4,7 @@
>  #include 
>  #include 
>  #include 
> +#include "../../util/header.h"

you could use just "header.h" right?

jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V8 3/4] tools/perf: Map the ID values with register names

[Jiri Olsa]

On Fri, Jan 08, 2016 at 02:32:30PM +0530, Anju T wrote:

SNIP

> +#endif /* ARCH_PERF_REGS_H */
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index 38a0853..acf0302 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -23,6 +23,11 @@ $(call detected_var,ARCH)
>  
>  NO_PERF_REGS := 1
>  
> +# Additional ARCH settings for ppc64
> +ifeq ($(ARCH),powerpc)
> + NO_PERF_REGS := 0
> +endif

please use space for indent in makefiles.. the line might get
switched for rule one day and it's hard to find ;-)

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v4] perf: bpf: Fix build breakage due to libbpf

[Jiri Olsa]

On Thu, Jan 07, 2016 at 06:23:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 07, 2016 at 05:39:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jan 07, 2016 at 12:28:15PM +0530, Naveen N. Rao escreveu:
> > > Can you please push at least the initial 3 patches of this for v4.4?  
> > > Wang Nan has posted v6 here:
> > > http://thread.gmane.org/gmane.linux.kernel/2110626
>  
> > So I tried it again today, from the v6, the latest patch with that
> > subject line in my inbox, and I get this, after applying the first three
> > patches:
>  
> > [acme@zoo linux]$ make -C tools clean > /dev/null
> > [acme@zoo linux]$ make -C tools/perf build-test
> > make: Entering directory '/home/git/linux/tools/perf'
> > Testing Makefile
> > tests/make:15: /scripts/Makefile.arch: No such file or directory
> > make[2]: *** No rule to make target '/scripts/Makefile.arch'.  Stop.
> > tests/make:5: recipe for target 'all' failed
> > make[1]: *** [all] Error 2
> > Makefile:81: recipe for target 'build-test' failed
> > make: *** [build-test] Error 2
> > make: Leaving directory '/home/git/linux/tools/perf'
> > [acme@zoo linux]$
> 
> > Trying to figure this out...
> 
> So, this doesn't seem to have been tessted, this part, specifically:
> 
> diff --git a/tools/perf/tests/make b/tools/perf/tests/make
> index 8ea3dffc5065..cd9c3ce1a5c2 100644
> --- a/tools/perf/tests/make
> +++ b/tools/perf/tests/make
> @@ -12,7 +12,7 @@ endif
>  else
>  PERF := .
>  
> -include config/Makefile.arch
> +include $(srctree)/scripts/Makefile.arch
>  
>  # FIXME looks like x86 is the only arch running tests ;-)
>  # we need some IS_(32/64) flag to make this generic
> 
> 
> 
> 
> Because $(srctree) is not set at that point, I copied the needed bits from
> tools/perf/Makefile.perf and got the patch below, which makes:L
> 
>   make -C tools/perf build-test
> 
> Work for me again.
> 
> This should wrap up the day nicely, making me think of Jens Axboe recent
> cool reaction to an untested patch:
> 
> http://lkml.kernel.org/r/5661c2bc.9030...@kernel.dk
> 
> :-)
> 
> Jiri, are you ok with the patch below, on top of the second patch in
> Wang's series, that you acked?

yep, looks good to me

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V7 3/4] tools/perf: Map the ID values with register names

[Jiri Olsa]

On Wed, Jan 06, 2016 at 03:15:50PM +0530, Anju T wrote:

SNIP

> + [PERF_REG_POWERPC_GPR28] = "gpr28",
> + [PERF_REG_POWERPC_GPR29] = "gpr29",
> + [PERF_REG_POWERPC_GPR30] = "gpr30",
> + [PERF_REG_POWERPC_GPR31] = "gpr31",
> + [PERF_REG_POWERPC_NIP] = "nip",
> + [PERF_REG_POWERPC_MSR] = "msr",
> + [PERF_REG_POWERPC_ORIG_R3] = "orig_r3",
> + [PERF_REG_POWERPC_CTR] = "ctr",
> + [PERF_REG_POWERPC_LNK] = "lnk",
> + [PERF_REG_POWERPC_XER] = "xer",
> + [PERF_REG_POWERPC_CCR] = "ccr",
> + [PERF_REG_POWERPC_TRAP] = "trap",
> + [PERF_REG_POWERPC_DAR] = "dar",
> + [PERF_REG_POWERPC_DSISR] = "dsisr"
> +};
> +
> +static inline const char *perf_reg_name(int id)
> +{
> +

extra line ^^^ ;-)

> + return reg_names[id];
> +}
> +#endif /* ARCH_PERF_REGS_H */
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile

jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 0/3] perf build: PowerPC: Fix build breakage due to libbpf

[Jiri Olsa]

On Tue, Dec 15, 2015 at 03:35:54AM +, Wang Nan wrote:
> Hi Naveen,
> 
>Now I know your problem is in native building and the reason is
> missing proper $(ARCH). I think other than that there's another problem
> in libbpf's building: if your problem is unable to compile libbpf,
> feature checker should find it and set NO_LIBBPF=1 for perf, so perf
> building won't be blocked, only BPF related features should be turned
> off. Also, I think including Makefile.arch from tools/scripts would be
> better, because libbpf is at the same level with perf. So I separate
> them into 3 patches. Please help me test it on PowerPC.
> 
> Cc: Arnaldo Carvalho de Melo <a...@kernel.org>
> Cc: Jiri Olsa <jo...@redhat.com>
> Cc: Naveen N. Rao <naveen.n@linux.vnet.ibm.com>
> Cc: Sukadev Bhattiprolu <suka...@linux.vnet.ibm.com>
> 
> Naveen N. Rao (1):
>   perf: bpf: Fix build breakage due to libbpf
> 
> Wang Nan (2):
>   perf tools: Fix PowerPC native building
>   tools: Move Makefile.arch from perf/config to tools/scripts

for the patchset:

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] perf: Fix build break on powerpc due to sample_reg_masks

[Jiri Olsa]

On Wed, Sep 30, 2015 at 11:28:36AM -0700, Sukadev Bhattiprolu wrote:
> From e29a7236122c4d807ec9ebc721b5d7d75c8d Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <suka...@linux.vnet.ibm.com>
> Date: Thu, 24 Sep 2015 17:53:49 -0400
> Subject: [PATCH v2] perf: Fix build break on powerpc due to sample_reg_masks
> 
> perf_regs.c does not get built on Powerpc as CONFIG_PERF_REGS is false.
> So the weak definition for 'sample_regs_masks' doesn't get picked up.
> 
> Adding perf_regs.o to util/Build unconditionally, exposes a redefinition
> error for 'perf_reg_value()' function (due to the static inline version
> in util/perf_regs.h). So use #ifdef HAVE_PERF_REGS_SUPPORT' around that
> function.
> 
> Signed-off-by: Sukadev Bhattiprolu <suka...@linux.vnet.ibm.com>

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

On Tue, Sep 29, 2015 at 11:06:17AM +0530, Naveen N. Rao wrote:
> On 2015/09/24 10:15PM, Naveen N Rao wrote:
> > On 2015/09/24 08:32AM, Stephane Eranian wrote:
> > > On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa <jo...@redhat.com> wrote:
> > > >
> > > > On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote:
> > > > > perf build currently fails on powerpc:
> > > > >
> > > > >   LINK perf
> > > > > libperf.a(libperf-in.o):(.toc+0x120): undefined reference to
> > > > > `sample_reg_masks'
> > > > > libperf.a(libperf-in.o):(.toc+0x130): undefined reference to
> > > > > `sample_reg_masks'
> > > > > collect2: error: ld returned 1 exit status
> > > > > make[1]: *** [perf] Error 1
> > > > > make: *** [all] Error 2
> > > > >
> > > > > This is due to parse-regs-options.c using sample_reg_masks, which is
> > > > > defined only with CONFIG_PERF_REGS.
> > > > >
> > > > > In addition, perf record -I is only useful if the arch supports
> > > > > PERF_REGS. Hence, let's expose -I conditionally.
> > > > >
> > > > > Signed-off-by: Naveen N. Rao <naveen.n@linux.vnet.ibm.com>
> > > >
> > > > hum, I wonder why we have sample_reg_masks defined as weak in 
> > > > util/perf_regs.c
> > > > which is also built only via CONFIG_PERF_REGS
> > > >
> > > > I wonder we could get rid of the weak definition via attached patch, 
> > > > Stephane?
> > > >
> > > But the whole point of having it weak is to avoid this error scenario
> > > on any arch without support
> > > and avoid ugly #ifdef HAVE_ in generic files.
> > > 
> > > if perf_regs.c is compiled on PPC, then why do we get the undefined?
> > 
> > As Jiri Olsa pointed out, powerpc and many other architectures don't 
> > (yet) have support for perf regs.
> > 
> > But, the larger reason to introduce #ifdef is so the user doesn't see 
> > options (s)he can't use on a specific architecture, along the same lines 
> > as builtin-probe.c
> 
> Stephane, Arnaldo,
> Suka has also posted a fix for this with a different approach [1]. Can 
> you please ack/pull one of these versions? Building perf is broken on 
> v4.3-rc due to this.

I did not get any answer for additional comments I made to the patch
(couldnt get marc.info working, sending the patch again)

> 
> [1] http://article.gmane.org/gmane.linux.kernel/2046370

I dont have this last version, which seems to have other changes
and patch in above link looks mangled, could you please repost it?

thanks,
jirka


---
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 142eeb341b29..19c8fd22fbe3 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1082,9 +1082,11 @@ struct option __record_options[] = {
"sample transaction flags (special events only)"),
OPT_BOOLEAN(0, "per-thread", _thread,
"use per-thread mmaps"),
+#ifdef HAVE_PERF_REGS_SUPPORT
OPT_CALLBACK_OPTARG('I', "intr-regs", _intr_regs, 
NULL, "any register",
"sample selected machine registers on interrupt,"
" use -I ? to list register names", parse_regs),
+#endif
OPT_BOOLEAN(0, "running-time", _time,
"Record running/enabled time of read (:S) events"),
OPT_CALLBACK('k', "clockid", ,
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 4bc7a9ab45b1..93c6371405a3 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -104,7 +104,7 @@ libperf-$(CONFIG_LIBBABELTRACE) += data-convert-bt.o
 
 libperf-y += scripting-engines/
 
-libperf-$(CONFIG_PERF_REGS) += perf_regs.o
+libperf-y += perf_regs.o
 libperf-$(CONFIG_ZLIB) += zlib.o
 libperf-$(CONFIG_LZMA) += lzma.o
 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote:

SNIP

> > > Suka has also posted a fix for this with a different approach [1]. Can 
> > > you please ack/pull one of these versions? Building perf is broken on 
> > > v4.3-rc due to this.
> > 
> > I did not get any answer for additional comments I made to the patch
> > (couldnt get marc.info working, sending the patch again)
> 
> Hi Jiri,
> I concur with the changes you proposed to my patch here (getting rid of 
> the weak variant):
> http://article.gmane.org/gmane.linux.kernel/2046108
> 
> I am aware of the other approach you posted (and the one attached 
> below). When I said "please ack/pull one of these versions", I meant one 
> of: your version, Suka's and mine.

I was hoping somebody could test it on ppc ;-)

I think the last version (in my last email) that keeps the weak
variable is correct, let's wait for Arnaldo to sort this out

> 
> > 
> > > 
> > > [1] http://article.gmane.org/gmane.linux.kernel/2046370
> > 
> > I dont have this last version, which seems to have other changes
> > and patch in above link looks mangled, could you please repost it?
> 
> Can you please check the raw version:
> http://article.gmane.org/gmane.linux.kernel/2046370/raw

we have __maybe_unused definition in tools/include/linux/compiler.h
why to redeclare it?

jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

On Tue, Sep 29, 2015 at 11:10:02AM -0700, Sukadev Bhattiprolu wrote:

SNIP

>  
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index 885e8ac..6b8eb13 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -6,6 +6,7 @@ const struct sample_reg __weak sample_reg_masks[] = {
>   SMPL_REG_END
>  };
>  
> +#ifdef HAVE_PERF_REGS_SUPPORT
>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
>  {
>   int i, idx = 0;
> @@ -29,3 +30,4 @@ out:
>   *valp = regs->cache_regs[id];
>   return 0;
>  }
> +#endif
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 2984dcc..8dbdfeb 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -3,6 +3,10 @@
>  
>  #include 
>  
> +#ifndef __maybe_unused
> +#define __maybe_unused __attribute__((unused))
> +#endif
> +

would the linux/compiler.h include do instead?

otherwise I'd be ok with this

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

On Tue, Sep 29, 2015 at 10:01:36PM +0530, Naveen N. Rao wrote:
> On 2015/09/29 12:47PM, Jiri Olsa wrote:
> > On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote:
> > 
> > SNIP
> > 
> > > > > Suka has also posted a fix for this with a different approach [1]. 
> > > > > Can 
> > > > > you please ack/pull one of these versions? Building perf is broken on 
> > > > > v4.3-rc due to this.
> > > > 
> > > > I did not get any answer for additional comments I made to the patch
> > > > (couldnt get marc.info working, sending the patch again)
> > > 
> > > Hi Jiri,
> > > I concur with the changes you proposed to my patch here (getting rid of 
> > > the weak variant):
> > > http://article.gmane.org/gmane.linux.kernel/2046108
> > > 
> > > I am aware of the other approach you posted (and the one attached 
> > > below). When I said "please ack/pull one of these versions", I meant one 
> > > of: your version, Suka's and mine.
> > 
> > I was hoping somebody could test it on ppc ;-)
> > 
> > I think the last version (in my last email) that keeps the weak
> > variable is correct, let's wait for Arnaldo to sort this out
> 
> I just tried it, but it fails. As Suka points out in his patch:
> "Adding perf_regs.o to util/Build unconditionally, exposes a 
> redefinition error for 'perf_reg_value()' function (due to the static 
> inline version in util/perf_regs.h). So use #ifdef 
> HAVE_PERF_REGS_SUPPORT' around that function."

could you (or Suka) please reply in here with the patch?

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v16 00/16] perf, tools: Add support for PMU events in JSON format

[Jiri Olsa]

On Thu, Sep 10, 2015 at 11:06:55AM -0700, Sukadev Bhattiprolu wrote:
> Sukadev Bhattiprolu [suka...@linux.vnet.ibm.com] wrote:
> | Arnaldo, Ingo,
> | 
> | I added Andi's patch https://lkml.org/lkml/2015/8/28/521 to 'json-v16' 
> branch.
> | and created a new branch, 'json-files-7' with updated to Intel data files.
> | Here are the three new branches.
> | 
> | https://github.com:sukadev/linux.git 
> | 
> | Branch  Description
> | --
> | json-v16Source Code only 
> | json-files-7x86 and Powerpc datafiles only
> | json-v16.1-with-dataBoth code and data (for build/test)
> 
> Arnaldo, Ingo,
> 
> I have not received any comments on this patch set since it was Acked by
> Jiri Olsa in June. Can you please queue this up for 4.3?

hi,
any news about this one?

thanks,
jirka
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev