| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Jan 2018 09:59:15 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>, Stephane Eranian <eranian@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH V3 0/5] bugs fix for large PEBS mmap read and rdpmc read
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@...ux.intel.com> wrote:
>>>
>>> From: Kan Liang <kan.liang@...ux.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)
>
Powered by blists - more mailing lists