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

["Liang, Kan" <kan.liang@linux.intel.com>]

On 1/30/2018 11:36 AM, Stephane Eranian wrote:
> 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.


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