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 | Fri, 17 Jul 2020 11:33:46 +0800 |
| |
Hi,
On 7/6/2020 8:55 AM, Jin, Yao wrote: > 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 >
Can this fix patch be accepted?
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 >>>>> >>>>
| |