Re: [PATCH v2] perf evsel: Enable ignore_missing_thread for pid option

[Jiri Olsa]

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