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=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED 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 CF635C282CB for ; Tue, 5 Feb 2019 22:35:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8F7DA20811 for ; Tue, 5 Feb 2019 22:35:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Yu9U6QjR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729778AbfBEWfJ (ORCPT ); Tue, 5 Feb 2019 17:35:09 -0500 Received: from mail-ot1-f65.google.com ([209.85.210.65]:45816 "EHLO mail-ot1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726900AbfBEWfI (ORCPT ); Tue, 5 Feb 2019 17:35:08 -0500 Received: by mail-ot1-f65.google.com with SMTP id 32so8635913ota.12 for ; Tue, 05 Feb 2019 14:35:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VX87Itx4aLiK9/l5YLFOK3TonBdJjGi7NcEnSmSMOHQ=; b=Yu9U6QjRk999owND0apm2k8vZC6G/ETL8a4+2U+jMPF6102D5I2FAvbNEwKqYorKoO 9h1e69e81Ns4QLKUQN6TlChDL1DPGXTz6/wP356LBPUlHLAfwDf8/Oxa542N7okU26FK nxNBjKSxC/9jw+xzKKI400BezVf1FnQuUW8XBS8gUTmtBGxXFDkR0QdvW5he5Qejllpw d/JMGsGL6Qh5fPiE4UuK8Dc/WtnTDJ+2ayaoYBRuGowFA9QQUfOYGBw+4DKbMSUtxg8W iMRdTAjcmsqqvvVy0gNqrwVcpMpaEywZUZj7LPGWx1aSB72591kzCLXv5vpRWXnIwIUm 1trA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VX87Itx4aLiK9/l5YLFOK3TonBdJjGi7NcEnSmSMOHQ=; b=VBOCAvF5GlWGexzJFGgi9ihp6pnd/w6GgRUwkmcyrF4gd3tCXD28NFIIO/nZ46k47Y +PQxC8o7E+rIkVZnDDn/vGwXL9tDJt/hvlJcAWJKmcPVuhfZmhYVtN1ZIO5cTL0UTQk8 wX3KoHrw48d4E8N1oc9013py5hYYICtREwUPEu6ZfZy94bV2IKOQsrBqT7r397OvNLGh AJMkssRPCZJt7IUx5ltkAo8X4E79qTTtK+cJDJn45yZ2oVTBe/OWlP9Uym4I9d+qr3Rl uH6p9MuM5DRZTsocuAogaxKDK93b3/Ap3wwWPbz89cbMQuKdl2ITBEI+PcLNHz9uT8Pw ChNw== X-Gm-Message-State: AHQUAubn8R93AMVog6ZQV1nLh8NsinnP/ux9ntS4x3S+if++vBhDBxx/ Pw2knQ+wdtjELVTkMe7b7D3b7BjIH4vbC9ZYnnDlgxt4 X-Google-Smtp-Source: AHgI3IaIRoPrWBMgata9paCM8StrvdEUvWzKgg7ocB8c2tGLNOfNYuVUW8zkCliq6s1FJGsrUOic/lvcHlbAN23POPU= X-Received: by 2002:a9d:7c1:: with SMTP id 59mr4256709oto.191.1549406107788; Tue, 05 Feb 2019 14:35:07 -0800 (PST) MIME-Version: 1.0 References: <20190205194856.967463-1-andriin@fb.com> <20190205194856.967463-2-andriin@fb.com> In-Reply-To: From: Andrii Nakryiko Date: Tue, 5 Feb 2019 14:34:56 -0800 Message-ID: Subject: Re: [PATCH bpf-next 1/2] btf: separate btf creation and loading To: Yonghong Song Cc: Andrii Nakryiko , Song Liu , Alexei Starovoitov , Martin Lau , "netdev@vger.kernel.org" , "daniel@iogearbox.net" Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, Feb 5, 2019 at 1:25 PM Yonghong Song wrote: > > > > On 2/5/19 11:48 AM, Andrii Nakryiko wrote: > > This change splits out previous btf__new functionality of constructing > > struct btf and loading it into kernel into two: > > - btf__new() just creates and initializes struct btf > > - btf__load() attempts to load existing struct btf into kernel > > > > btf__free will still close BTF fd, if it was ever loaded successfully > > into kernel. > > > > This change allows users of libbpf to manipulate BTF using its API, > > without the need to unnecessarily load it into kernel. > > > > One of the intended use cases is pahole using libbpf to do DWARF to BTF > > conversion and deduplication using libbpf, while handling ELF sections > > overwrites and other concerns on its own. > > > > Signed-off-by: Andrii Nakryiko > > Acked-by: Song Liu > > --- > > tools/lib/bpf/btf.c | 53 ++++++++++++++++++++++------------------ > > tools/lib/bpf/btf.h | 1 + > > tools/lib/bpf/libbpf.c | 2 +- > > tools/lib/bpf/libbpf.map | 1 + > > 4 files changed, 32 insertions(+), 25 deletions(-) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index 4949f8840bda..065d51fa63e5 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c > > @@ -366,8 +366,6 @@ void btf__free(struct btf *btf) > > > > struct btf *btf__new(__u8 *data, __u32 size) > > { > > - __u32 log_buf_size = 0; > > - char *log_buf = NULL; > > struct btf *btf; > > int err; > > > > @@ -377,15 +375,6 @@ struct btf *btf__new(__u8 *data, __u32 size) > > > > btf->fd = -1; > > > > - log_buf = malloc(BPF_LOG_BUF_SIZE); > > - if (!log_buf) { > > - err = -ENOMEM; > > - goto done; > > - } > > - > > - *log_buf = 0; > > - log_buf_size = BPF_LOG_BUF_SIZE; > > - > > btf->data = malloc(size); > > if (!btf->data) { > > err = -ENOMEM; > > @@ -395,17 +384,6 @@ struct btf *btf__new(__u8 *data, __u32 size) > > memcpy(btf->data, data, size); > > btf->data_size = size; > > > > - btf->fd = bpf_load_btf(btf->data, btf->data_size, > > - log_buf, log_buf_size, false); > > - > > - if (btf->fd == -1) { > > - err = -errno; > > - pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno); > > - if (log_buf && *log_buf) > > - pr_warning("%s\n", log_buf); > > - goto done; > > - } > > - > > err = btf_parse_hdr(btf); > > if (err) > > goto done; > > @@ -417,8 +395,6 @@ struct btf *btf__new(__u8 *data, __u32 size) > > err = btf_parse_type_sec(btf); > > > > done: > > - free(log_buf); > > - > > if (err) { > > btf__free(btf); > > return ERR_PTR(err); > > @@ -427,6 +403,35 @@ struct btf *btf__new(__u8 *data, __u32 size) > > return btf; > > } > > > > +int btf__load(struct btf* btf) { > > + __u32 log_buf_size = BPF_LOG_BUF_SIZE; > > + char *log_buf = NULL; > > + > > + if (btf->fd >= 0) { > > + return -EEXIST; > > + } > > + > > + log_buf = malloc(log_buf_size); > > + if (!log_buf) > > + return -ENOMEM; > > + > > + *log_buf = 0; > > + > > + btf->fd = bpf_load_btf(btf->data, btf->data_size, > > + log_buf, log_buf_size, false); > > + if (btf->fd < 0) { > > + btf->fd = -errno; > > Why you set btf->fd = -errno? Do you have any intended use for it later. > If not, I would still prefer the existing value -1. This will be > consistent with all other fd field convention in libbpf. I though it would be convenient to get created fd back from btf_load(), but I don't have strong preferences. In any case, we have btf__get_fd() to get it back, so I will change btf__load() to return just error code. > > > + pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno); > > + if (*log_buf) > > + pr_warning("%s\n", log_buf); > > + goto done; > > + } > > + > > +done: > > + free(log_buf); > > + return btf->fd; > > +} > > + > > int btf__fd(const struct btf *btf) > > { > > return btf->fd; > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > > index 25a9d2db035d..e8410887f93a 100644 > > --- a/tools/lib/bpf/btf.h > > +++ b/tools/lib/bpf/btf.h > > @@ -57,6 +57,7 @@ struct btf_ext_header { > > > > LIBBPF_API void btf__free(struct btf *btf); > > LIBBPF_API struct btf *btf__new(__u8 *data, __u32 size); > > +LIBBPF_API int btf__load(struct btf* btf); > > LIBBPF_API __s32 btf__find_by_name(const struct btf *btf, > > const char *type_name); > > LIBBPF_API __u32 btf__get_nr_types(const struct btf *btf); > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 47969aa0faf8..75b82c1cfc72 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -835,7 +835,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > obj->efile.maps_shndx = idx; > > else if (strcmp(name, BTF_ELF_SEC) == 0) { > > obj->btf = btf__new(data->d_buf, data->d_size); > > - if (IS_ERR(obj->btf)) { > > + if (IS_ERR(obj->btf) || btf__load(obj->btf) < 0) { > > pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > BTF_ELF_SEC, PTR_ERR(obj->btf)); > > obj->btf = NULL; > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > index 89c1149e32ee..ffa1fe044f6a 100644 > > --- a/tools/lib/bpf/libbpf.map > > +++ b/tools/lib/bpf/libbpf.map > > @@ -134,6 +134,7 @@ LIBBPF_0.0.2 { > > bpf_object__find_map_fd_by_name; > > bpf_get_link_xdp_id; > > btf__dedup; > > + btf__load; > > Maybe put btf__load after btf__get_strings based on alphabetical order. > With the above changes, Yep, will fix to be alphabetically sorted. > > > > btf__get_map_kv_tids; > > btf__get_nr_types; > > btf__get_strings; > >