Re: [RFC] Don't print sample_type bits in non-group events not set in the group's was Re: [PATCH] perf, script: Fix crash with printing mixed trace point and other events

[Jiri Olsa <jolsa@redhat.com>]

On Sat, Jan 19, 2019 at 04:37:45PM +0100, Jiri Olsa wrote:
> On Fri, Jan 18, 2019 at 08:11:42AM -0800, Andi Kleen wrote:
> > > +static bool perf_evsel__should_skip(struct perf_evsel *evsel)
> > > +{
> > > +	struct perf_event_attr *attr = &evsel->attr;
> > > +	struct perf_evsel *leader = evsel->leader;
> > > +
> > > +	return (leader != evsel) && !attr->freq && !attr->sample_freq;
> > > +}
> > > +
> > >  static int process_sample_event(struct perf_tool *tool,
> > >  				union perf_event *event,
> > >  				struct perf_sample *sample,
> > > @@ -1934,6 +1942,9 @@ static int process_sample_event(struct perf_tool *tool,
> > >  	struct perf_script *scr = container_of(tool, struct perf_script, tool);
> > >  	struct addr_location al;
> > >  
> > > +	if (perf_evsel__should_skip(evsel))
> > > +		return 0;
> > 
> > That just skips, but surely it has to be displayed somewhere?
> 
> actually maybe it's better to keep the current output,
> and sort out the output for those slave events
> 
> with attached patch I can do:
> 
>   # ./perf script -Ftrace:+period,-cpu
>               ex  5535 546813.189028:      11069 cpu/cpu-cycles,period=10000/:  ffffffffa492ad90 _raw_spin_lock+0x10 ([kernel.kallsyms])
>               ex  5535 546813.189028:        101                probe_ex:main: 
>               ex  5535 546813.189034:       9074 cpu/cpu-cycles,period=10000/:  ffffffffa4928066 down_read+0x6 ([kernel.kallsyms])
>               ex  5535 546813.189041:      10102 cpu/cpu-cycles,period=10000/:  ffffffffa492af22 _raw_spin_lock_irqsave+0x22 ([kernel.kallsyms])
>               ex  5535 546813.189041:          1                probe_ex:main: 
> 
> also now it won't make sample for slave events
> with zero value/period read
> 
> note the patch needs to be split into more patches,
> sending it all together for discussion over the solution

any feedback on this one?

jirka

> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 0c5dc4a52fc0..c8791d6bdf8e 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2726,6 +2726,10 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
>  			pr_warning("Overriding previous field request for %s events.\n",
>  				   event_type(type));
>  
> +		/* Don't override defaults for +- */
> +		if (strchr(tok, '+') || strchr(tok, '-'))
> +			goto parse;
> +
>  		output[type].fields = 0;
>  		output[type].user_set = true;
>  		output[type].wildcard_set = false;
> @@ -2810,6 +2814,10 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
>  				rc = -EINVAL;
>  				goto out;
>  			}
> +			if (change == REMOVE)
> +				output[type].fields &= ~all_output_options[i].field;
> +			else
> +				output[type].fields |= all_output_options[i].field;
>  			output[type].user_set = true;
>  			output[type].wildcard_set = true;
>  		}
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index dbc0466db368..7487b8f7ff96 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -956,6 +956,15 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
>  		attr->sample_freq    = 0;
>  		attr->sample_period  = 0;
>  		attr->write_backward = 0;
> +
> +		/*
> +		 * We don't get sample for slave events, so let's
> +		 * have them following the master sample_type to
> +		 * make it easy during report.
> +		 */
> +		attr->sample_type |= leader->attr.sample_type;
> +		perf_evsel__set_sample_bit(evsel, PERIOD);
> +		perf_evsel__reset_sample_bit(evsel, READ);
>  	}
>  
>  	if (opts->no_samples)
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index d6f41611f504..0cbb2a2fb942 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1188,6 +1188,13 @@ static int deliver_sample_value(struct perf_evlist *evlist,
>  		return 0;
>  	}
>  
> +	/*
> +	 * There's no reason to deliver sample
> +	 * for zero period, bail out.
> +	 */
> +	if (!sample->period)
> +		return 0;
> +
>  	return tool->sample(tool, event, sample, sid->evsel, machine);
>  }
>