Messages in this thread | | | From | Namhyung Kim <> | Subject | Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type | Date | Thu, 06 Feb 2014 15:24:31 +0900 |
| |
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
| |