Messages in this thread | | | Date | Mon, 29 Oct 2012 16:43:04 +0100 | Subject | Re: [Patch v1 04/10] perf/x86: add memory profiling via PEBS Load Latency | From | Stephane Eranian <> |
| |
On Mon, Oct 29, 2012 at 4:38 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, 2012-10-29 at 16:15 +0100, Stephane Eranian wrote: >> + fll = event->hw.flags & PERF_X86_EVENT_PEBS_LDLAT; >> + >> perf_sample_data_init(&data, 0, event->hw.last_period); >> >> + data.period = event->hw.last_period; >> + sample_type = event->attr.sample_type; >> + >> + /* >> + * if PEBS-LL or PreciseStore >> + */ >> + if (fll) { >> + if (sample_type & PERF_SAMPLE_ADDR) >> + data.addr = pebs->dla; >> + >> + /* >> + * Use latency for cost (only avail with PEBS-LL) >> + */ >> + if (fll && (sample_type & PERF_SAMPLE_COST)) >> + data.cost = pebs->lat; >> + >> + /* >> + * data.dsrc encodes the data source >> + */ >> + if (sample_type & PERF_SAMPLE_DSRC) { >> + if (fll) >> + data.dsrc.val = load_latency_data(pebs->dse); >> + } >> + } > > > Why does fl1 exist? the above looks really convoluted, all fl1 checks > inside are pointless. Also 'fl1' is a bizarre name, 'pebs_ll' would be a > better name I guess. > You meant fll, instead I think.
> What's wrong with: > > if (event->hw.flags & PERF_X86_EVENT_PEBS_LDLAT) { > if (sample_type & PERF_SAMPLE_ADDR) > data.addr = pebs->dla; > > if (sample_type & PERF_SAMPLE_COST) > data.cost = perf->lat; > > if (sample_type & PERF_SAMPLE_DSRC) > data.dsrc.val = load_latency_data(pebs->dse); > } > Well, that would work too, but I am trying to factorize the code with Precise Store which is a later patch. But we could clone your proposal and do:
> if (event->hw.flags & PERF_X86_EVENT_PEBS_ST) { > if (sample_type & PERF_SAMPLE_ADDR) > data.addr = pebs->dla; > > if (sample_type & PERF_SAMPLE_DSRC) > data.dsrc.val = precise_store_data(pebs->dse); > }
I am fine with that too.
| |