Messages in this thread | | | Date | Fri, 05 Sep 2014 11:42:59 +0300 | From | Adrian Hunter <> | Subject | Re: [PATCH 04/10] perf record: Filter out POLLHUP'ed file descriptors |
| |
On 09/04/2014 06:19 PM, Arnaldo Carvalho de Melo wrote: > Em Thu, Sep 04, 2014 at 03:32:08PM +0300, Adrian Hunter escreveu: >> On 09/04/2014 12:59 AM, Arnaldo Carvalho de Melo wrote: >>> So that we don't continue polling on vanished file descriptors, i.e. >>> file descriptors for events monitoring threads that exited. > >>> I.e. the following 'perf record' command now exits as expected, instead >>> of staying in an eternal loop: > >>> $ sleep 5s & >>> $ perf record -p `pidof sleep` > >>> Reported-by: Jiri Olsa <jolsa@redhat.com> > <SNIP> >>> +++ b/tools/perf/builtin-record.c >>> @@ -467,6 +467,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) >>> if (err > 0 || (err < 0 && errno == EINTR)) >>> err = 0; >>> waking++; >>> + if (perf_evlist__filter_pollfd(rec->evlist, POLLERR | POLLHUP) == 0) >> >> If the poll fds only include the ones mmapped, then mightn't it filter out >> ones still in use? e.g. what if you record two processes and one exits? > > Humm, without looking at the code, we would be needlessly looking at the > mmaps for the threads that exited. > > I.e. we need to associate at the fdarray level the mmaps for the > descriptors, so that we can, at fdarray__filter() time, munmap the > POLLHUPed descriptors, right? > > I'll take a look at the code to see what is needed. > > But then this makes the patch set good for a class of problems while > maintaining existing behaviour for the case you outlined, i.e. this is a > partial fix and should go now, with further patches on top of it?
No I was meaning something different. For example, 'perf record' opens an event for 2 processes per-cpu and gets 4 file descriptors:
task1 task2 cpu0 fd0 fd1 cpu1 fd2 fd3
Now, perf record will mmap fd0 and fd2 and set-output fd1->fd0 and fd3->fd2.
pollfds includes only fd0 and fd2.
But if task2 exits, the POLLHUP will appear on fd1 and fd3.
I think Jiri's patchset changed pollfds to include all fds for that reason.
| |