Messages in this thread | | | From | David Carrillo-Cisneros <> | Date | Mon, 8 Aug 2016 22:04:23 -0700 | Subject | Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG |
| |
Hi Nilay,
Sounds good, I will post an updated version.
Thanks, David
On Mon, Aug 8, 2016 at 9:12 AM, Nilay Vaish <nilayvaish@gmail.com> wrote: > On 7 August 2016 at 15:10, David Carrillo-Cisneros <davidcc@google.com> wrote: >> Hi Nilay, >> >>>> static int perf_event_read(struct perf_event *event, bool group) >>>> { >>>> - int ret = 0; >>>> + int ret = 0, cpu_to_read; >>>> >>>> - /* >>>> - * If event is enabled and currently active on a CPU, update the >>>> - * value in the event structure: >>>> - */ >>>> - if (event->state == PERF_EVENT_STATE_ACTIVE) { >>>> + cpu_to_read = find_cpu_to_read(event); >>>> + >>>> + if (cpu_to_read >= 0) { >>>> struct perf_read_data data = { >>>> .event = event, >>>> .group = group, >>>> .ret = 0, >>>> }; >>>> - ret = smp_call_function_single(event->oncpu, >>>> + ret = smp_call_function_single(cpu_to_read, >>>> __perf_event_read, &data, >>>> 1); >>>> ret = ret ? : data.ret; >>>> } else if (event->state == PERF_EVENT_STATE_INACTIVE) { >>>> >>> >>> I would like to suggest a small change to this patch. I think the check on >>> PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read() >>> function. The new function should assume that the event is active. I find >>> this more readable since the next check in function perf_event_read() is on >>> PERF_EVENT_STATE_INACTIVE. >> >> Two oncoming flags that Intel CQM/CMT will use are meant to allow read >> even if event is inactive. This makes sense in CQM/CMT because the hw >> RMID is always reserved. I am ok with keeping the check for >> STATE_ACTIVE until those flags are actually introduced, tough. > > > Hello David > > Lets go with checking PERF_EVENT_STATE_ACTIVE in perf_event_read() for > the time being. With the new version of the patch that you posted, I > find that checking PERF_EVENT_STATE_ACTIVE in find_cpu_to_read() makes > you introduce another if statement for checking STATE_INACTIVE. > > If your CQM/CMT patches later need the code structure you have now, I > would also support it. But as of now, I think, it is better to check > STATE_ACTIVE in perf_event_read(). > > > Thanks > Nilay
| |