linux-kernel - Re: [PATCH 3/3] perf events: Change perf parameter --pid to process-wide collection instead of thread-wide

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 18 Mar 2010 10:35:48 -0300
From:	Arnaldo Carvalho de Melo <acme@...radead.org>
To:	"Zhang, Yanmin" <yanmin_zhang@...ux.intel.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Avi Kivity <avi@...hat.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, Sheng Yang <sheng@...ux.intel.com>,
	Joerg Roedel <joro@...tes.org>,
	Jes Sorensen <Jes.Sorensen@...hat.com>,
	Marcelo Tosatti <mtosatti@...hat.com>,
	Gleb Natapov <gleb@...hat.com>, kvm@...r.kernel.org,
	zhiteng.huang@...el.com, Zachary Amsden <zamsden@...hat.com>
Subject: Re: [PATCH 3/3] perf events: Change perf parameter --pid to
 process-wide collection instead of thread-wide

Em Thu, Mar 18, 2010 at 05:31:06PM +0800, Zhang, Yanmin escreveu:
> From: Zhang, Yanmin <yanmin_zhang@...ux.intel.com>
> 
> Parameter --pid (or -p) of perf currently means a thread-wide collection.
> For exmaple, if a process whose id is 8888 has 10 threads, 'perf top -p 8888'
> just collects the main thread statistics. That's misleading. Users are
> used to attach a whole process when debugging a process by gdb. To follow
> normal usage style, the patch change --pid to process-wide collection and
> add --tid (-t) to mean a thread-wide collection.
> 
> Usage example is:
> #perf top -p 8888
> #perf record -p 8888 -f sleep 10
> #perf stat -p 8888 -f sleep 10
> Above commands collect the statistics of all threads of process 8888.
> 
> Signed-off-by: Zhang Yanmin <yanmin_zhang@...ux.intel.com>

Just did visual inspection of the three patches, all sane, except for
some coding style nits, don't worry right now for that, I'll fix them up
myself, but please take those into account int the future, highlight
below.
 
> ---
> 
> diff -Nraup linux-2.6_tip0317_statrecord/tools/perf/builtin-record.c linux-2.6_tip0317_statrecordpid/tools/perf/builtin-record.c
> --- linux-2.6_tip0317_statrecord/tools/perf/builtin-record.c	2010-03-18 13:48:39.578181540 +0800
> +++ linux-2.6_tip0317_statrecordpid/tools/perf/builtin-record.c	2010-03-18 14:28:41.449631936 +0800
> +			mmap_array[nr_cpu][counter][thread_index].mask = mmap_pages*page_size - 1;
> +			mmap_array[nr_cpu][counter][thread_index].base = mmap(NULL, (mmap_pages+1)*page_size,

Spaces around +, *,  etc

> +				PROT_READ|PROT_WRITE, MAP_SHARED, fd[nr_cpu][counter][thread_index], 0);
> +			if (mmap_array[nr_cpu][counter][thread_index].base == MAP_FAILED) {
> +				error("failed to mmap with %d (%s)\n", errno, strerror(errno));
> +				exit(-1);
> +			}
> +	} else {
> +		all_tids=malloc(sizeof(pid_t));

Ditto here for =

> +		if (!all_tids)
> +			return -ENOMEM;
> +
> +		all_tids[0] = target_tid;
> +		thread_num = 1;
> +	}
> +
> +	for (i = 0; i < MAX_NR_CPUS; i++) {
> +		for (j = 0; j < MAX_COUNTERS; j++) {
> +			fd[i][j] = malloc(sizeof(int)*thread_num);
> +			mmap_array[i][j] = malloc(
> +				sizeof(struct mmap_data)*thread_num);

Ditto

> +			if (!fd[i][j] || !mmap_array[i][j])
> +				return -ENOMEM;
> +		}
> +	}
> +	event_array = malloc(
> +		sizeof(struct pollfd)*MAX_NR_CPUS*MAX_COUNTERS*thread_num);

Ditto

Should be, I suggest:

	event_array = malloc((sizeof(struct pollfd) * MAX_NR_CPUS *
			      MAX_COUNTERS * thread_num));


Anyway, I'll fix some of these while merging, now.

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