linux-kernel - Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87vbwu1h8d.fsf@sejong.aot.lge.com>
Date:	Wed, 05 Feb 2014 10:27:30 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	David Ahern <dsahern@...il.com>, Ingo Molnar <mingo@...e.hu>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Andi Kleen <andi@...stfloor.org>,
	Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/