lkml.org 
[lkml]   [2015]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU registration


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 <mpe@ellerman.id.au>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> Cc: Stephane Eranian <eranian@google.com>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> 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



\
 
 \ /
  Last update: 2015-06-09 14:01    [W:0.063 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site