linux-kernel - Re: [PATCH RFC] hist lookups

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 20 Nov 2018 10:13:21 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     David Miller <davem@...emloft.net>, acme@...nel.org,
        linux-kernel@...r.kernel.org, jolsa@...nel.org, kernel-team@....com
Subject: Re: [PATCH RFC] hist lookups

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