* [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).