lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <ABD8FBC0-149E-4C85-BC4A-244347D63A2B@linux.vnet.ibm.com>
Date: Sat, 13 Jul 2024 11:31:48 +0530
From: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ian Rogers <irogers@...gle.com>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>, akanksha@...ux.ibm.com,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Disha Goel <disgoel@...ux.vnet.ibm.com>
Subject: Re: [PATCH V6 18/18] tools/perf: Set instruction name to be used with
 insn-stat when using raw instruction



> On 13 Jul 2024, at 2:57 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> 
> On Sun, Jul 07, 2024 at 08:14:19PM +0530, Athira Rajeev wrote:
>> Since the "ins.name" is not set while using raw instruction,
>> perf annotate with insn-stat gives wrong data:
>> 
>> Result from "./perf annotate --data-type --insn-stat":
>> 
>> Annotate Instruction stats
>> total 615, ok 419 (68.1%), bad 196 (31.9%)
>> 
>>  Name      :  Good   Bad
>> -----------------------------------------------------------
>>            :   419   196
>> 
>> Patch sets "dl->ins.name" in arch specific function "check_ppc_insn"
>> while initialising "struct disasm_line". Also update "ins_find" function
>> to pass "struct disasm_line" as a parameter so as to set its name field
>> in arch specific call.
>> 
>> With the patch changes:
>> 
>> Annotate Instruction stats
>> total 609, ok 446 (73.2%), bad 163 (26.8%)
>> 
>>  Name/opcode:  Good   Bad
>> -----------------------------------------------------------
>>  58                  :   323    80
>>  32                  :    49    43
>>  34                  :    33    11
>>  OP_31_XOP_LDX       :     8    20
>>  40                  :    23     0
>>  OP_31_XOP_LWARX     :     5     1
>>  OP_31_XOP_LWZX      :     2     3
>>  OP_31_XOP_LDARX     :     3     0
>>  33                  :     0     2
>>  OP_31_XOP_LBZX      :     0     1
>>  OP_31_XOP_LWAX      :     0     1
>>  OP_31_XOP_LHZX      :     0     1
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
>> ---
>> .../perf/arch/powerpc/annotate/instructions.c  | 18 +++++++++++++++---
>> tools/perf/builtin-annotate.c                  |  4 ++--
>> tools/perf/util/annotate.c                     |  2 +-
>> tools/perf/util/disasm.c                       | 10 +++++-----
>> tools/perf/util/disasm.h                       |  2 +-
>> 5 files changed, 24 insertions(+), 12 deletions(-)
>> 
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c b/tools/perf/arch/powerpc/annotate/instructions.c
>> index af1032572bf3..ede9eeade0ab 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -189,8 +189,9 @@ static int cmp_offset(const void *a, const void *b)
>> return (val1->value - val2->value);
>> }
>> 
>> -static struct ins_ops *check_ppc_insn(u32 raw_insn)
>> +static struct ins_ops *check_ppc_insn(struct disasm_line *dl)
>> {
>> + int raw_insn = dl->raw.raw_insn;
>> int opcode = PPC_OP(raw_insn);
>> int mem_insn_31 = PPC_21_30(raw_insn);
>> struct insn_offset *ret;
>> @@ -198,19 +199,30 @@ static struct ins_ops *check_ppc_insn(u32 raw_insn)
>> "OP_31_INSN",
>> mem_insn_31
>> };
>> + char name_insn[32];
>> 
>> /*
>>  * Instructions with opcode 32 to 63 are memory
>>  * instructions in powerpc
>>  */
>> if ((opcode & 0x20)) {
>> + /*
>> +  * Set name in case of raw instruction to
>> +  * opcode to be used in insn-stat
>> +  */
>> + if (!strlen(dl->ins.name)) {
>> + sprintf(name_insn, "%d", opcode);
>> + dl->ins.name = strdup(name_insn);
>> + }
>> return &load_store_ops;
>> } else if (opcode == 31) {
>> /* Check for memory instructions with opcode 31 */
>> ret = bsearch(&mem_insns_31_opcode, ins_array, ARRAY_SIZE(ins_array), sizeof(ins_array[0]), cmp_offset);
>> - if (ret != NULL)
>> + if (ret) {
>> + if (!strlen(dl->ins.name))
>> + dl->ins.name = strdup(ret->name);
>> return &load_store_ops;
>> - else {
>> + } else {
>> mem_insns_31_opcode.value = PPC_22_30(raw_insn);
>> ret = bsearch(&mem_insns_31_opcode, arithmetic_ins_op_31, ARRAY_SIZE(arithmetic_ins_op_31),
>> sizeof(arithmetic_ins_op_31[0]), cmp_offset);
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index b10b7f005658..68e929d4746e 100644
>> --- a/tools/perf/builtin-annotate.c
>> +++ b/tools/perf/builtin-annotate.c
>> @@ -396,10 +396,10 @@ static void print_annotate_item_stat(struct list_head *head, const char *title)
>> printf("total %d, ok %d (%.1f%%), bad %d (%.1f%%)\n\n", total,
>>        total_good, 100.0 * total_good / (total ?: 1),
>>        total_bad, 100.0 * total_bad / (total ?: 1));
>> - printf("  %-10s: %5s %5s\n", "Name", "Good", "Bad");
>> + printf("  %-10s: %5s %5s\n", "Name/opcode", "Good", "Bad");
> 
> It should be "%-20s".
> 
> Thanks,
> Namhyung

Sure, will make this change

Thanks
Athira
> 
> 
>> printf("-----------------------------------------------------------\n");
>> list_for_each_entry(istat, head, list)
>> - printf("  %-10s: %5d %5d\n", istat->name, istat->good, istat->bad);
>> + printf("  %-20s: %5d %5d\n", istat->name, istat->good, istat->bad);
>> printf("\n");
>> }
>> 
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 8db2f32700aa..e1f24dff8042 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2229,7 +2229,7 @@ static struct annotated_item_stat *annotate_data_stat(struct list_head *head,
>> return NULL;
>> 
>> istat->name = strdup(name);
>> - if (istat->name == NULL) {
>> + if ((istat->name == NULL) || (!strlen(istat->name))) {
>> free(istat);
>> return NULL;
>> }
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index 63681df6482b..cd283c42195c 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -859,7 +859,7 @@ static void ins__sort(struct arch *arch)
>> qsort(arch->instructions, nmemb, sizeof(struct ins), ins__cmp);
>> }
>> 
>> -static struct ins_ops *__ins__find(struct arch *arch, const char *name, u32 raw_insn)
>> +static struct ins_ops *__ins__find(struct arch *arch, const char *name, struct disasm_line *dl)
>> {
>> struct ins *ins;
>> const int nmemb = arch->nr_instructions;
>> @@ -871,7 +871,7 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name, u32 raw_
>>  */
>> struct ins_ops *ops;
>> 
>> - ops = check_ppc_insn(raw_insn);
>> + ops = check_ppc_insn(dl);
>> if (ops)
>> return ops;
>> }
>> @@ -905,9 +905,9 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name, u32 raw_
>> return ins ? ins->ops : NULL;
>> }
>> 
>> -struct ins_ops *ins__find(struct arch *arch, const char *name, u32 raw_insn)
>> +struct ins_ops *ins__find(struct arch *arch, const char *name, struct disasm_line *dl)
>> {
>> - struct ins_ops *ops = __ins__find(arch, name, raw_insn);
>> + struct ins_ops *ops = __ins__find(arch, name, dl);
>> 
>> if (!ops && arch->associate_instruction_ops)
>> ops = arch->associate_instruction_ops(arch, name);
>> @@ -917,7 +917,7 @@ struct ins_ops *ins__find(struct arch *arch, const char *name, u32 raw_insn)
>> 
>> static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map_symbol *ms)
>> {
>> - dl->ins.ops = ins__find(arch, dl->ins.name, dl->raw.raw_insn);
>> + dl->ins.ops = ins__find(arch, dl->ins.name, dl);
>> 
>> if (!dl->ins.ops)
>> return;
>> diff --git a/tools/perf/util/disasm.h b/tools/perf/util/disasm.h
>> index c1bb1e484bfb..f56beedeb9da 100644
>> --- a/tools/perf/util/disasm.h
>> +++ b/tools/perf/util/disasm.h
>> @@ -105,7 +105,7 @@ struct annotate_args {
>> struct arch *arch__find(const char *name);
>> bool arch__is(struct arch *arch, const char *name);
>> 
>> -struct ins_ops *ins__find(struct arch *arch, const char *name, u32 raw_insn);
>> +struct ins_ops *ins__find(struct arch *arch, const char *name, struct disasm_line *dl);
>> int ins__scnprintf(struct ins *ins, char *bf, size_t size,
>>    struct ins_operands *ops, int max_ins_name);
>> 
>> -- 
>> 2.43.0



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ