* [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
@ 2020-07-03 0:42 Jin Yao
2020-07-03 11:00 ` Jiri Olsa
0 siblings, 1 reply; 9+ messages in thread
From: Jin Yao @ 2020-07-03 0:42 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, irogers, Jin Yao
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 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) ]
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-03 0:42 [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event Jin Yao @ 2020-07-03 11:00 ` Jiri Olsa 2020-07-04 0:31 ` Jin, Yao 0 siblings, 1 reply; 9+ messages in thread From: Jiri Olsa @ 2020-07-03 11:00 UTC (permalink / raw) To: Jin Yao Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin, irogers, Adrian Hunter 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' > > 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 > > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-03 11:00 ` Jiri Olsa @ 2020-07-04 0:31 ` Jin, Yao 2020-07-06 0:47 ` Ian Rogers 0 siblings, 1 reply; 9+ messages in thread From: Jin, Yao @ 2020-07-04 0:31 UTC (permalink / raw) To: Jiri Olsa Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin, irogers, Adrian Hunter 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' >> >> 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 >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-04 0:31 ` Jin, Yao @ 2020-07-06 0:47 ` Ian Rogers 2020-07-06 0:55 ` Jin, Yao 0 siblings, 1 reply; 9+ messages in thread From: Ian Rogers @ 2020-07-06 0:47 UTC (permalink / raw) To: Jin, Yao Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter 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 > >> 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 > >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-06 0:47 ` Ian Rogers @ 2020-07-06 0:55 ` Jin, Yao 2020-07-17 3:33 ` Jin, Yao 0 siblings, 1 reply; 9+ messages in thread From: Jin, Yao @ 2020-07-06 0:55 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter 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 >>>> >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-06 0:55 ` Jin, Yao @ 2020-07-17 3:33 ` Jin, Yao 2020-07-17 8:24 ` Jiri Olsa 0 siblings, 1 reply; 9+ messages in thread From: Jin, Yao @ 2020-07-17 3:33 UTC (permalink / raw) To: Ian Rogers, Jiri Olsa, Arnaldo Carvalho de Melo Cc: Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter 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 >>>>> >>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-17 3:33 ` Jin, Yao @ 2020-07-17 8:24 ` Jiri Olsa 2020-07-17 8:30 ` Jin, Yao 0 siblings, 1 reply; 9+ messages in thread From: Jiri Olsa @ 2020-07-17 8:24 UTC (permalink / raw) To: Jin, Yao Cc: Ian Rogers, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter On Fri, Jul 17, 2020 at 11:33:46AM +0800, Jin, Yao wrote: > 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? hi, my only concern was that it would conflict with Adrian's patch, other than that: Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-17 8:24 ` Jiri Olsa @ 2020-07-17 8:30 ` Jin, Yao 2020-07-17 11:38 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 9+ messages in thread From: Jin, Yao @ 2020-07-17 8:30 UTC (permalink / raw) To: Jiri Olsa Cc: Ian Rogers, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter On 7/17/2020 4:24 PM, Jiri Olsa wrote: > On Fri, Jul 17, 2020 at 11:33:46AM +0800, Jin, Yao wrote: >> 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? > > hi, > my only concern was that it would conflict with Adrian's patch, > other than that: > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > thanks, > jirka > Thanks Jiri! Adrian's patch has not been merged otherwise I can rebase my patch on top of Adrian's patch. Thanks Jin Yao ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event 2020-07-17 8:30 ` Jin, Yao @ 2020-07-17 11:38 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 9+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-07-17 11:38 UTC (permalink / raw) To: Jin, Yao Cc: Jiri Olsa, Ian Rogers, Jiri Olsa, Peter Zijlstra, Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang, Jin, Yao, Adrian Hunter Em Fri, Jul 17, 2020 at 04:30:21PM +0800, Jin, Yao escreveu: > > > On 7/17/2020 4:24 PM, Jiri Olsa wrote: > > On Fri, Jul 17, 2020 at 11:33:46AM +0800, Jin, Yao wrote: > > > 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? > > > > hi, > > my only concern was that it would conflict with Adrian's patch, > > other than that: > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > > > thanks, > > jirka > > > > Thanks Jiri! > > Adrian's patch has not been merged otherwise I can rebase my patch on top of Adrian's patch. I'll check this today. - Arnaldo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-17 11:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-03 0:42 [PATCH] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event Jin Yao 2020-07-03 11:00 ` Jiri Olsa 2020-07-04 0:31 ` Jin, Yao 2020-07-06 0:47 ` Ian Rogers 2020-07-06 0:55 ` Jin, Yao 2020-07-17 3:33 ` Jin, Yao 2020-07-17 8:24 ` Jiri Olsa 2020-07-17 8:30 ` Jin, Yao 2020-07-17 11:38 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).