netdev - Re: [PATCH bpf-next 2/2] bpf, libbpf: simplify perf RB walk and do incremental updates

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Oct 2018 20:04:05 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     alexei.starovoitov@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 2/2] bpf, libbpf: simplify perf RB walk and do
 incremental updates

On Thu, 11 Oct 2018 16:02:07 +0200, Daniel Borkmann wrote:
> Clean up and improve bpf_perf_event_read_simple() ring walk a bit
> to use similar tail update scheme as in perf and bcc allowing the
> kernel to make forward progress not only after full timely walk.

The extra memory barriers won't impact performance?  If I read the code
correctly we now have:

	while (bla) {
		head = HEAD
		rmb()

		...

		mb()
		TAIL = tail
	}

Would it make sense to try to piggy back on the mb() for head re-read
at least?  Perhaps that's a non-issue, just wondering.

> Also few other improvements to use realloc() instead of free() and
> malloc() combination and for the callback use proper perf_event_header
> instead of void pointer, so that real applications can use container_of()
> macro with proper type checking.

FWIW the free() + malloc() was to avoid the the needless copy of the
previous event realloc() may do.  It makes sense to use realloc()
especially if you want to put extra info in front of the buffer, just
sayin' it wasn't a complete braino ;)

> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>