* [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
@ 2020-07-20 1:00 Jin Yao
2020-07-20 9:17 ` Jiri Olsa
0 siblings, 1 reply; 8+ messages in thread
From: Jin Yao @ 2020-07-20 1:00 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:
/* 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 not be set with
PERF_PMU_CAP_EXTENDED_REGS. But unfortunately now, 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 and attr->sample_regs_user.
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) ]
v2:
---
Rebase to perf/core
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 | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9aa51a65593d..11794d3b7879 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
+ if (opts->sample_intr_regs && !evsel->no_aux_samples &&
+ !evsel__is_dummy_event(evsel)) {
attr->sample_regs_intr = opts->sample_intr_regs;
evsel__set_sample_bit(evsel, REGS_INTR);
}
- if (opts->sample_user_regs && !evsel->no_aux_samples) {
+ if (opts->sample_user_regs && !evsel->no_aux_samples &&
+ !evsel__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] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-20 1:00 [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event Jin Yao
@ 2020-07-20 9:17 ` Jiri Olsa
2020-07-22 5:00 ` Jin, Yao
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2020-07-20 9:17 UTC (permalink / raw)
To: Jin Yao
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, irogers, Adrian Hunter
On Mon, Jul 20, 2020 at 09:00:13AM +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:
>
> /* 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 not be set with
> PERF_PMU_CAP_EXTENDED_REGS. But unfortunately now, 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 and attr->sample_regs_user.
>
> 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) ]
>
> v2:
> ---
> Rebase to perf/core
>
> 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 | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 9aa51a65593d..11794d3b7879 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
> + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
> + !evsel__is_dummy_event(evsel)) {
hum, I thought it'd look something like this:
if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel))
but I'm not sure how no_aux_samples flag works exactly.. so it might be
correct.. just making sure ;-)
cc-ing Adrian
jirka
> attr->sample_regs_intr = opts->sample_intr_regs;
> evsel__set_sample_bit(evsel, REGS_INTR);
> }
>
> - if (opts->sample_user_regs && !evsel->no_aux_samples) {
> + if (opts->sample_user_regs && !evsel->no_aux_samples &&
> + !evsel__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] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-20 9:17 ` Jiri Olsa
@ 2020-07-22 5:00 ` Jin, Yao
2020-07-22 11:08 ` Jiri Olsa
0 siblings, 1 reply; 8+ messages in thread
From: Jin, Yao @ 2020-07-22 5:00 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/20/2020 5:17 PM, Jiri Olsa wrote:
> On Mon, Jul 20, 2020 at 09:00:13AM +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:
>>
>> /* 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 not be set with
>> PERF_PMU_CAP_EXTENDED_REGS. But unfortunately now, 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 and attr->sample_regs_user.
>>
>> 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) ]
>>
>> v2:
>> ---
>> Rebase to perf/core
>>
>> 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 | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 9aa51a65593d..11794d3b7879 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
>> + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
>> + !evsel__is_dummy_event(evsel)) {
>
> hum, I thought it'd look something like this:
>
> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel))
>
> but I'm not sure how no_aux_samples flag works exactly.. so it might be
> correct.. just making sure ;-)
>
> cc-ing Adrian
>
> jirka
>
>
no_aux_samples is set to false by default and it's only set to true by pt, right?
So most of the time, !evsel->no_aux_samples is always true.
if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) {
attr->sample_regs_intr = opts->sample_intr_regs;
evsel__set_sample_bit(evsel, REGS_INTR);
}
So even if the evsel is dummy event, the condition check is true. :(
Or maybe I misunderstand anything?
Thanks
Jin Yao
>> attr->sample_regs_intr = opts->sample_intr_regs;
>> evsel__set_sample_bit(evsel, REGS_INTR);
>> }
>>
>> - if (opts->sample_user_regs && !evsel->no_aux_samples) {
>> + if (opts->sample_user_regs && !evsel->no_aux_samples &&
>> + !evsel__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] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-22 5:00 ` Jin, Yao
@ 2020-07-22 11:08 ` Jiri Olsa
2020-07-23 1:01 ` Jin, Yao
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2020-07-22 11:08 UTC (permalink / raw)
To: Jin, Yao
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, irogers, Adrian Hunter
On Wed, Jul 22, 2020 at 01:00:03PM +0800, Jin, Yao wrote:
SNIP
> > >
> > > 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 and attr->sample_regs_user.
> > >
> > > 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) ]
> > >
> > > v2:
> > > ---
> > > Rebase to perf/core
> > >
> > > 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 | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index 9aa51a65593d..11794d3b7879 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
> > > + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
> > > + !evsel__is_dummy_event(evsel)) {
> >
> > hum, I thought it'd look something like this:
> >
> > if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel))
> >
> > but I'm not sure how no_aux_samples flag works exactly.. so it might be
> > correct.. just making sure ;-)
> >
> > cc-ing Adrian
> >
> > jirka
> >
> >
>
> no_aux_samples is set to false by default and it's only set to true by pt, right?
>
> So most of the time, !evsel->no_aux_samples is always true.
>
> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) {
> attr->sample_regs_intr = opts->sample_intr_regs;
> evsel__set_sample_bit(evsel, REGS_INTR);
> }
>
> So even if the evsel is dummy event, the condition check is true. :(
>
> Or maybe I misunderstand anything?
I was just curious, because I did not follow the no_aux_samples
usage in detail.. so how about a case where:
evsel->no_aux_samples == true and evsel__is_dummy_event(evsel) = false
then the original condition will be false for non dummy event
(opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel))
is that ok?
jirka
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-22 11:08 ` Jiri Olsa
@ 2020-07-23 1:01 ` Jin, Yao
2020-07-29 7:23 ` Jin, Yao
0 siblings, 1 reply; 8+ messages in thread
From: Jin, Yao @ 2020-07-23 1:01 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, Adrian,
On 7/22/2020 7:08 PM, Jiri Olsa wrote:
> On Wed, Jul 22, 2020 at 01:00:03PM +0800, Jin, Yao wrote:
>
> SNIP
>
>>>>
>>>> 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 and attr->sample_regs_user.
>>>>
>>>> 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) ]
>>>>
>>>> v2:
>>>> ---
>>>> Rebase to perf/core
>>>>
>>>> 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 | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>>> index 9aa51a65593d..11794d3b7879 100644
>>>> --- a/tools/perf/util/evsel.c
>>>> +++ b/tools/perf/util/evsel.c
>>>> @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
>>>> + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
>>>> + !evsel__is_dummy_event(evsel)) {
>>>
>>> hum, I thought it'd look something like this:
>>>
>>> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel))
>>>
>>> but I'm not sure how no_aux_samples flag works exactly.. so it might be
>>> correct.. just making sure ;-)
>>>
>>> cc-ing Adrian
>>>
>>> jirka
>>>
>>>
>>
>> no_aux_samples is set to false by default and it's only set to true by pt, right?
>>
>> So most of the time, !evsel->no_aux_samples is always true.
>>
>> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) {
>> attr->sample_regs_intr = opts->sample_intr_regs;
>> evsel__set_sample_bit(evsel, REGS_INTR);
>> }
>>
>> So even if the evsel is dummy event, the condition check is true. :(
>>
>> Or maybe I misunderstand anything?
>
> I was just curious, because I did not follow the no_aux_samples
> usage in detail.. so how about a case where:
>
> evsel->no_aux_samples == true and evsel__is_dummy_event(evsel) = false
>
> then the original condition will be false for non dummy event
>
> (opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel))
>
> is that ok?
>
I searched the perf source and found the no_aux_samples was only set to true in intel-pt.c. So I
assume for the non-pt usage, the no_aux_samples is always false.
For non-pt usage,
(opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is equal to
(opts->sample_intr_regs && !evsel__is_dummy_event(evsel))
For pt usage, we need to consider the case that evsel__is_dummy_event(evsel) is true or false.
If evsel__is_dummy_event(evsel) is true:
(opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is false.
It's expected.
If evsel__is_dummy_event(evsel) is false:
(opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is equal to
(opts->sample_intr_regs && !evsel->no_aux_samples)
That's the current code logic.
So I think the condition "(opts->sample_intr_regs && !evsel->no_aux_samples &&
!evsel__is_dummy_event(evsel))" looks reasonable.
Adrian, please correct me if I'm wrong here.
Thanks
Jin Yao
> jirka
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-23 1:01 ` Jin, Yao
@ 2020-07-29 7:23 ` Jin, Yao
2020-08-04 7:06 ` Adrian Hunter
0 siblings, 1 reply; 8+ messages in thread
From: Jin, Yao @ 2020-07-29 7:23 UTC (permalink / raw)
To: Jiri Olsa, Adrian Hunter
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, irogers
Hi Adrian,
Could you help to check if following condition will break PT?
"(opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel))"
Thanks
Jin Yao
On 7/23/2020 9:01 AM, Jin, Yao wrote:
> Hi Jiri, Adrian,
>
> On 7/22/2020 7:08 PM, Jiri Olsa wrote:
>> On Wed, Jul 22, 2020 at 01:00:03PM +0800, Jin, Yao wrote:
>>
>> SNIP
>>
>>>>>
>>>>> 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 and attr->sample_regs_user.
>>>>>
>>>>> 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) ]
>>>>>
>>>>> v2:
>>>>> ---
>>>>> Rebase to perf/core
>>>>>
>>>>> 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 | 6 ++++--
>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>>>> index 9aa51a65593d..11794d3b7879 100644
>>>>> --- a/tools/perf/util/evsel.c
>>>>> +++ b/tools/perf/util/evsel.c
>>>>> @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
>>>>> + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
>>>>> + !evsel__is_dummy_event(evsel)) {
>>>>
>>>> hum, I thought it'd look something like this:
>>>>
>>>> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel))
>>>>
>>>> but I'm not sure how no_aux_samples flag works exactly.. so it might be
>>>> correct.. just making sure ;-)
>>>>
>>>> cc-ing Adrian
>>>>
>>>> jirka
>>>>
>>>>
>>>
>>> no_aux_samples is set to false by default and it's only set to true by pt, right?
>>>
>>> So most of the time, !evsel->no_aux_samples is always true.
>>>
>>> if (opts->sample_intr_regs && (!evsel->no_aux_samples || !evsel__is_dummy_event(evsel)) {
>>> attr->sample_regs_intr = opts->sample_intr_regs;
>>> evsel__set_sample_bit(evsel, REGS_INTR);
>>> }
>>>
>>> So even if the evsel is dummy event, the condition check is true. :(
>>>
>>> Or maybe I misunderstand anything?
>>
>> I was just curious, because I did not follow the no_aux_samples
>> usage in detail.. so how about a case where:
>>
>> evsel->no_aux_samples == true and evsel__is_dummy_event(evsel) = false
>>
>> then the original condition will be false for non dummy event
>>
>> (opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel))
>>
>> is that ok?
>>
>
> I searched the perf source and found the no_aux_samples was only set to true in intel-pt.c. So I
> assume for the non-pt usage, the no_aux_samples is always false.
>
> For non-pt usage,
> (opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is equal to
> (opts->sample_intr_regs && !evsel__is_dummy_event(evsel))
>
> For pt usage, we need to consider the case that evsel__is_dummy_event(evsel) is true or false.
>
> If evsel__is_dummy_event(evsel) is true:
> (opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is false.
> It's expected.
>
> If evsel__is_dummy_event(evsel) is false:
> (opts->sample_intr_regs && !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) is equal to
> (opts->sample_intr_regs && !evsel->no_aux_samples)
> That's the current code logic.
>
> So I think the condition "(opts->sample_intr_regs && !evsel->no_aux_samples &&
> !evsel__is_dummy_event(evsel))" looks reasonable.
>
> Adrian, please correct me if I'm wrong here.
>
> Thanks
> Jin Yao
>
>> jirka
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-07-29 7:23 ` Jin, Yao
@ 2020-08-04 7:06 ` Adrian Hunter
2020-08-04 12:06 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2020-08-04 7:06 UTC (permalink / raw)
To: Jin, Yao, Jiri Olsa
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, irogers
On 29/07/20 10:23 am, Jin, Yao wrote:
> Hi Adrian,
>
> Could you help to check if following condition will break PT?
>
> "(opts->sample_intr_regs && !evsel->no_aux_samples &&
> !evsel__is_dummy_event(evsel))"
Sorry for slow response - I've been away.
This is fine. It will not break PT.
no_aux_samples is useful for evsels that have been added by the code rather
than requested by the user. For old kernels PT adds sched_switch tracepoint
to track context switches (before the current context switch event was
added) and having auxiliary sample information unnecessarily uses up space
in the perf buffer.
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
> Thanks
> Jin Yao
>
> On 7/23/2020 9:01 AM, Jin, Yao wrote:
>> Hi Jiri, Adrian,
>>
>> On 7/22/2020 7:08 PM, Jiri Olsa wrote:
>>> On Wed, Jul 22, 2020 at 01:00:03PM +0800, Jin, Yao wrote:
>>>
>>> SNIP
>>>
>>>>>>
>>>>>> 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 and attr->sample_regs_user.
>>>>>>
>>>>>> 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) ]
>>>>>>
>>>>>> v2:
>>>>>> ---
>>>>>> Rebase to perf/core
>>>>>>
>>>>>> 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 | 6 ++++--
>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>>>>> index 9aa51a65593d..11794d3b7879 100644
>>>>>> --- a/tools/perf/util/evsel.c
>>>>>> +++ b/tools/perf/util/evsel.c
>>>>>> @@ -1014,12 +1014,14 @@ 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 && !evsel->no_aux_samples) {
>>>>>> + if (opts->sample_intr_regs && !evsel->no_aux_samples &&
>>>>>> + !evsel__is_dummy_event(evsel)) {
>>>>>
>>>>> hum, I thought it'd look something like this:
>>>>>
>>>>> if (opts->sample_intr_regs && (!evsel->no_aux_samples ||
>>>>> !evsel__is_dummy_event(evsel))
>>>>>
>>>>> but I'm not sure how no_aux_samples flag works exactly.. so it might be
>>>>> correct.. just making sure ;-)
>>>>>
>>>>> cc-ing Adrian
>>>>>
>>>>> jirka
>>>>>
>>>>>
>>>>
>>>> no_aux_samples is set to false by default and it's only set to true by
>>>> pt, right?
>>>>
>>>> So most of the time, !evsel->no_aux_samples is always true.
>>>>
>>>> if (opts->sample_intr_regs && (!evsel->no_aux_samples ||
>>>> !evsel__is_dummy_event(evsel)) {
>>>> attr->sample_regs_intr = opts->sample_intr_regs;
>>>> evsel__set_sample_bit(evsel, REGS_INTR);
>>>> }
>>>>
>>>> So even if the evsel is dummy event, the condition check is true. :(
>>>>
>>>> Or maybe I misunderstand anything?
>>>
>>> I was just curious, because I did not follow the no_aux_samples
>>> usage in detail.. so how about a case where:
>>>
>>> evsel->no_aux_samples == true and evsel__is_dummy_event(evsel) = false
>>>
>>> then the original condition will be false for non dummy event
>>>
>>> (opts->sample_intr_regs && !evsel->no_aux_samples &&
>>> !evsel__is_dummy_event(evsel))
>>>
>>> is that ok?
>>>
>>
>> I searched the perf source and found the no_aux_samples was only set to
>> true in intel-pt.c. So I assume for the non-pt usage, the no_aux_samples
>> is always false.
>>
>> For non-pt usage,
>> (opts->sample_intr_regs && !evsel->no_aux_samples &&
>> !evsel__is_dummy_event(evsel)) is equal to
>> (opts->sample_intr_regs && !evsel__is_dummy_event(evsel))
>>
>> For pt usage, we need to consider the case that
>> evsel__is_dummy_event(evsel) is true or false.
>>
>> If evsel__is_dummy_event(evsel) is true:
>> (opts->sample_intr_regs && !evsel->no_aux_samples &&
>> !evsel__is_dummy_event(evsel)) is false.
>> It's expected.
>>
>> If evsel__is_dummy_event(evsel) is false:
>> (opts->sample_intr_regs && !evsel->no_aux_samples &&
>> !evsel__is_dummy_event(evsel)) is equal to
>> (opts->sample_intr_regs && !evsel->no_aux_samples)
>> That's the current code logic.
>>
>> So I think the condition "(opts->sample_intr_regs &&
>> !evsel->no_aux_samples && !evsel__is_dummy_event(evsel))" looks reasonable.
>>
>> Adrian, please correct me if I'm wrong here.
>>
>> Thanks
>> Jin Yao
>>
>>> jirka
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event
2020-08-04 7:06 ` Adrian Hunter
@ 2020-08-04 12:06 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-08-04 12:06 UTC (permalink / raw)
To: Adrian Hunter
Cc: Jin, Yao, Jiri Olsa, jolsa, peterz, mingo, alexander.shishkin,
Linux-kernel, ak, kan.liang, yao.jin, irogers
Em Tue, Aug 04, 2020 at 10:06:56AM +0300, Adrian Hunter escreveu:
> On 29/07/20 10:23 am, Jin, Yao wrote:
> > Hi Adrian,
> >
> > Could you help to check if following condition will break PT?
> >
> > "(opts->sample_intr_regs && !evsel->no_aux_samples &&
> > !evsel__is_dummy_event(evsel))"
>
> Sorry for slow response - I've been away.
>
> This is fine. It will not break PT.
>
> no_aux_samples is useful for evsels that have been added by the code rather
> than requested by the user. For old kernels PT adds sched_switch tracepoint
> to track context switches (before the current context switch event was
> added) and having auxiliary sample information unnecessarily uses up space
> in the perf buffer.
>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Thanks for checking and providing the comment, that I added as a
committer note together with your Acked-by, appreciated.
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-04 12:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 1:00 [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event Jin Yao
2020-07-20 9:17 ` Jiri Olsa
2020-07-22 5:00 ` Jin, Yao
2020-07-22 11:08 ` Jiri Olsa
2020-07-23 1:01 ` Jin, Yao
2020-07-29 7:23 ` Jin, Yao
2020-08-04 7:06 ` Adrian Hunter
2020-08-04 12:06 ` 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).