linux-kernel - Re: [PATCH v2] perf evsel: Enable ignore_missing

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] [day] [month] [year] [list]

Message-ID: <20171208115453.GA7889@krava>
Date:   Fri, 8 Dec 2017 12:54:53 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Mengting Zhang <zhangmengting@...wei.com>
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        acme@...hat.com, huawei.libin@...wei.com, wangnan0@...wei.com,
        cj.chengjian@...wei.com
Subject: Re: [PATCH v2] perf evsel: Enable ignore_missing_thread for pid
 option

On Thu, Dec 07, 2017 at 09:43:33PM +0800, Mengting Zhang wrote:

SNIP

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index f894893..d0ef889 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1592,10 +1592,43 @@ static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
>  	return fprintf(fp, "  %-32s %s\n", name, val);
>  }
>  
> +static void perf_evsel__remove_fd(struct perf_evsel *pos,
> +				  int nr_cpus, int nr_threads,
> +				  int thread_idx)
> +{
> +	for (int cpu = 0; cpu < nr_cpus; cpu++)
> +		for (int thread = thread_idx; thread < nr_threads - 1; thread++)
> +			FD(pos, cpu, thread) = FD(pos, cpu, thread + 1);
> +}
> +
> +static int perf_evlist__update_fds(struct perf_evsel *evsel,
> +				   int nr_cpus, int cpu_idx,
> +				   int nr_threads, int thread_idx)

we use '__' to delimit the object, so you'd need to call this
function with perf_evlist as a first argument

I think 'update_fds' name would be ok

> +{
> +	struct perf_evsel *pos;
> +	struct perf_evlist *evlist = evsel->evlist;
> +
> +	if (cpu_idx >= nr_cpus || thread_idx >= nr_threads)
> +		return -EINVAL;
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		nr_cpus = pos != evsel ? nr_cpus : cpu_idx;
> +
> +		perf_evsel__remove_fd(pos, nr_cpus, nr_threads, thread_idx);
> +

could you please add comment in here explaining why we
don't iterate whole list.. it's clear now, but in future
after more changes in here could be pita ;-)

> +		if (pos == evsel)
> +			break;
> +	}
> +	return 0;
> +}
> +
>  static bool ignore_missing_thread(struct perf_evsel *evsel,
> +				  int nr_cpus, int cpu,
>  				  struct thread_map *threads,
>  				  int thread, int err)
>  {
> +	pid_t ignore_pid = thread_map__pid(threads, thread);
> +
>  	if (!evsel->ignore_missing_thread)
>  		return false;
>  
> @@ -1611,11 +1644,17 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
>  	if (threads->nr == 1)
>  		return false;
>  
> +	/* We should remove fd for missing_thread first
> +	 * because thread_map__remove() will decrease threads->nr.
> +	 */

there's some guide for the multiline comment style,
I think you should put it like:

	/* 
	 * We should remove fd for missing_thread first
	 * because thread_map__remove() will decrease threads->nr.
	 */

thanks,
jirka