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 10:25:27 -0500 |
| |
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.
Thanks, Kan
| |