Re: [PATCH v2] perf evsel: Don't set sample_regs_intr/sample_regs_user for dummy event

[Jiri Olsa]

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@xxxxxxxxxxxxxxx>
> ---
>  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
>