Re: [PATCH V3 0/5] bugs fix for large PEBS mmap read and rdpmc read

From: Liang, Kan
Date: Tue Jan 30 2018 - 11:48:32 EST




On 1/30/2018 11:36 AM, Stephane Eranian wrote:
On Tue, Jan 30, 2018 at 7:25 AM, Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:


From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>

------

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.


Oh, right, the kernel has already guaranteed that.
if (cpuc->n_pebs == cpuc->n_large_pebs) {
threshold = ds->pebs_absolute_maximum -
x86_pmu.max_pebs_events * x86_pmu.pebs_record_size;
} else {

Sorry for the noise.

jirka's patch looks good to me.

Thanks,
Kan