On Mon, Oct 29, 2012 at 4:38 PM, Peter Zijlstra <pet...@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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/