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
| ||
|
Date: Thu, 7 Dec 2017 22:03:55 +0800 From: zhangmengting <zhangmengting@...wei.com> To: Jiri Olsa <jolsa@...hat.com> CC: <linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <acme@...hat.com>, <huawei.libin@...wei.com>, <wangnan0@...wei.com>, <cj.chengjian@...wei.com> Subject: Re: [PATCH] perf evsel: Enable ignore_missing_thread for pid option Hi Jiri, Thanks for your review! I've sent a patch V2 to address these issues. On 2017/12/6 20:59, Jiri Olsa wrote: > On Tue, Dec 05, 2017 at 05:03:33PM +0800, Mengting Zhang wrote: >> While monitoring a multithread process with pid option, perf sometimes >> may return sys_perf_event_open failure with 3(No such process) if any >> of the process's threads die before we open the event. However, we want >> perf continue monitoring the remaining threads and do not exit with error. >> >> Here, the patch enables perf_evsel::ignore_missing_thread for -p option >> to ignore complete failure if any of threads die before we open the event. >> But it may still return sys_perf_event_open failure with 22(Invalid) if we >> monitors several event groups. >> >> sys_perf_event_open: pid 28960 cpu 40 group_fd 118202 flags 0x8 >> sys_perf_event_open: pid 28961 cpu 40 group_fd 118203 flags 0x8 >> WARNING: Ignored open failure for pid 28962 >> sys_perf_event_open: pid 28962 cpu 40 group_fd [118203] flags 0x8 >> sys_perf_event_open failed, error -22 >> >> That is because when we ignore a missing thread, we change the thread_idx >> without dealing with its fds, FD(evsel, cpu, thread). Then get_group_fd() >> may return a wrong group_fd for the next thread and sys_perf_event_open() >> return with 22. > oops, nice catch > > SNIP > >> +static int group_fd__remove(struct perf_evsel *evsel, >> + int nr_cpus, int cpu_idx, >> + int nr_threads, int thread_idx) > please call this something more generic like update_fds, > I think it affects more stuff than just group_fds Yeah, not just change the group_fds. It affects fds related with the missing thread. > >> +{ >> + struct perf_evsel *pos; >> + struct perf_evlist *evlist = evsel->evlist; >> + >> + if (nr_cpus < 1 || nr_threads < 1) >> + return -EINVAL; > we already have check for threads->nr == 1 in ignore_missing_thread > also not sure how possible is to get nr_cpus < 1, but ok Yes, this condition seems redundant. I will remove this condition. >> + >> + if (cpu_idx >= nr_cpus || thread_idx >= nr_threads) >> + return -EINVAL; >> + >> + evlist__for_each_entry(evlist, pos) { >> + if (pos != evsel) { >> + for (int cpu = 0; cpu < nr_cpus; cpu++) >> + for (int thread = thread_idx; thread < nr_threads; thread++) >> + FD(pos, cpu, thread) = FD(pos, cpu, thread + 1); >> + } >> + else { >> + for (int cpu = 0; cpu < cpu_idx; cpu++) >> + for (int thread = thread_idx; thread < nr_threads; thread++) >> + FD(pos, cpu, thread) = FD(pos, cpu, thread + 1); >> + break; >> + } >> + } > could you please put this into some generic function, like: > > void perf_evsel__remove_thread(evsel, nr_cpus, nr_threads, int thread_idx) > { > for (int cpu = 0; cpu < nr_cpus; cpu++) > for (int thread = thread_idx; thread < nr_threads; thread++) > FD(pos, cpu, thread) = FD(pos, cpu, thread + 1); > } > > > with the loop would be like: > > evlist__for_each_entry(evlist, pos) { > int nr_cpus = pos != evsel ? nr_cpus : cpu_idx; > > perf_evsel__remove_thread(evsel, nr_cpus, nr_threads, thread_idx) > } > > or something along those lines... That looks much nicer, just like literate programming! > thanks for catching this > > jirka > > . >
Powered by blists - more mailing lists