bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
	daniel@iogearbox.net, kernel-team@fb.com
Subject: Re: [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs
Date: Wed, 10 Mar 2021 18:34:17 -0800	[thread overview]
Message-ID: <20210311023417.vhwe4avhvri7gcr5@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210310040431.916483-6-andrii@kernel.org>

On Tue, Mar 09, 2021 at 08:04:26PM -0800, Andrii Nakryiko wrote:
> +
> +	struct btf *strtab_btf; /* we use struct btf to manage strings */
...
> +	str_off = btf__add_str(linker->strtab_btf, sec->sec_name);
> +	sec->shdr->sh_name = str_off;

That bit took me an hour to grok.
That single line comment above is far far from obvious.
What the logic is relying on is that string section in BTF format
has the same zero terminated set of strings as ELF's .strtab section.
There is no BTF anywhere here in this 'strtab_btf'.
The naming choice made it double hard.
My understanding that you're using that instead of renaming btf_add_mem()
into something generic to rely on string hashmap for string dedup?

The commit log in patch 2 that introduces btf_raw_strs() sort of talks about
this code puzzle, but I would never guessed that's what you meant based
on patch 2 alone.

Did you consider some renaming/generalizing of string management to
avoid btf__add_str() through out the patch 5?
The "btf_" prefix makes things challenging to read.
Especially when patch 6 is using btf__add_str() to add to real BTF.

Mainly pointing it out for others who might be looking at the patches.

  reply	other threads:[~2021-03-11  2:35 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-10  4:04 [PATCH bpf-next 00/10] BPF static linking Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 01/10] libbpf: expose btf_type_by_id() internally Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 02/10] libbpf: add internal helper to get raw BTF strings section Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 03/10] libbpf: generalize BTF and BTF.ext type ID and strings iteration Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 04/10] libbpf: add generic BTF type shallow copy API Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs Andrii Nakryiko
2021-03-11  2:34   ` Alexei Starovoitov [this message]
2021-03-11  3:29     ` Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 06/10] libbpf: add BPF static linker BTF and BTF.ext support Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking Andrii Nakryiko
2021-03-11 11:31   ` Quentin Monnet
2021-03-11 18:45     ` Andrii Nakryiko
2021-03-12 18:07       ` Quentin Monnet
2021-03-13 18:37         ` Andrii Nakryiko
2021-03-13 21:27           ` Quentin Monnet
2021-03-10  4:04 ` [PATCH bpf-next 08/10] selftests/bpf: re-generate vmlinux.h and BPF skeletons if bpftool changed Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 09/10] selftests/bpf: pass all BPF .o's through BPF static linker Andrii Nakryiko
2021-03-10  4:04 ` [PATCH bpf-next 10/10] selftests/bpf: add multi-file statically linked BPF object file test Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210311023417.vhwe4avhvri7gcr5@ast-mbp.dhcp.thefacebook.com \
    --to=alexei.starovoitov@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).