Messages in this thread | | | Date | Tue, 30 Jan 2018 16:04:54 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH V3 0/5] bugs fix for large PEBS mmap read and rdpmc read |
| |
On Tue, Jan 30, 2018 at 09:59:15AM -0500, Liang, Kan wrote: > > > 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.
it also sets the attr->freq, so it's not in fixed mode
jirka
| |