| Message ID | 1437045206-7491-7-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> +static void p8_nest_read_counter(struct perf_event *event) > +{ > + uint64_t *addr; > + u64 data = 0; You've got a u64 and a uint64_t, and then... > + > + addr = (u64 *)event->hw.event_base; ... you cast to event_base to a u64 pointer, which you assign to a uint64_t pointer. > + data = __be64_to_cpu(*addr); And now you dereference the pointer. Could you just have: data = __be64_to_cpu(*event->hw.event_base); > + local64_set(&event->hw.prev_count, data); > +} > + > +static void p8_nest_perf_event_update(struct perf_event *event) > +{ > + u64 counter_prev, counter_new, final_count; > + uint64_t *addr; > + > + addr = (uint64_t *)event->hw.event_base; Here at least the cast type is the same as the type of addr, but again, why do you need the different types, and why local variable? > + counter_prev = local64_read(&event->hw.prev_count); > + counter_new = __be64_to_cpu(*addr); > + final_count = counter_new - counter_prev; > + > + local64_set(&event->hw.prev_count, counter_new); > + local64_add(final_count, &event->count); > +} > + > +static void p8_nest_event_start(struct perf_event *event, int flags) > +{ > + event->hw.state = 0; Should this be an enum or a #define rather than a bare 0? (It may not need to be, I was just wondering because I don't know what 0 means.) > + p8_nest_read_counter(event); > +} > +
On Wednesday 22 July 2015 10:26 AM, Daniel Axtens wrote: >> +static void p8_nest_read_counter(struct perf_event *event) >> +{ >> + uint64_t *addr; >> + u64 data = 0; > You've got a u64 and a uint64_t, and then... >> + >> + addr = (u64 *)event->hw.event_base; > ... you cast to event_base to a u64 pointer, which you assign to a > uint64_t pointer. >> + data = __be64_to_cpu(*addr); > And now you dereference the pointer. > Could you just have: > data = __be64_to_cpu(*event->hw.event_base); >> + local64_set(&event->hw.prev_count, data); >> +} >> + >> +static void p8_nest_perf_event_update(struct perf_event *event) >> +{ >> + u64 counter_prev, counter_new, final_count; >> + uint64_t *addr; >> + >> + addr = (uint64_t *)event->hw.event_base; > Here at least the cast type is the same as the type of addr, but again, > why do you need the different types, and why local variable? Damn sorry, copy paste errors. When I added debug prints i messed the type case in both the functions. I will make them as uint64_t. Thanks for this detail review Maddy >> + counter_prev = local64_read(&event->hw.prev_count); >> + counter_new = __be64_to_cpu(*addr); >> + final_count = counter_new - counter_prev; >> + >> + local64_set(&event->hw.prev_count, counter_new); >> + local64_add(final_count, &event->count); >> +} >> + >> +static void p8_nest_event_start(struct perf_event *event, int flags) >> +{ >> + event->hw.state = 0; > Should this be an enum or a #define rather than a bare 0? (It may not > need to be, I was just wondering because I don't know what 0 means.) I could remove it since was just initializing at the start. >> + p8_nest_read_counter(event); >> +} >> +
On Thu, 2015-07-23 at 12:14 +0530, Madhavan Srinivasan wrote: > > On Wednesday 22 July 2015 10:26 AM, Daniel Axtens wrote: > >> +static void p8_nest_read_counter(struct perf_event *event) > >> +{ > >> + uint64_t *addr; > >> + u64 data = 0; > > You've got a u64 and a uint64_t, and then... > >> + > >> + addr = (u64 *)event->hw.event_base; > > ... you cast to event_base to a u64 pointer, which you assign to a > > uint64_t pointer. > >> + data = __be64_to_cpu(*addr); > > And now you dereference the pointer. > > Could you just have: > > data = __be64_to_cpu(*event->hw.event_base); > >> + local64_set(&event->hw.prev_count, data); > >> +} > >> + > >> +static void p8_nest_perf_event_update(struct perf_event *event) > >> +{ > >> + u64 counter_prev, counter_new, final_count; > >> + uint64_t *addr; > >> + > >> + addr = (uint64_t *)event->hw.event_base; > > Here at least the cast type is the same as the type of addr, but again, > > why do you need the different types, and why local variable? > > Damn sorry, copy paste errors. When I added debug prints i messed > the type case in both the functions. I will make them as uint64_t. No please use u64/u32 etc. Most code in powerpc does and I prefer them. cheers
On Thursday 23 July 2015 02:34 PM, Michael Ellerman wrote: > On Thu, 2015-07-23 at 12:14 +0530, Madhavan Srinivasan wrote: >> On Wednesday 22 July 2015 10:26 AM, Daniel Axtens wrote: >>>> +static void p8_nest_read_counter(struct perf_event *event) >>>> +{ >>>> + uint64_t *addr; >>>> + u64 data = 0; >>> You've got a u64 and a uint64_t, and then... >>>> + >>>> + addr = (u64 *)event->hw.event_base; >>> ... you cast to event_base to a u64 pointer, which you assign to a >>> uint64_t pointer. >>>> + data = __be64_to_cpu(*addr); >>> And now you dereference the pointer. >>> Could you just have: >>> data = __be64_to_cpu(*event->hw.event_base); >>>> + local64_set(&event->hw.prev_count, data); >>>> +} >>>> + >>>> +static void p8_nest_perf_event_update(struct perf_event *event) >>>> +{ >>>> + u64 counter_prev, counter_new, final_count; >>>> + uint64_t *addr; >>>> + >>>> + addr = (uint64_t *)event->hw.event_base; >>> Here at least the cast type is the same as the type of addr, but again, >>> why do you need the different types, and why local variable? >> Damn sorry, copy paste errors. When I added debug prints i messed >> the type case in both the functions. I will make them as uint64_t. > No please use u64/u32 etc. Most code in powerpc does and I prefer them. > > cheers Ok Sure. Will use only u64 and u32. Thanks maddy >
diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c index f3418bdec1cd..2ebd0508e9b3 100644 --- a/arch/powerpc/perf/nest-pmu.c +++ b/arch/powerpc/perf/nest-pmu.c @@ -24,6 +24,101 @@ static struct attribute_group p8_nest_format_group = { .attrs = p8_nest_format_attrs, }; +static int p8_nest_event_init(struct perf_event *event) +{ + int chip_id; + + if (event->attr.type != event->pmu->type) + return -ENOENT; + + /* Sampling not supported yet */ + if (event->hw.sample_period) + return -EINVAL; + + /* unsupported modes and filters */ + if (event->attr.exclude_user || + event->attr.exclude_kernel || + event->attr.exclude_hv || + event->attr.exclude_idle || + event->attr.exclude_host || + event->attr.exclude_guest) + return -EINVAL; + + if (event->cpu < 0) + return -EINVAL; + + chip_id = topology_physical_package_id(event->cpu); + event->hw.event_base = event->attr.config + + p8_nest_perchip_info[chip_id].vbase; + + return 0; +} + +static void p8_nest_read_counter(struct perf_event *event) +{ + uint64_t *addr; + u64 data = 0; + + addr = (u64 *)event->hw.event_base; + data = __be64_to_cpu(*addr); + local64_set(&event->hw.prev_count, data); +} + +static void p8_nest_perf_event_update(struct perf_event *event) +{ + u64 counter_prev, counter_new, final_count; + uint64_t *addr; + + addr = (uint64_t *)event->hw.event_base; + counter_prev = local64_read(&event->hw.prev_count); + counter_new = __be64_to_cpu(*addr); + final_count = counter_new - counter_prev; + + local64_set(&event->hw.prev_count, counter_new); + local64_add(final_count, &event->count); +} + +static void p8_nest_event_start(struct perf_event *event, int flags) +{ + event->hw.state = 0; + p8_nest_read_counter(event); +} + +static void p8_nest_event_stop(struct perf_event *event, int flags) +{ + if (flags & PERF_EF_UPDATE) + p8_nest_perf_event_update(event); +} + +static int p8_nest_event_add(struct perf_event *event, int flags) +{ + if (flags & PERF_EF_START) + p8_nest_event_start(event, flags); + + return 0; +} + +/* + * Populate pmu ops in the structure + */ +static int update_pmu_ops(struct nest_pmu *pmu) +{ + if (!pmu) + return -EINVAL; + + pmu->pmu.task_ctx_nr = perf_invalid_context; + pmu->pmu.event_init = p8_nest_event_init; + pmu->pmu.add = p8_nest_event_add; + pmu->pmu.del = p8_nest_event_stop; + pmu->pmu.start = p8_nest_event_start; + pmu->pmu.stop = p8_nest_event_stop; + pmu->pmu.read = p8_nest_perf_event_update; + pmu->pmu.attr_groups = pmu->attr_groups; + + return 0; +} + + static int nest_event_info(struct property *pp, char *name, struct nest_ima_events *p8_events, int string, u32 val) { @@ -189,6 +284,16 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index) update_events_in_group( (struct nest_ima_events *)p8_events_arr, idx, pmu_ptr); + update_pmu_ops(pmu_ptr); + /* Register the pmu */ + ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1); + if (ret) { + pr_err("Nest PMU %s Register failed\n", pmu_ptr->pmu.name); + return ret; + } + + pr_info("%s performance monitor hardware support registered\n", + pmu_ptr->pmu.name); return 0; }
Add set of generic nest pmu related event functions to be used by each nest pmu. Add code to register nest pmus. 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 | 105 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+)