| Message ID | 1433260778-26497-8-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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? > + 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? > +} > + > +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. > + > + > 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
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
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; + attr->attr.attr.name = name; + attr->attr.attr.mode = 0444; + attr->attr.show = perf_event_sysfs_show; + + return &attr->attr.attr; +} + +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; +} + + 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; }
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(+)