| Message ID | 1437045206-7491-5-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> 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]; > + > +static int nest_event_info(struct property *pp, char *name, > + struct nest_ima_events *p8_events, int string, u32 val) 'int string' is a bit confusing. 'bool is_string' might be clearer, but I think it would be even better still to have different functions for string and non-string cases, especially because you only need val in the non-string case. That will also allow you to give the functions clearer names. I think the function is populating the event with info from the dt property (in the string case) or the val argument (non-string case) - maybe the names could reflect that somehow? > +{ > + char *buf; > + > + > +static int nest_pmu_create(struct device_node *dev, int pmu_index) > +{ > + struct nest_ima_events **p8_events_arr, *p8_events; > + struct nest_pmu *pmu_ptr; > + struct property *pp; > + char *buf, *start; > + const __be32 *lval; > + u32 val; > + int idx = 0, ret; > + > + if (!dev) > + return -EINVAL; > + > + /* memory for nest pmus */ > + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); > + if (!pmu_ptr) > + return -ENOMEM; > + > + /* Needed for hotplug/migration */ > + per_nest_pmu_arr[pmu_index] = pmu_ptr; > + > + /* memory for nest pmu events */ > + p8_events_arr = kzalloc((sizeof(struct nest_ima_events) * 64), > + GFP_KERNEL); > + if (!p8_events_arr) > + return -ENOMEM; > + p8_events = (struct nest_ima_events *)p8_events_arr; I'm still quite uncomfortable with this. - Why * 64? Should it be * P8_NEST_MAX_EVENTS_SUPPORTED? Or is it a different constant? - p8_events = p8_events_arr[0] would be clearer > + > + /* > + * Loop through each property > + */ > + for_each_property_of_node(dev, pp) { > + start = pp->name; > + > + if (!strcmp(pp->name, "name")) { > + if (!pp->value || > + (strnlen(pp->value, pp->length) == pp->length) || > + (pp->length > P8_NEST_MAX_PMU_NAME_LEN)) > + return -EINVAL; > + > + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + /* Save the name to register it later */ > + sprintf(buf, "Nest_%s", (char *)pp->value); > + pmu_ptr->pmu.name = (char *)buf; > + continue; > + } > + > + /* Skip these, we dont need it */ "don't" instead of "dont". > + if (!strcmp(pp->name, "phandle") || > + !strcmp(pp->name, "device_type") || > + !strcmp(pp->name, "linux,phandle")) > + continue; > + > + if (strncmp(pp->name, "unit.", 5) == 0) { > + /* Skip first few chars in the name */ The whole comment is pretty uninformative, as is the similar comment below. If you need a comment at all, maybe something along the lines of "Strip the prefix because <reason we don't need/want the prefix>"? > + start += 5; > + ret = nest_event_info(pp, start, p8_events++, 1, 0); > + } else if (strncmp(pp->name, "scale.", 6) == 0) { > + /* Skip first few chars in the name */ > + start += 6; > + ret = nest_event_info(pp, start, p8_events++, 1, 0); > + } else { > + lval = of_get_property(dev, pp->name, NULL); > + val = (uint32_t)be32_to_cpup(lval); > + > + ret = nest_event_info(pp, start, p8_events++, 0, val); > + } > + > + if (ret) > + return ret; > + > + /* book keeping */ > + idx++; You don't seem to use idx in this function, apart from incrementing it here...? > + } > + > + return 0; > +}
On Wednesday 22 July 2015 09:37 AM, Daniel Axtens wrote: > >> 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]; >> + >> +static int nest_event_info(struct property *pp, char *name, >> + struct nest_ima_events *p8_events, int string, u32 val) > 'int string' is a bit confusing. 'bool is_string' might be clearer, but > I think it would be even better still to have different functions for > string and non-string cases, especially because you only need val in the > non-string case. I would perfer to be a single function, since most of the code is same just of the sting or val part. yes. We can make is as is_string and will add comment explaining what is done here? > That will also allow you to give the functions clearer names. I think > the function is populating the event with info from the dt property (in > the string case) or the val argument (non-string case) - maybe the names > could reflect that somehow? >> +{ >> + char *buf; >> + > >> + >> +static int nest_pmu_create(struct device_node *dev, int pmu_index) >> +{ >> + struct nest_ima_events **p8_events_arr, *p8_events; >> + struct nest_pmu *pmu_ptr; >> + struct property *pp; >> + char *buf, *start; >> + const __be32 *lval; >> + u32 val; >> + int idx = 0, ret; >> + >> + if (!dev) >> + return -EINVAL; >> + >> + /* memory for nest pmus */ >> + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); >> + if (!pmu_ptr) >> + return -ENOMEM; >> + >> + /* Needed for hotplug/migration */ >> + per_nest_pmu_arr[pmu_index] = pmu_ptr; >> + >> + /* memory for nest pmu events */ >> + p8_events_arr = kzalloc((sizeof(struct nest_ima_events) * 64), >> + GFP_KERNEL); >> + if (!p8_events_arr) >> + return -ENOMEM; >> + p8_events = (struct nest_ima_events *)p8_events_arr; > I'm still quite uncomfortable with this. > - Why * 64? Should it be * P8_NEST_MAX_EVENTS_SUPPORTED? Or is it a > different constant? Yes it should be P8_NEST_MAX_EVENTS_SUPPORTED, and it should be define as 64. Missed it to replace the macro. Nice catch. > - p8_events = p8_events_arr[0] would be clearer > >> + >> + /* >> + * Loop through each property >> + */ >> + for_each_property_of_node(dev, pp) { >> + start = pp->name; >> + >> + if (!strcmp(pp->name, "name")) { >> + if (!pp->value || >> + (strnlen(pp->value, pp->length) == pp->length) || >> + (pp->length > P8_NEST_MAX_PMU_NAME_LEN)) >> + return -EINVAL; >> + >> + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + /* Save the name to register it later */ >> + sprintf(buf, "Nest_%s", (char *)pp->value); >> + pmu_ptr->pmu.name = (char *)buf; >> + continue; >> + } >> + >> + /* Skip these, we dont need it */ > "don't" instead of "dont". Will change it. >> + if (!strcmp(pp->name, "phandle") || >> + !strcmp(pp->name, "device_type") || >> + !strcmp(pp->name, "linux,phandle")) >> + continue; >> + >> + if (strncmp(pp->name, "unit.", 5) == 0) { >> + /* Skip first few chars in the name */ > The whole comment is pretty uninformative, as is the similar comment > below. If you need a comment at all, maybe something along the lines of > "Strip the prefix because <reason we don't need/want the prefix>"? Yes will change it. Thanks >> + start += 5; >> + ret = nest_event_info(pp, start, p8_events++, 1, 0); >> + } else if (strncmp(pp->name, "scale.", 6) == 0) { >> + /* Skip first few chars in the name */ >> + start += 6; >> + ret = nest_event_info(pp, start, p8_events++, 1, 0); >> + } else { >> + lval = of_get_property(dev, pp->name, NULL); >> + val = (uint32_t)be32_to_cpup(lval); >> + >> + ret = nest_event_info(pp, start, p8_events++, 0, val); >> + } >> + >> + if (ret) >> + return ret; >> + >> + /* book keeping */ >> + idx++; > You don't seem to use idx in this function, apart from incrementing it > here...? Used in next patch. >> + } >> + >> + return 0; >> +} >
On Thu, 2015-07-23 at 11:33 +0530, Madhavan Srinivasan wrote: > > On Wednesday 22 July 2015 09:37 AM, Daniel Axtens wrote: > > > >> 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]; > >> + > >> +static int nest_event_info(struct property *pp, char *name, > >> + struct nest_ima_events *p8_events, int string, u32 val) > > 'int string' is a bit confusing. 'bool is_string' might be clearer, but > > I think it would be even better still to have different functions for > > string and non-string cases, especially because you only need val in the > > non-string case. > > I would perfer to be a single function, since most of the code is same > just of the sting or val part. yes. We can make is as is_string and will > add comment explaining what is done here? I think Daniel's right, it would be better as two functions. The only part that is common after the if (string) check is the p8_events->ev_value = buf assignment. So you should be able to keep all the code up to the if (string) check in a shared function and just have two wrappers that use it. cheers
On Thursday 23 July 2015 02:41 PM, Michael Ellerman wrote: > On Thu, 2015-07-23 at 11:33 +0530, Madhavan Srinivasan wrote: >> On Wednesday 22 July 2015 09:37 AM, Daniel Axtens wrote: >>> >>>> 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]; >>>> + >>>> +static int nest_event_info(struct property *pp, char *name, >>>> + struct nest_ima_events *p8_events, int string, u32 val) >>> 'int string' is a bit confusing. 'bool is_string' might be clearer, but >>> I think it would be even better still to have different functions for >>> string and non-string cases, especially because you only need val in the >>> non-string case. >> I would perfer to be a single function, since most of the code is same >> just of the sting or val part. yes. We can make is as is_string and will >> add comment explaining what is done here? > I think Daniel's right, it would be better as two functions. > > The only part that is common after the if (string) check is the > p8_events->ev_value = buf assignment. > > So you should be able to keep all the code up to the if (string) check in a > shared function and just have two wrappers that use it. > > cheers Sure. Will do. Maddy > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c index e7d45ed4922d..c4c08e4dee55 100644 --- a/arch/powerpc/perf/nest-pmu.c +++ b/arch/powerpc/perf/nest-pmu.c @@ -11,6 +11,121 @@ #include "nest-pmu.h" 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]; + +static int nest_event_info(struct property *pp, char *name, + struct nest_ima_events *p8_events, int string, u32 val) +{ + char *buf; + + /* memory for event name */ + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + strncpy(buf, name, strlen(name)); + p8_events->ev_name = buf; + + /* memory for content */ + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + if (string) { + /* string content*/ + if (!pp->value || + (strnlen(pp->value, pp->length) == pp->length) || + (pp->length > P8_NEST_MAX_PMU_NAME_LEN)) + return -EINVAL; + + strncpy(buf, (const char *)pp->value, pp->length); + } else + sprintf(buf, "event=0x%x", val); + + p8_events->ev_value = buf; + return 0; +} + +static int nest_pmu_create(struct device_node *dev, int pmu_index) +{ + struct nest_ima_events **p8_events_arr, *p8_events; + struct nest_pmu *pmu_ptr; + struct property *pp; + char *buf, *start; + const __be32 *lval; + u32 val; + int idx = 0, ret; + + if (!dev) + return -EINVAL; + + /* memory for nest pmus */ + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); + if (!pmu_ptr) + return -ENOMEM; + + /* Needed for hotplug/migration */ + per_nest_pmu_arr[pmu_index] = pmu_ptr; + + /* memory for nest pmu events */ + p8_events_arr = kzalloc((sizeof(struct nest_ima_events) * 64), + GFP_KERNEL); + if (!p8_events_arr) + return -ENOMEM; + p8_events = (struct nest_ima_events *)p8_events_arr; + + /* + * Loop through each property + */ + for_each_property_of_node(dev, pp) { + start = pp->name; + + if (!strcmp(pp->name, "name")) { + if (!pp->value || + (strnlen(pp->value, pp->length) == pp->length) || + (pp->length > P8_NEST_MAX_PMU_NAME_LEN)) + return -EINVAL; + + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + /* Save the name to register it later */ + sprintf(buf, "Nest_%s", (char *)pp->value); + pmu_ptr->pmu.name = (char *)buf; + continue; + } + + /* Skip these, we dont need it */ + if (!strcmp(pp->name, "phandle") || + !strcmp(pp->name, "device_type") || + !strcmp(pp->name, "linux,phandle")) + continue; + + if (strncmp(pp->name, "unit.", 5) == 0) { + /* Skip first few chars in the name */ + start += 5; + ret = nest_event_info(pp, start, p8_events++, 1, 0); + } else if (strncmp(pp->name, "scale.", 6) == 0) { + /* Skip first few chars in the name */ + start += 6; + ret = nest_event_info(pp, start, p8_events++, 1, 0); + } else { + lval = of_get_property(dev, pp->name, NULL); + val = (uint32_t)be32_to_cpup(lval); + + ret = nest_event_info(pp, start, p8_events++, 0, val); + } + + if (ret) + return ret; + + /* book keeping */ + idx++; + } + + return 0; +} static int nest_ima_dt_parser(void) { @@ -19,7 +134,7 @@ static int nest_ima_dt_parser(void) const __be64 *chip_ima_size; struct device_node *dev; struct perchip_nest_info *p8ni; - int idx; + int idx, ret; /* * "nest-ima" folder contains two things, @@ -50,6 +165,15 @@ static int nest_ima_dt_parser(void) p8ni->vbase = (uint64_t) phys_to_virt(p8ni->pbase); } + /* Look for supported Nest PMU units */ + idx = 0; + for_each_node_by_type(dev, "nest-ima-unit") { + ret = nest_pmu_create(dev, idx); + if (ret) + return ret; + idx++; + } + return 0; }
Parse device tree to detect supported nest pmu units. Traverse through each nest pmu unit folder to find supported events and corresponding unit/scale files (if any). The nest unit event file from DT, will contain the offset in the reserved memory region to get the counter data for a given event. Kernel code uses this offset as event configuration value. Device tree parser code also looks for scale/unit in the file name and passes on the file as an event attr for perf tool to use in the post processing. 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 | 126 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)