| Message ID | 1437045206-7491-6-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Anton Blanchard <anton@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 | 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? > +} > + > +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?
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 <mpe@ellerman.id.au> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Paul Mackerras <paulus@samba.org> >> Cc: Anton Blanchard <anton@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 | 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. >
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; +} + +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; + + 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 nest_ima_events **p8_events_arr, *p8_events; @@ -93,6 +154,7 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index) /* Save the name to register it later */ sprintf(buf, "Nest_%s", (char *)pp->value); pmu_ptr->pmu.name = (char *)buf; + pmu_ptr->attr_groups[1] = &p8_nest_format_group; continue; } @@ -124,6 +186,9 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index) idx++; } + update_events_in_group( + (struct nest_ima_events *)p8_events_arr, idx, pmu_ptr); + return 0; }
Add code to create event/format attributes and attribute groups for each nest pmu. Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Anton Blanchard <anton@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 | 65 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)