* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.