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 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