From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19BD5C433EF for ; Fri, 17 Sep 2021 17:28:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F0E1261212 for ; Fri, 17 Sep 2021 17:28:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344079AbhIQR30 (ORCPT ); Fri, 17 Sep 2021 13:29:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50032 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344072AbhIQR3Z (ORCPT ); Fri, 17 Sep 2021 13:29:25 -0400 Received: from mail-qv1-xf49.google.com (mail-qv1-xf49.google.com [IPv6:2607:f8b0:4864:20::f49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 255B5C061574 for ; Fri, 17 Sep 2021 10:28:02 -0700 (PDT) Received: by mail-qv1-xf49.google.com with SMTP id e8-20020a0cf348000000b0037a350958f2so94297498qvm.7 for ; Fri, 17 Sep 2021 10:28:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=WPK2aJQkQXvATQHG8vjoQcTSTfV20fL4631O3tzVk3k=; b=TDaTG4rZ2X2Xy+BvzC13AkUdRBznYC0vpBwsJ6eecgNAjmsiu2Z3epderbH1oHnWYx l2R6UhgytVXelUc2lpvyuTRkGeV2bVu+VHvT7ClWpOAZ85tCT9AuLks2a64/vAP7OXmr Glvt/ao0SuH6GmfHOTmJeNvxLaZTTVyjiCC7SHGltTTE3Fo52Eoyr2sO7m/1I2VG19p6 SRiQq1gTuFX/exAEim+5npK2lpEVLthdU+UAOIBrRTM9VdjZ+h2cs+VLXlecdaUWSzuI WR8+Uo8sl4zSbq5x5Y5HDn9F4Yh63oSxBBeT2Si4UMqUtgydQApLbh2/g8Bl7peYQtz6 0aUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=WPK2aJQkQXvATQHG8vjoQcTSTfV20fL4631O3tzVk3k=; b=eSt9CQmu9M9nnGI+450NSxHFoSjWRfIvofqOHT1WgresM6hiPS9ddAcQfGvfqL6R88 btuQ7VEt9y9zvk7faElGEUp0xBNZOSZCEqFyOuouBBh8NpdyRvLKPZkBfZH7tUeNzKyD 6C+pou9ggLFy4jZmIwPAQ94y7F1QW4YNSmG7Ru8rOXGsTtZuWSUU0FeER2ODoELlKVSr b5NyySvoeOxopgioyV/QEkF8M6/K65caz0wDR7PKm9zmoJXGYFG+mX88e4kuB5W9PKQK tMfFrRxi+wLIRQdhGqEdpmcNn8ytCYDVefE/KF98cXMKCj0Ziwsxbv8OvdPzaiyUJ16L TVkA== X-Gm-Message-State: AOAM531BBr8sE+7RHrNEd+nQynWpjNSKu7BORgK6QEMxJnCxwtDwtD+x 5BtEy1OkaBxAaTOdhDfebwpWnRE= X-Google-Smtp-Source: ABdhPJyW1ksD3gbmh3+/ZtNpzHHv/tAxxjtpZF2bgcJNywi6hJ2OKEMBXCJqHvEW1ZbzjWUmFfD6Xbk= X-Received: from sdf2.svl.corp.google.com ([2620:15c:2c4:201:5937:6f21:df47:504]) (user=sdf job=sendgmr) by 2002:a05:6214:3e1:: with SMTP id cf1mr12342194qvb.43.1631899680920; Fri, 17 Sep 2021 10:28:00 -0700 (PDT) Date: Fri, 17 Sep 2021 10:27:58 -0700 In-Reply-To: <20210917061020.821711-6-andrii@kernel.org> Message-Id: Mime-Version: 1.0 References: <20210917061020.821711-1-andrii@kernel.org> <20210917061020.821711-6-andrii@kernel.org> Subject: Re: [PATCH bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals From: sdf@google.com To: Andrii Nakryiko Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, kernel-team@fb.com Content-Type: text/plain; charset="UTF-8"; format=flowed; delsp=yes Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 09/16, Andrii Nakryiko wrote: > Move closer to not relying on bpf_sec_def internals that won't be part > of public API, when pluggable SEC() handlers will be allowed. Drop > pre-calculated prefix length, and in various helpers don't rely on this > prefix length availability. Also minimize reliance on knowing > bpf_sec_def's prefix for few places where section prefix shortcuts are > supported (e.g., tp vs tracepoint, raw_tp vs raw_tracepoint). > Given checking some string for having a given string-constant prefix is > such a common operation and so annoying to be done with pure C code, add > a small macro helper, str_has_pfx(), and reuse it throughout libbpf.c > where prefix comparison is performed. With __builtin_constant_p() it's > possible to have a convenient helper that checks some string for having > a given prefix, where prefix is either string literal (or compile-time > known string due to compiler optimization) or just a runtime string > pointer, which is quite convenient and saves a lot of typing and string > literal duplication. > Signed-off-by: Andrii Nakryiko > --- > tools/lib/bpf/libbpf.c | 41 ++++++++++++++++++--------------- > tools/lib/bpf/libbpf_internal.h | 7 ++++++ > 2 files changed, 30 insertions(+), 18 deletions(-) > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8ab2edbf7a3b..52c398fae0af 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -226,7 +226,6 @@ typedef struct bpf_link *(*attach_fn_t)(const struct > bpf_program *prog, long coo > struct bpf_sec_def { > const char *sec; > - size_t len; > enum bpf_prog_type prog_type; > enum bpf_attach_type expected_attach_type; > bool is_exp_attach_type_optional; > @@ -1671,7 +1670,7 @@ static int bpf_object__process_kconfig_line(struct > bpf_object *obj, > void *ext_val; > __u64 num; > - if (strncmp(buf, "CONFIG_", 7)) > + if (!str_has_pfx(buf, "CONFIG_")) > return 0; > sep = strchr(buf, '='); > @@ -2919,7 +2918,7 @@ static Elf_Data *elf_sec_data(const struct > bpf_object *obj, Elf_Scn *scn) > static bool is_sec_name_dwarf(const char *name) > { > /* approximation, but the actual list is too long */ > - return strncmp(name, ".debug_", sizeof(".debug_") - 1) == 0; > + return str_has_pfx(name, ".debug_"); > } > static bool ignore_elf_section(GElf_Shdr *hdr, const char *name) > @@ -2941,7 +2940,7 @@ static bool ignore_elf_section(GElf_Shdr *hdr, > const char *name) > if (is_sec_name_dwarf(name)) > return true; > - if (strncmp(name, ".rel", sizeof(".rel") - 1) == 0) { > + if (str_has_pfx(name, ".rel")) { > name += sizeof(".rel") - 1; > /* DWARF section relocations */ > if (is_sec_name_dwarf(name)) > @@ -6890,8 +6889,7 @@ static int bpf_object__resolve_externs(struct > bpf_object *obj, > if (err) > return err; > pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver); > - } else if (ext->type == EXT_KCFG && > - strncmp(ext->name, "CONFIG_", 7) == 0) { > + } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_")) > { > need_config = true; > } else if (ext->type == EXT_KSYM) { > if (ext->ksym.type_id) > @@ -7955,7 +7953,6 @@ void bpf_program__set_expected_attach_type(struct > bpf_program *prog, > attachable, attach_btf) \ > { \ > .sec = string, \ > - .len = sizeof(string) - 1, \ > .prog_type = ptype, \ > .expected_attach_type = eatype, \ > .is_exp_attach_type_optional = eatype_optional, \ > @@ -7986,7 +7983,6 @@ void bpf_program__set_expected_attach_type(struct > bpf_program *prog, > #define SEC_DEF(sec_pfx, ptype, ...) { \ > .sec = sec_pfx, \ > - .len = sizeof(sec_pfx) - 1, \ > .prog_type = BPF_PROG_TYPE_##ptype, \ > .preload_fn = libbpf_preload_prog, \ > __VA_ARGS__ \ > @@ -8160,10 +8156,8 @@ static const struct bpf_sec_def > *find_sec_def(const char *sec_name) > int i, n = ARRAY_SIZE(section_defs); > for (i = 0; i < n; i++) { > - if (strncmp(sec_name, > - section_defs[i].sec, section_defs[i].len)) > - continue; > - return §ion_defs[i]; > + if (str_has_pfx(sec_name, section_defs[i].sec)) > + return §ion_defs[i]; > } > return NULL; > } > @@ -8517,7 +8511,7 @@ static int libbpf_find_attach_btf_id(struct > bpf_program *prog, int *btf_obj_fd, > prog->sec_name); > return -ESRCH; > } > - attach_name = prog->sec_name + prog->sec_def->len; > + attach_name = prog->sec_name + strlen(prog->sec_def->sec); > /* BPF program's BTF ID */ > if (attach_prog_fd) { > @@ -9454,8 +9448,11 @@ static struct bpf_link *attach_kprobe(const struct > bpf_program *prog, long cooki > char *func; > int n, err; > - func_name = prog->sec_name + prog->sec_def->len; > - opts.retprobe = strcmp(prog->sec_def->sec, "kretprobe/") == 0; > + opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/"); > + if (opts.retprobe) > + func_name = prog->sec_name + sizeof("kretprobe/") - 1; > + else > + func_name = prog->sec_name + sizeof("kprobe/") - 1; > n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset); > if (n < 1) { > @@ -9627,8 +9624,11 @@ static struct bpf_link *attach_tp(const struct > bpf_program *prog, long cookie) > if (!sec_name) > return libbpf_err_ptr(-ENOMEM); > - /* extract "tp//" */ > - tp_cat = sec_name + prog->sec_def->len; > + /* extract "tp//" or "tracepoint//" */ > + if (str_has_pfx(prog->sec_name, "tp/")) > + tp_cat = sec_name + sizeof("tp/") - 1; > + else > + tp_cat = sec_name + sizeof("tracepoint/") - 1; > tp_name = strchr(tp_cat, '/'); > if (!tp_name) { > free(sec_name); > @@ -9674,7 +9674,12 @@ struct bpf_link > *bpf_program__attach_raw_tracepoint(const struct bpf_program *pr > static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, > long cookie) > { > - const char *tp_name = prog->sec_name + prog->sec_def->len; > + const char *tp_name; > + > + if (str_has_pfx(prog->sec_name, "raw_tp/")) > + tp_name = prog->sec_name + sizeof("raw_tp/") - 1; > + else > + tp_name = prog->sec_name + sizeof("raw_tracepoint/") - 1; > return bpf_program__attach_raw_tracepoint(prog, tp_name); > } > diff --git a/tools/lib/bpf/libbpf_internal.h > b/tools/lib/bpf/libbpf_internal.h > index ceb0c98979bc..ec79400517d4 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -89,6 +89,13 @@ > (offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD)) > #endif > +/* Check whether a string `str` has prefix `pfx`, regardless if `pfx` is > + * a string literal known at compilation time or char * pointer known > only at > + * runtime. > + */ > +#define str_has_pfx(str, pfx) \ > + (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : > strlen(pfx)) == 0) > + Nit: maybe 'starts_with'? c++ has that in stdlib and iirc python also has either starts_with or statswith.