Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[Namhyung Kim <namhyung@kernel.org>]

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


But shouldn't it be "||" instead of "&&" for checking
user_freq/interval?  The opts->freq was setup by code below

  util/record.c::record_opts__config_freq()

	bool user_freq = opts->user_freq != UINT_MAX;
	unsigned int max_rate;

	if (opts->user_interval != ULLONG_MAX)
		opts->default_interval = opts->user_interval;
	if (user_freq)
		opts->freq = opts->user_freq;

	/*
	 * User specified count overrides default frequency.
	 */
	if (opts->default_interval)
		opts->freq = 0;
	else if (opts->freq) {
		opts->default_interval = opts->freq;
	} else {
		pr_err("frequency and count are zero, aborting\n");
		return -1;
	}


Thanks,
Namhyung