From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH bpf-next 2/2] bpf, libbpf: simplify perf RB walk and do incremental updates Date: Thu, 11 Oct 2018 20:04:05 -0700 Message-ID: <20181011200405.0af4a48b@cakuba.netronome.com> References: <20181011140207.27602-1-daniel@iogearbox.net> <20181011140207.27602-3-daniel@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: alexei.starovoitov@gmail.com, netdev@vger.kernel.org To: Daniel Borkmann Return-path: Received: from mail-qt1-f180.google.com ([209.85.160.180]:41270 "EHLO mail-qt1-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726034AbeJLKeW (ORCPT ); Fri, 12 Oct 2018 06:34:22 -0400 Received: by mail-qt1-f180.google.com with SMTP id l41-v6so12417781qtl.8 for ; Thu, 11 Oct 2018 20:04:09 -0700 (PDT) In-Reply-To: <20181011140207.27602-3-daniel@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: 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