Messages in this thread |  | | Subject | Re: [PATCH V3 0/5] bugs fix for large PEBS mmap read and rdpmc read | From | "Liang, Kan" <> | Date | Tue, 30 Jan 2018 09:59:15 -0500 |
| |
On 1/30/2018 8:39 AM, Jiri Olsa wrote: > On Tue, Jan 30, 2018 at 01:16:39AM -0800, Stephane Eranian wrote: >> Hi, >> >> On Mon, Jan 29, 2018 at 8:29 AM, <kan.liang@linux.intel.com> wrote: >>> >>> From: Kan Liang <kan.liang@linux.intel.com> >>> >>> ------ >>> >>> Changes since V2: >>> - Refined the changelog >>> - Introduced specific read function for large PEBS. >>> The previous generic PEBS read function is confusing. >>> Disabled PMU in pmu::read() path for large PEBS. >>> Handled the corner case when reload_times == 0. >>> - Modified the parameter of intel_pmu_save_and_restart_reload() >>> Discarded local64_cmpxchg >>> - Added fixes tag >>> - Added WARN to handle reload_times == 0 || reload_val == 0 >>> >>> Changes since V1: >>> - Check PERF_X86_EVENT_AUTO_RELOAD before call >>> intel_pmu_save_and_restore() >> >> It is not yet clear to me why PERF_SAMPLE_PERIOD is not allowed >> with large PEBS. Large PEBS requires fixed period. So the kernel could >> make up the period from the event and store it in the sampling buffer. >> >> I tried using large PEBS recently, and despite trying different option >> combination of perf record, I was not able to get it to work. >> >> $ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp >> --no-timestamp --no-period -a -C 0 >> >> But I was able to make this work with a much older kernel. >> >> Another annoyance I ran into is with perf record requiring -c period >> in order not to set >> PERF_SAMPLE_PERIOD in the event. >> >> If I do: >> perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp >> --no-timestamp --no-period -a -C 0 >> >> I get >> >> perf_event_attr: >> type 4 >> size 112 >> config 0x10d1 >> { sample_period, sample_freq } 199936 >> sample_type IP|TID|CPU >> >> But if I do: >> perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp >> --no-timestamp --no-period -a -C 0 >> >> I get >> >> perf_event_attr: >> type 4 >> size 112 >> config 0x10d1 >> { sample_period, sample_freq } 199936 >> sample_type IP|TID|CPU|PERIOD >> >> Perf should check if all events have a period=, then it should not >> pass PERF_SAMPLE_PERIOD, even >> more so when only one event is defined. >> >> Also it does not seem to honor --no-period. > > yep, there's a bug in period=x term handling > we did not re/set the sample_type based on that > > attached patch fixes that for me, also takes into account > the --no/-period options > > jirka > > > --- > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index f251e824edac..907267206973 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1566,7 +1566,8 @@ static struct option __record_options[] = { > OPT_BOOLEAN_SET('T', "timestamp", &record.opts.sample_time, > &record.opts.sample_time_set, > "Record the sample timestamps"), > - OPT_BOOLEAN('P', "period", &record.opts.period, "Record the sample period"), > + OPT_BOOLEAN_SET('P', "period", &record.opts.period, &record.opts.period_set, > + "Record the sample period"), > OPT_BOOLEAN('n', "no-samples", &record.opts.no_samples, > "don't sample"), > OPT_BOOLEAN_SET('N', "no-buildid-cache", &record.no_buildid_cache, > diff --git a/tools/perf/perf.h b/tools/perf/perf.h > index 2357f4ccc9c7..cfe46236a5e5 100644 > --- a/tools/perf/perf.h > +++ b/tools/perf/perf.h > @@ -50,6 +50,7 @@ struct record_opts { > bool sample_time_set; > bool sample_cpu; > bool period; > + bool period_set; > bool running_time; > bool full_auxtrace; > bool auxtrace_snapshot_mode; > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 66fa45198a11..ff359c9ece2e 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel, > if (!(term->weak && opts->user_interval != ULLONG_MAX)) { > attr->sample_period = term->val.period; > attr->freq = 0; > + perf_evsel__reset_sample_bit(evsel, PERIOD); > } > break; > case PERF_EVSEL__CONFIG_TERM_FREQ: > if (!(term->weak && opts->user_freq != UINT_MAX)) { > attr->sample_freq = term->val.freq; > attr->freq = 1; > + perf_evsel__set_sample_bit(evsel, PERIOD); > }
If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set. That could be a problem for large PEBS. The PEBS buffer is shared among events. It doesn't support freq mode yet.
Thanks, Kan
> break; > case PERF_EVSEL__CONFIG_TERM_TIME: > @@ -969,9 +971,6 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, > if (target__has_cpu(&opts->target) || opts->sample_cpu) > perf_evsel__set_sample_bit(evsel, CPU); > > - if (opts->period) > - perf_evsel__set_sample_bit(evsel, PERIOD); > - > /* > * When the user explicitly disabled time don't force it here. > */ > @@ -1073,6 +1072,14 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, > apply_config_terms(evsel, opts, track); > > evsel->ignore_missing_thread = opts->ignore_missing_thread; > + > + /* The --period option takes the precedence. */ > + if (opts->period_set) { > + if (opts->period) > + perf_evsel__set_sample_bit(evsel, PERIOD); > + else > + perf_evsel__reset_sample_bit(evsel, PERIOD); > + } > } > > static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads) >
|  |