Re: [PATCH 04/10] perf record: Filter out POLLHUP'ed file descriptors

[Jiri Olsa]

On Mon, Sep 08, 2014 at 11:33:17AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> 
> So what you're saying is that we should have been polling all the fds
> all the time?
> 
> Because after all we end up trying to consume everything in all the ring
> buffers when just one of them gets a POLLRD anyway...
> 
> > > I'll look at the kernel code for that...
> > >  
> > > > > I.e. when we ask the kernel to point fd B to fd A's mmap (what you
> > > > > called set-output) and fd B inserts an event into fd A's mmap ring
> > > > > buffer, we get fd A poll return as POLLRD, no?
> > > 
> > > > > Have to check... Otherwise we would have to poll all fds all the time,
> > > > > not just the ones mmaping, right?
> > > 
> > > > > > I think Jiri's patchset changed pollfds to include all fds for that reason.
> > > 
> > > > hm, I did not think of that.. ;-) I needed more grained feedback
> > > > for future features like cpu hotplug
> > > 
> > > So this is good for something you didn't tried to fix (and document) but
> > > good for something that may be nice in the future? Grumpf, we have
> > > already way too much stuff that will be eventually used but is not used
> > > right now :-\
>  
> > IMO it's more clear to poll pm all event FDs.. and now with the
> > case Adrian described it seems necessary anyway
> 
> I would have to check why was that we were polling just the one where
> the mmap is done, I don't recall being the one to do it, probably who
> did it thought that since the ring buffer is there, it was enough (and
> possibly scaled better, dunno) to do the polling in just one of them.

for read notification it's ok to poll just for one event, because they
all share same ringbuffer and perf_poll checks if there's ANY new data

for the hup notification I think we need to poll all of them

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/