Messages in this thread | | | From | Stephane Eranian <> | Date | Tue, 30 Jan 2018 08:36:45 -0800 | Subject | Re: [PATCH V3 0/5] bugs fix for large PEBS mmap read and rdpmc read |
| |
On Tue, Jan 30, 2018 at 7:25 AM, Liang, Kan <kan.liang@linux.intel.com> wrote: > > > On 1/30/2018 10:04 AM, Jiri Olsa wrote: >> >> 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 >> > > I mean the TERM_PERIOD. It probably enable the large PEBS. >>>> attr->freq = 0; >>>> + perf_evsel__reset_sample_bit(evsel, PERIOD); > > The events in fixed mode could enable large PEBS. Events in freq mode should > not enable large PEBS. > I think that could be a problem if some events try to enable large PEBS, > while others not. > You only enable large PEBS if 100% of the events use fixed periods, either via -c period or because they all use individual period=p. The --no-period could also be used to remove the period for measurements where the period is not needed.
> Thanks, > Kan > >
| |