Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752047AbbGWGdN (ORCPT ); Thu, 23 Jul 2015 02:33:13 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:54412 "EHLO e23smtp09.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750842AbbGWGdC (ORCPT ); Thu, 23 Jul 2015 02:33:02 -0400 X-Helo: d23dlp03.au.ibm.com X-MailFrom: maddy@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Message-ID: <55B08A65.8070906@linux.vnet.ibm.com> Date: Thu, 23 Jul 2015 12:02:05 +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: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Stephane Eranian , Paul Mackerras , Anton Blanchard , Sukadev Bhattiprolu , Anshuman Khandual Subject: Re: [PATCH v5 5/7] powerpc/powernv: add event attribute and group to nest pmu References: <1437045206-7491-1-git-send-email-maddy@linux.vnet.ibm.com> <1437045206-7491-6-git-send-email-maddy@linux.vnet.ibm.com> <1437540294.30906.30.camel@axtens.net> In-Reply-To: <1437540294.30906.30.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: 15072306-0033-0000-0000-000001D52353 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4406 Lines: 121 On Wednesday 22 July 2015 10:14 AM, Daniel Axtens wrote: > On Thu, 2015-07-16 at 16:43 +0530, Madhavan Srinivasan wrote: >> Add code to create event/format attributes and attribute groups for >> each nest pmu. >> >> Cc: Michael Ellerman >> Cc: Benjamin Herrenschmidt >> Cc: Paul Mackerras >> Cc: Anton Blanchard >> Cc: Sukadev Bhattiprolu >> Cc: Anshuman Khandual >> Cc: Stephane Eranian >> Signed-off-by: Madhavan Srinivasan >> --- >> arch/powerpc/perf/nest-pmu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 65 insertions(+) >> >> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c >> index c4c08e4dee55..f3418bdec1cd 100644 >> --- a/arch/powerpc/perf/nest-pmu.c >> +++ b/arch/powerpc/perf/nest-pmu.c >> @@ -13,6 +13,17 @@ >> static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS]; >> static struct nest_pmu *per_nest_pmu_arr[P8_NEST_MAX_PMUS]; >> >> +PMU_FORMAT_ATTR(event, "config:0-20"); >> +static struct attribute *p8_nest_format_attrs[] = { >> + &format_attr_event.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group p8_nest_format_group = { >> + .name = "format", >> + .attrs = p8_nest_format_attrs, >> +}; >> + >> static int nest_event_info(struct property *pp, char *name, >> struct nest_ima_events *p8_events, int string, u32 val) >> { >> @@ -46,6 +57,56 @@ static int nest_event_info(struct property *pp, char *name, >> return 0; >> } >> >> +/* >> + * Populate event name and string in attribute >> + */ >> +static struct attribute *dev_str_attr(const char *name, const char *str) >> +{ >> + struct perf_pmu_events_attr *attr; >> + >> + attr = kzalloc(sizeof(*attr), GFP_KERNEL); >> + >> + sysfs_attr_init(&attr->attr.attr); >> + >> + attr->event_str = str; >> + attr->attr.attr.name = name; >> + attr->attr.attr.mode = 0444; >> + attr->attr.show = perf_event_sysfs_show; >> + >> + return &attr->attr.attr; > So I asked you about this before, and you pointed me to > perf_event_sysfs_show. Looking at that in kernel/events/core.c, it looks > like that uses container_of to pull out the perf_pmu_events_attr. So I > guess that is at least mostly correct. > > I'm hoping something else uses container_of to pull out attr->attr, so > that they can actually grab the attr->attr.show function pointer, so > that perf_event_sysfs_show actually gets called. Where would that be? OK, what we return is the device attribute struct which also have sysfs_ops. So ->show and ->store are those entries in the strucutre and here we only populate show ops using perf_event_sysfs_show. Now at the time of pmu registering, we end up calling device_add->device_create_file-> sysfs_create_file which will end up adding a sysfs device file linked to this ->show ops. >> +} >> + >> +static int update_events_in_group( >> + struct nest_ima_events *p8_events, int idx, struct nest_pmu *pmu) >> +{ >> + struct attribute_group *attr_group; >> + struct attribute **attrs; >> + int i; >> + >> + /* >> + * Allocate memory for both event attribute group and for >> + * event attributes array. >> + */ >> + attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) + >> + sizeof(*attr_group)), GFP_KERNEL); >> + if (!attr_group) >> + return -ENOMEM; >> + >> + /* >> + * Assign memory for event attribute array >> + */ >> + attrs = (struct attribute **)(attr_group + 1); >> + attr_group->name = "events"; >> + attr_group->attrs = attrs; > I am super uncomfortable with this block, especially the assignment to > attrs. I *think* you're trying to allocate an attribute group and a set > of attributes, but you've combined the allocation into one big > contiguous chunk, and then you're trying to tease them apart. Is that > necessary? Could it be two allocs, one for the attribute_group and one > for the attribute? I wanted to avoid two function calls here, but this is not a hot path This happens at the pmu init time (booting), so I guess we can have two allocs here. > -- 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/