Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933066AbbFILoW (ORCPT ); Tue, 9 Jun 2015 07:44:22 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:34531 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932628AbbFILnh (ORCPT ); Tue, 9 Jun 2015 07:43:37 -0400 Message-ID: <5576D0D8.8070304@linux.vnet.ibm.com> Date: Tue, 09 Jun 2015 17:11:12 +0530 From: Madhavan Srinivasan User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Daniel Axtens CC: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Stephane Eranian , Paul Mackerras , Sukadev Bhattiprolu , Anshuman Khandual Subject: Re: [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU registration References: <1433260778-26497-1-git-send-email-maddy@linux.vnet.ibm.com> <1433260778-26497-8-git-send-email-maddy@linux.vnet.ibm.com> <1433293592.438.74.camel@axtens.net> In-Reply-To: <1433293592.438.74.camel@axtens.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15060911-1618-0000-0000-0000023A6A75 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4407 Lines: 130 On Wednesday 03 June 2015 06:36 AM, Daniel Axtens wrote: > On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: >> Patch adds common event attribute function and Nest pmu registration call. >> >> Cc: Michael Ellerman >> Cc: Benjamin Herrenschmidt >> Cc: Paul Mackerras >> Cc: Sukadev Bhattiprolu >> Cc: Anshuman Khandual >> Cc: Stephane Eranian >> Signed-off-by: Madhavan Srinivasan >> --- >> arch/powerpc/perf/nest-pmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 52 insertions(+) >> >> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c >> index 514a0be..dd84fd7 100644 >> --- a/arch/powerpc/perf/nest-pmu.c >> +++ b/arch/powerpc/perf/nest-pmu.c >> @@ -244,6 +244,49 @@ static int update_pmu_ops(struct nest_pmu *pmu) >> return 0; >> } >> >> +/* >> + * Populate event name and string in attribute >> + */ >> +struct attribute *dev_str_attr(char *name, char *str) >> +{ >> + struct perf_pmu_events_attr *attr; >> + >> + attr = kzalloc(sizeof(*attr), GFP_KERNEL); >> + >> + attr->event_str = (const char *)str; > Erk. Two things: > - Is str const or not? If you're treating it as const here, should you > pass that through the function signature? > - Who is responsible for the memory behind it? It looks like a caller > can't construct str dynamically, pass it to this function and then free > it, because that will invalidate attr->event_str. Is this documented? Yes. Valid point. str should be and it is const. My bad, will fix the function signature. >> + attr->attr.attr.name = name; >> + attr->attr.attr.mode = 0444; >> + attr->attr.show = perf_event_sysfs_show; >> + >> + return &attr->attr.attr; > If you're returning the address of attr->attr.attr, then: > - why don't you just deal directly with struct attribute * in the > function? Why an entire struct perf_pmu_events_attr *? > - with the function as written, if you return just &attr->attr.attr, > don't attr->event_str and attr->attr.show get lost? Kindly have should look at perf_event_sysfs_show function in include/linux/perf_event.h. Even though we return only &attr->attr.attr, we are not freeing the memory of perf_pmu_event_attr, hence will not be lost :) . >> +} >> + >> +int update_events_in_group( >> + struct ppc64_nest_ima_events *p8_events, int idx, >> + struct nest_pmu *pmu) >> +{ >> + struct attribute_group *attr_group; >> + struct attribute **attrs; >> + int i; >> + >> + attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) + >> + sizeof(*attr_group)), GFP_KERNEL); >> + if (!attr_group) >> + return -ENOMEM; >> + >> + attrs = (struct attribute **)(attr_group + 1); >> + attr_group->name = "events"; >> + attr_group->attrs = attrs; >> + >> + for (i=0; i< idx; i++, p8_events++) >> + attrs[i] = dev_str_attr((char *)p8_events->ev_name, >> + (char *)p8_events->ev_value); >> + >> + pmu->attr_groups[0] = attr_group; >> + return 0; >> +} > I'm very confused by what this function is trying to do. Could you add > some comments? I'm particularly confused by the relationship between > attrs and attr_group. This function mainly creates a "event" attribute group for this PMU. It does so with the list of event files parsed from the device tree for this pmu in the nest_pmu_create function. WIll add comments in the next version. >> + >> + >> static int nest_pmu_create(struct device_node *dev, int pmu_index) >> { >> struct ppc64_nest_ima_events **p8_events_arr; >> @@ -364,6 +407,15 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index) >> } >> } >> >> + update_events_in_group( >> + (struct ppc64_nest_ima_events *)p8_events_arr, >> + idx, pmu_ptr); >> + update_pmu_ops(pmu_ptr); >> + >> + /* Register the pmu */ >> + perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1); >> + printk(KERN_INFO "Nest PMU %s Registered\n", pmu_ptr->pmu.name); >> + >> return 0; >> } >> > Regards, > Daniel > Apologizes on late response to this mail. Missed it. Thanks for review Maddy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/