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

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);
 }