Messages in this thread | | | Subject | Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event | From | "Jin, Yao" <> | Date | Wed, 22 Jul 2020 13:00:03 +0800 |
| |
Hi Jiri,
On 7/20/2020 5:17 PM, Jiri Olsa wrote: > On Mon, Jul 20, 2020 at 09:00:13AM +0800, Jin Yao wrote: >> Since commit 0a892c1c9472 ("perf record: Add dummy event during system wide synthesis"), >> a dummy event is added to capture mmaps. >> >> But if we run perf-record as, >> >> # perf record -e cycles:p -IXMM0 -a -- sleep 1 >> Error: >> dummy:HG: PMU Hardware doesn't support sampling/overflow-interrupts. Try 'perf stat' >> >> The issue is, if we enable the extended regs (-IXMM0), but the >> pmu->capabilities is not set with PERF_PMU_CAP_EXTENDED_REGS, the kernel >> will return -EOPNOTSUPP error. >> >> See following code: >> >> /* in kernel/events/core.c */ >> static int perf_try_init_event(struct pmu *pmu, struct perf_event *event) >> >> { >> .... >> if (!(pmu->capabilities & PERF_PMU_CAP_EXTENDED_REGS) && >> has_extended_regs(event)) >> ret = -EOPNOTSUPP; >> .... >> } >> >> For software dummy event, the PMU should not be set with >> PERF_PMU_CAP_EXTENDED_REGS. But unfortunately now, the dummy >> event has possibility to be set with PERF_REG_EXTENDED_MASK bit. >> >> In evsel__config, /* tools/perf/util/evsel.c */ >> >> if (opts->sample_intr_regs) { >> attr->sample_regs_intr = opts->sample_intr_regs; >> } >> >> If we use -IXMM0, the attr>sample_regs_intr will be set with >> PERF_REG_EXTENDED_MASK bit. >> >> It doesn't make sense to set attr->sample_regs_intr for a >> software dummy event. >> >> This patch adds dummy event checking before setting >> attr->sample_regs_intr and attr->sample_regs_user. >> >> After: >> # ./perf record -e cycles:p -IXMM0 -a -- sleep 1 >> [ perf record: Woken up 1 times to write data ] >> [ perf record: Captured and wrote 0.413 MB perf.data (45 samples) ] >> >> v2: >> --- >> Rebase to perf/core >> >> Fixes: 0a892c1c9472 ("perf record: Add dummy event during system wide synthesis") >> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> >> --- >> tools/perf/util/evsel.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> index 9aa51a65593d..11794d3b7879 100644 >> --- a/tools/perf/util/evsel.c >> +++ b/tools/perf/util/evsel.c >> @@ -1014,12 +1014,14 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts, >> if (callchain && callchain->enabled && !evsel->no_aux_samples) >> evsel__config_callchain(evsel, opts, callchain); >> >> - if (opts->sample_intr_regs && !evsel->no_aux_samples) { >> + if (opts->sample_intr_regs && !evsel->no_aux_samples && >> + !evsel__is_dummy_event(evsel)) { > > hum, I thought it'd look something like this: > > if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) > > but I'm not sure how no_aux_samples flag works exactly.. so it might be > correct.. just making sure ;-) > > cc-ing Adrian > > jirka > >
no_aux_samples is set to false by default and it's only set to true by pt, right?
So most of the time, !evsel->no_aux_samples is always true.
if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) { attr->sample_regs_intr = opts->sample_intr_regs; evsel__set_sample_bit(evsel, REGS_INTR); }
So even if the evsel is dummy event, the condition check is true. :(
Or maybe I misunderstand anything?
Thanks Jin Yao
>> attr->sample_regs_intr = opts->sample_intr_regs; >> evsel__set_sample_bit(evsel, REGS_INTR); >> } >> >> - if (opts->sample_user_regs && !evsel->no_aux_samples) { >> + if (opts->sample_user_regs && !evsel->no_aux_samples && >> + !evsel__is_dummy_event(evsel)) { >> attr->sample_regs_user |= opts->sample_user_regs; >> evsel__set_sample_bit(evsel, REGS_USER); >> } >> -- >> 2.17.1 >> >
| |