by Madhavan Srinivasan

[permalink] [raw]

Subject: Re: [PATCH v4 5/7] powerpc/powernv: add event attribute and group to nest pmu

On Thursday 09 July 2015 04:13 AM, Sukadev Bhattiprolu wrote:
> Madhavan Srinivasan [[email protected]] wrote:
> | Add code to create event/format attributes and attribute groups for
> | each nest pmu.
> |
> | Cc: Michael Ellerman <[email protected]>
> | Cc: Benjamin Herrenschmidt <[email protected]>
> | Cc: Paul Mackerras <[email protected]>
> | Cc: Anton Blanchard <[email protected]>
> | Cc: Sukadev Bhattiprolu <[email protected]>
> | Cc: Anshuman Khandual <[email protected]>
> | Cc: Stephane Eranian <[email protected]>
> | Signed-off-by: Madhavan Srinivasan <[email protected]>
> | ---
> | arch/powerpc/perf/nest-pmu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
> | 1 file changed, 57 insertions(+)
> |
> | diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
> | index 6116ff3..20ed9f8 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");
> | +struct attribute *p8_nest_format_attrs[] = {
>
> name collision unlikely, but could this be static struct?
>
> | + &format_attr_event.attr,
> | + NULL,
> | +};
> | +
> | +struct attribute_group p8_nest_format_group = {
>
> static struct?

Sure will check it.

>
> | + .name = "format",
> | + .attrs = p8_nest_format_attrs,
> | +};
> | +
> | static int nest_event_info(struct property *pp, char *start,
> | struct nest_ima_events *p8_events, int flg, u32 val)
> | {
> | @@ -45,6 +56,48 @@ static int nest_event_info(struct property *pp, char *start,
> | return 0;
> | }
> |
> | +/*
> | + * Populate event name and string in attribute
> | + */
> | +struct attribute *dev_str_attr(const char *name, const char *str)
>
> static function?

Sure. Will check it.

>
> | +{
> | + struct perf_pmu_events_attr *attr;
> | +
> | + attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> | +
>
> We recently needed following in 24x7 counters to keep lockdep happy:

Yeah, i saw your patch and wanted to add it, but missed it. My bad
Will add it in the next version.

>
> 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;
> | +}
> | +
> | +int update_events_in_group(
>
> static function?
>
> nit: do we need a new line before the first parameter? some functions
> in the file don't add the new line.

Checkpatch complained abt 80 character limit, hence did a split.

>
> | + struct nest_ima_events *p8_events, int nevents, struct nest_pmu *pmu)
>
> s/idx/nevents/?

Weird, I dont see "nevents" in the patch I sent? it is "idx". Dont know
where
it came from :)

>
> | +{
> | + struct attribute_group *attr_group;
> | + struct attribute **attrs;
> | + int i;
> | +
> | + /* Allocate memory for event attribute group */
> | + attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) +
> | + sizeof(*attr_group)), GFP_KERNEL);
> | + if (!attr_group)
> | + return -ENOMEM;
> | +
> | + attrs = (struct attribute **)(attr_group + 1);
>
> Can you add a comment on the +1?

Sure, will do.

>
> | + 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;
>
> The ->attr_groups[0] is initialized here, after the ->attr_groups[1] and
> attr_groups[2] are initialized in caller. Since, ->attr_groups[1] and
> ->attr_groups[2] are set to global (loop-invariant) values, can we
> initialize all the attribute-groups here? May need to rename function.

I dont get it, reinitialize all the three here in this function?

> | + return 0;
> | +}
> | +
> | static int nest_pmu_create(struct device_node *dev, int pmu_index)
> | {
> | struct nest_ima_events **p8_events_arr, *p8_events;
> | @@ -91,6 +144,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;
> | }
> |
> | @@ -122,6 +176,9 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index)
> | idx++;
> | }
> |
> | + update_events_in_group(
>
> nit: need newline before first param?

once again, checkpatch warning of 80 character.

>
> | + (struct nest_ima_events *)p8_events_arr, idx, pmu_ptr);
> | +
> | return 0;
> | }
> |
> | --
> | 1.9.1

2015-07-09 21:32:11

by Sukadev Bhattiprolu

[permalink] [raw]

Subject: Re: [PATCH v4 4/7] powerpc/powernv: detect supported nest pmus and its events

Madhavan Srinivasan [[email protected]] wrote:
|
| > Are the 'start.*' and 'unit.*' files events by themselves or just attributes
| > of events?
|
| These are attributes needed for computation. unit and scale attributes
| will be used by perf tool in post-processing the counter data. These
| can also use by other tools like pcp.

OK. Thanks for clarifying.