Messages in this thread | | | Subject | Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event | From | "Jin, Yao" <> | Date | Mon, 6 Jul 2020 08:55:44 +0800 |
| |
Hi Ian,
On 7/6/2020 8:47 AM, Ian Rogers wrote: > On Fri, Jul 3, 2020 at 5:31 PM Jin, Yao <yao.jin@linux.intel.com> wrote: >> >> Hi Jiri, >> >> On 7/3/2020 7:00 PM, Jiri Olsa wrote: >>> On Fri, Jul 03, 2020 at 08:42:15AM +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' >>>> > > Sorry for the breakage caused by modifying the dummy event. Could we > add a test to cover the issue? Perhaps in tools/perf/tests/shell/. > Trying to reproduce with a register on my skylakex on a 5.6.14 kernel > with: > > $ perf record -e cycles:p -IAX -a -- sleep 1 > > succeeds. > > Thanks, > Ian >
-IAX should be no problem. The issue only occurs on the platform with extended regs supports, such as ICL. So I don't know if it's suitable to add it to perf test suite.
Thanks Jin Yao
>>>> 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 pieces. >>>> >>>> /* 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 be not set with >>>> PERF_PMU_CAP_EXTENDED_REGS. But unfortunately in current code, 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. >>>> >>>> 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) ] >>> >>> LGTM, Adrian (cc-ed) just added another check to the same place, >>> but it looks like both of them should be there: >>> >>> https://lore.kernel.org/lkml/20200630133935.11150-2-adrian.hunter@intel.com/ >>> >>> jirka >>> >> >> Thanks Jiri! Yes, it looks like both of checks should be added here. >> >> So do I post v2 (just rebase) once Adrian's patch gets merged? >> >> Thanks >> Jin Yao >> >>>> >>>> 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 | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>>> index 96e5171dce41..df3315543e86 100644 >>>> --- a/tools/perf/util/evsel.c >>>> +++ b/tools/perf/util/evsel.c >>>> @@ -1020,12 +1020,12 @@ 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) { >>>> + if (opts->sample_intr_regs && !is_dummy_event(evsel)) { >>>> attr->sample_regs_intr = opts->sample_intr_regs; >>>> evsel__set_sample_bit(evsel, REGS_INTR); >>>> } >>>> >>>> - if (opts->sample_user_regs) { >>>> + if (opts->sample_user_regs && !is_dummy_event(evsel)) { >>>> attr->sample_regs_user |= opts->sample_user_regs; >>>> evsel__set_sample_bit(evsel, REGS_USER); >>>> } >>>> -- >>>> 2.17.1 >>>> >>>
| |