Hi Jiri,
On Wed, 5 Feb 2014 15:33:29 +0100, Jiri Olsa wrote:
> On Wed, Feb 05, 2014 at 10:27:30AM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>>
>> On Mon, 3 Feb 2014 12:44:41 +0100, Jiri Olsa wrote:
>> > We use PERF_SAMPLE_PERIOD sample type only for frequency
>> > setup -F (default) option. The -c does not need store period,
>> > because it's always the same.
>> >
>> > In -c case the report code uses '1' as period. Fixing
>> > it to perf_event_attr::sample_period.
>>
>> All 3 patches look good. But I found something strange. When we
>> setup/config evsel attrs following code is used:
>>
>> util/evsel.c::perf_evsel__config()
>>
>> /*
>> * We default some events to a 1 default interval. But keep
>> * it a weak assumption overridable by the user.
>> */
>> if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
>> opts->user_interval != ULLONG_MAX)) {
>> if (opts->freq) {
>> perf_evsel__set_sample_bit(evsel, PERIOD);
>> attr->freq = 1;
>> attr->sample_freq = opts->freq;
>> } else {
>> attr->sample_period = opts->default_interval;
>> }
>> }
>
> yes, I think thats right.. we should use || instead of &&
>
> It will allow to change period for event types with predefined
> attr->sample_period like tracepoints.
Right. As I read the code, it works "if (!attr->sample_period)" case only.
>
> However, I tried with tracepoints and even with this fix
> and following command line:
>
> # perf record -e syscalls:sys_enter_read -c 2 ls
>
> you'll still get samples with period 1. The reason is in
> kernel code:
>
> static void perf_swevent_event(struct perf_event *event, u64 nr,
> struct perf_sample_data *data,
> struct pt_regs *regs)
> {
> ...
> if ((event->attr.sample_type & PERF_SAMPLE_PERIOD) &&
> !event->attr.freq) {
> data->period = nr;
> return perf_swevent_overflow(event, 1, data, regs);
>
> bacause above condition is true for tracepoints.
>
> It looks like a bug, but I'm not sure how handy it'd be
> set period other than 1 for tracepoints thought.. ;)
Agreed. But at least we should support whatever user wants IMHO..
>
> Maybe it's not that big issue in comparison of screwing
> up other software events processing.
:)
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
