From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752972AbcF2PU7 (ORCPT ); Wed, 29 Jun 2016 11:20:59 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:43031 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752351AbcF2PU4 (ORCPT ); Wed, 29 Jun 2016 11:20:56 -0400 X-IBM-Helo: d03dlp03.boulder.ibm.com X-IBM-MailFrom: ravi.bangoria@linux.vnet.ibm.com Subject: Re: [PATCH v2 3/4] perf annotate: add powerpc support To: "Naveen N. Rao" References: <1467198958-9195-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com> <1467198958-9195-4-git-send-email-ravi.bangoria@linux.vnet.ibm.com> <20160629144521.GC14508@naverao1-tp.localdomain> Cc: linux-kernel@vger.kernel.org, acme@kernel.org, linuxppc-dev@lists.ozlabs.org, anton@ozlabs.org, mpe@ellerman.id.au, ananth@in.ibm.com, dja@axtens.net, David.Laight@ACULAB.COM From: Ravi Bangoria Date: Wed, 29 Jun 2016 20:49:49 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160629144521.GC14508@naverao1-tp.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16062915-0004-0000-0000-00000FCD2AEB X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16062915-0005-0000-0000-000076A09C4A Message-Id: <5773E715.1000203@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-06-29_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1606290143 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks Naveen, On Wednesday 29 June 2016 08:15 PM, Naveen N. Rao wrote: > On 2016/06/29 04:45PM, Ravi Bangoria wrote: >> From: Naveen N. Rao >> >> Powerpc has long list of branch instructions and hardcoding them in >> table appears to be error-prone. So, add new function to find >> instruction instead of creating table. This function dynamically >> create table(list of 'struct ins'), and instead of creating object >> every time, first check if list already contain object for that >> nemonics. >> >> Signed-off-by: Naveen N. Rao >> Signed-off-by: Ravi Bangoria >> --- >> Changes in v2: >> - Corrected few memory leaks. >> - Created Dynamic list for powerpc to optimize memory consumption >> >> tools/perf/util/annotate.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 121 insertions(+) >> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c >> index 36a5825..812bfad 100644 >> --- a/tools/perf/util/annotate.c >> +++ b/tools/perf/util/annotate.c >> @@ -461,6 +461,11 @@ static struct ins instructions_arm[] = { >> { .name = "bne", .ops = &jump_ops, }, >> }; >> >> +struct instructions_powerpc { >> + struct ins *ins; >> + struct list_head list; >> +}; >> + >> static int ins__key_cmp(const void *name, const void *insp) >> { >> const struct ins *ins = insp; >> @@ -476,6 +481,120 @@ static int ins__cmp(const void *a, const void *b) >> return strcmp(ia->name, ib->name); >> } >> >> +static int list_add__ins_powerpc(struct instructions_powerpc *head, >> + struct ins *ins) >> +{ >> + struct instructions_powerpc *ins_powerpc; >> + >> + ins_powerpc = zalloc(sizeof(struct instructions_powerpc)); >> + if (!ins_powerpc) >> + return -1; >> + >> + ins_powerpc->ins = ins; >> + list_add_tail(&(ins_powerpc->list), &(head->list)); >> + >> + return 0; >> +} >> + >> +static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head, >> + const char *name) >> +{ >> + struct instructions_powerpc *pos; >> + >> + list_for_each_entry(pos, &head->list, list) { >> + if (!strcmp(pos->ins->name, name)) >> + return pos->ins; >> + } >> + return NULL; >> +} >> + >> +static struct ins *ins__find_powerpc(const char *name) >> +{ >> + int i; >> + struct ins *ins; >> + static struct instructions_powerpc head; >> + static bool list_initialized; >> + >> + if (!list_initialized) { >> + INIT_LIST_HEAD(&head.list); >> + list_initialized = true; >> + } >> + >> + /* >> + * Search if we already created object of 'struct ins' >> + * for this instruction >> + */ >> + ins = list_search__ins_powerpc(&head, name); >> + if (ins) >> + return ins; >> + >> + ins = zalloc(sizeof(struct ins)); >> + if (!ins) >> + return NULL; >> + >> + ins->name = strdup(name); >> + if (!ins->name) >> + goto err; > You can move the above two inside the below if condition, so that you > only allocate memory if needed. > > Or, what would be better would be to pass 'name' and the appropriate ops > pointer to the helper above (list_add__ins_powerpc) and have that > allocate 'struct ins' and insert into the list. Yes I will think about this. >> + >> + if (name[0] == 'b') { >> + /* branch instructions */ >> + ins->ops = &jump_ops; >> + >> + /* >> + * - Few start with 'b', but aren't branch instructions. >> + * - Let's also ignore instructions involving 'ctr' and >> + * 'tar' since target branch addresses for those can't >> + * be determined statically. >> + */ >> + if (!strncmp(name, "bcd", 3) || >> + !strncmp(name, "brinc", 5) || >> + !strncmp(name, "bper", 4) || >> + strstr(name, "ctr") || >> + strstr(name, "tar")) >> + goto err; > You are still leaking ins->name here. Ah!! Sorry. I missed that we are using strdup here. Will correct it. -Ravi