Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751694AbdLAWqM (ORCPT ); Fri, 1 Dec 2017 17:46:12 -0500 Received: from mail-qk0-f194.google.com ([209.85.220.194]:45239 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751256AbdLAWqL (ORCPT ); Fri, 1 Dec 2017 17:46:11 -0500 X-Google-Smtp-Source: AGs4zMZjs9EV9IU5y6zavC5BKlAcOEOufh2UG/kObwFFsKn3p0Cjsrm4B3klu/1h3MvYNukNs3zUmw== Date: Fri, 1 Dec 2017 14:46:06 -0800 From: Jakub Kicinski To: Quentin Monnet Cc: Roman Gushchin , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com, ast@kernel.org, daniel@iogearbox.net, kafai@fb.com Subject: Re: [PATCH net-next 1/5] libbpf: add ability to guess program type based on section name Message-ID: <20171201144606.5ddd0272@cakuba.netronome.com> In-Reply-To: <614dc433-9445-52df-a202-f7cb1cb79ca1@netronome.com> References: <20171130134302.2840-1-guro@fb.com> <20171130134302.2840-2-guro@fb.com> <614dc433-9445-52df-a202-f7cb1cb79ca1@netronome.com> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id vB1MkGor020650 Content-Length: 3378 Lines: 88 On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote: > Thanks Roman! > One comment in-line. > > 2017-11-30 13:42 UTC+0000 ~ Roman Gushchin > > The bpf_prog_load() function will guess program type if it's not > > specified explicitly. This functionality will be used to implement > > loading of different programs without asking a user to specify > > the program type. In first order it will be used by bpftool. > > > > Signed-off-by: Roman Gushchin > > Cc: Alexei Starovoitov > > Cc: Daniel Borkmann > > Cc: Jakub Kicinski > > --- > > tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 5aa45f89da93..9f2410beaa18 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -1721,6 +1721,41 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT); > > BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP); > > BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT); > > > > +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog) > > +{ > > + if (!prog->section_name) > > + goto err; > > + > > + if (strncmp(prog->section_name, "socket", 6) == 0) > > + return BPF_PROG_TYPE_SOCKET_FILTER; > > + if (strncmp(prog->section_name, "kprobe/", 7) == 0) > > + return BPF_PROG_TYPE_KPROBE; > > + if (strncmp(prog->section_name, "kretprobe/", 10) == 0) > > + return BPF_PROG_TYPE_KPROBE; > > + if (strncmp(prog->section_name, "tracepoint/", 11) == 0) > > + return BPF_PROG_TYPE_TRACEPOINT; > > + if (strncmp(prog->section_name, "xdp", 3) == 0) > > + return BPF_PROG_TYPE_XDP; > > + if (strncmp(prog->section_name, "perf_event", 10) == 0) > > + return BPF_PROG_TYPE_PERF_EVENT; > > + if (strncmp(prog->section_name, "cgroup/skb", 10) == 0) > > + return BPF_PROG_TYPE_CGROUP_SKB; > > + if (strncmp(prog->section_name, "cgroup/sock", 11) == 0) > > + return BPF_PROG_TYPE_CGROUP_SOCK; > > + if (strncmp(prog->section_name, "cgroup/dev", 10) == 0) > > + return BPF_PROG_TYPE_CGROUP_DEVICE; > > + if (strncmp(prog->section_name, "sockops", 7) == 0) > > + return BPF_PROG_TYPE_SOCK_OPS; > > + if (strncmp(prog->section_name, "sk_skb", 6) == 0) > > + return BPF_PROG_TYPE_SK_SKB; > > I do not really like these hard-coded lengths, maybe we could work out > something nicer with a bit of pre-processing work? Perhaps something like: > > #define SOCKET_FILTER_SEC_PREFIX "socket" > #define KPROBE_SEC_PREFIX "kprobe/" > […] > > #define TRY_TYPE(string, __TYPE) \ > do { \ > if (!strncmp(string, __TYPE ## _SEC_PREFIX, \ > sizeof(__TYPE ## _SEC_PREFIX))) \ > return BPF_PROG_TYPE_ ## __TYPE; \ > } while(0); I like the suggestion, but I think return and goto statements hiding inside macros are slightly frowned upon in the netdev. Perhaps just a macro that wraps the strncmp() with sizeof would be enough? Without the return inside? > static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog) > { > if (!prog->section_name) > goto err; > > TRY_TYPE(prog->section_name, SOCKET_FILTER); > TRY_TYPE(prog->section_name, KPROBE); > […] > > err: > pr_warning("…", > prog->section_name); > > return BPF_PROG_TYPE_UNSPEC; > }