Re: [PATCH 08/17] perf record: Don't poll on overwrite channel

[Wangnan (F)]

On 2016/5/13 21:12, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 13, 2016 at 07:56:05AM +0000, Wang Nan escreveu:
> > There's no need to receive events from overwritable ring buffer. Instead,
> > perf should make them run background until something happen. This patch
> > makes normal events from overwrite ring buffer ignored.
> >
> > Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> > Signed-off-by: He Kuang <hekuang@xxxxxxxxxx>
> > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Cc: Zefan Li <lizefan@xxxxxxxxxx>
> > Cc: pi3orama@xxxxxxx
> > ---
> >   tools/perf/util/evlist.c | 23 +++++++++++++++++++----
> >   1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index abce588..f0b0457 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -461,9 +461,9 @@ int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
> >   	return 0;
> >   }
> >   
> > -static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx)
> > +static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx, short revent)
> >   {
> > -	int pos = fdarray__add(&evlist->pollfd, fd, POLLIN | POLLERR | POLLHUP);
> > +	int pos = fdarray__add(&evlist->pollfd, fd, revent | POLLERR | POLLHUP);
> >   	/*
> >   	 * Save the idx so that when we filter out fds POLLHUP'ed we can
> >   	 * close the associated evlist->mmap[] entry.
> > @@ -479,7 +479,7 @@ static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx
> >   
> >   int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd)
> >   {
> > -	return __perf_evlist__add_pollfd(evlist, fd, -1);
> > +	return __perf_evlist__add_pollfd(evlist, fd, -1, POLLIN);
> >   }
> >   
> >   static void perf_evlist__munmap_filtered(struct fdarray *fda, int fd)
> > @@ -1077,6 +1077,18 @@ perf_evlist__channel_complete(struct perf_evlist *evlist)
> >   	return 0;
> >   }
> >   
> > +static bool
> > +perf_evlist__should_poll(struct perf_evlist *evlist,
> > +			 struct perf_evsel *evsel,
> > +			 int channel)
> > +{
> > +	if (evsel->system_wide)
> > +		return false;
> So, what is the above doing in this patch? If we should not poll when in
> syswide mode, then this should be in a separate patch, unrelated to
> 'channels'.  No?

I think the name 'system_wide' is more or less missleading. It is not means
an event in 'perf record -a', but means "a selected event to be opened 
always
without a pid when configured by perf_evsel__config().". See bf8e8f4b8.

Here we use similary logic in existing perf_evlist__mmap_per_evsel. It never
poll system_wide evsel:

                /*
                 * The system_wide flag causes a selected event to be 
opened
                 * always without a pid.  Consequently it will never get a
                 * POLLHUP, but it is used for tracking in combination with
                 * other events, so it should not need to be polled anyway.
                 * Therefore don't add it for polling.
                 */
                if (!evsel->system_wide &&
                    __perf_evlist__add_pollfd(evlist, fd, idx) < 0) {
                        perf_evlist__mmap_put(evlist, idx);
                        return -1;
                }

Thank you.