Re: [PATCH RFC] hist lookups

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Nov 19, 2018 at 10:12:57AM +0100, Jiri Olsa wrote:
> On Mon, Nov 19, 2018 at 02:26:03PM +0900, Namhyung Kim wrote:
> > Hi Jirka
> > 
> > Sorry for late!
> > 
> > On Tue, Nov 06, 2018 at 12:54:36PM +0100, Jiri Olsa wrote:
> > > On Mon, Nov 05, 2018 at 08:53:42PM -0800, David Miller wrote:
> > > > 
> > > > Jiri,
> > > > 
> > > > Because you now run queued_events__queue() lockless with that condvar
> > > > trick, it is possible for top->qe.in to be seen as one past the data[]
> > > > array, this is because the rotate_queues() code goes:
> > > > 
> > > > 	if (++top->qe.in > &top->qe.data[1])
> > > > 		top->qe.in = &top->qe.data[0];
> > > > 
> > > > So for a brief moment top->qe.in is out of range and thus
> > > > perf_top__mmap_read_idx() can try to enqueue to top->qe.data[2]
> > > > 
> > > > We can just do:
> > > > 
> > > > 	if (top->qe.in == &top->qe.data[1])
> > > > 		top->qe.in = &top->qe.data[0];
> > > > 	else
> > > > 		top->qe.in = &top->qe.data[1];
> > > > 
> > > > Or, make top->qe.in an index, and simply go:
> > > > 
> > > > 	top->qe.in ^= 1;
> > > > 
> > > > Either way will fix the bug.
> > > 
> > > ah right.. I had originaly full mutex around that,
> > > then I switched it off in the last patch and did
> > > not realize this implication.. nice ;-)
> > 
> > I like the rotate_queues() using cond-variable.  Have you tried to use
> > the same for hists->lock in hists__get_rotate_entries_in() too?
> > 
> > Eventually it'd be nice to avoid locks when a single thread processes
> > all the events.
> 
> yep, I thought we could use it there as well, but it could
> be more tricky because we use hists->lock for that, which
> is used on other places as well.. will check

I found hists->lock used only 3 places - hists__get_rotate_entries_in()
and perf_event__process_sample() to protect rotating.  The other place
is perf_top__record_precise_ip() which is called during the sample
processing before going to sleep.

Hmm.. I think processing thread should not go to sleep anyway.  It's
the responsibility of display thread show warning (and waiting for
user input or timeout).  I'll try to do something..

Thanks,
Namhyung