From: Song Liu <songliubraving@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>,
"daniel@iogearbox.net" <daniel@iogearbox.net>,
Yonghong Song <yhs@fb.com>, Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 02/12] libbpf: implement BPF CO-RE offset relocation algorithm
Date: Wed, 31 Jul 2019 17:46:59 +0000 [thread overview]
Message-ID: <989ABCA7-D84C-43A8-853B-4C9E25FF133E@fb.com> (raw)
In-Reply-To: <CAEf4Bzb6swYtf7J_m1bZo6o+aT1AcCXZX5ZBw7Uja=Tne2LCuw@mail.gmail.com>
> On Jul 31, 2019, at 10:18 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 31, 2019 at 1:30 AM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>> On Jul 30, 2019, at 11:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Tue, Jul 30, 2019 at 10:19 PM Song Liu <songliubraving@fb.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Jul 30, 2019, at 6:00 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>>>
>>>>> On Tue, Jul 30, 2019 at 5:39 PM Song Liu <songliubraving@fb.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Jul 30, 2019, at 12:53 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>>>>
>>>>>>> This patch implements the core logic for BPF CO-RE offsets relocations.
>>>>>>> Every instruction that needs to be relocated has corresponding
>>>>>>> bpf_offset_reloc as part of BTF.ext. Relocations are performed by trying
>>>>>>> to match recorded "local" relocation spec against potentially many
>>>>>>> compatible "target" types, creating corresponding spec. Details of the
>>>>>>> algorithm are noted in corresponding comments in the code.
>>>>>>>
>>>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>
>> [...]
>>
>>>>>>
>>>>>
>>>>> I just picked the most succinct and non-repetitive form. It's
>>>>> immediately apparent which type it's implicitly converted to, so I
>>>>> felt there is no need to repeat it. Also, just (void *) is much
>>>>> shorter. :)
>>>>
>>>> _All_ other code in btf.c converts the pointer to the target type.
>>>
>>> Most in libbpf.c doesn't, though. Also, I try to preserve pointer
>>> constness for uses that don't modify BTF types (pretty much all of
>>> them in libbpf), so it becomes really verbose, despite extremely short
>>> variable names:
>>>
>>> const struct btf_member *m = (const struct btf_member *)(t + 1);
>>
>> I don't think being verbose is a big problem here. Overusing
>
> Problem is too big and strong word to describe this :). It hurts
> readability and will often quite artificially force either wrapping
> the line or unnecessarily splitting declaration and assignment. Void *
> on the other hand is short and usually is in the same line as target
> type declaration, if not, you'll have to find local variable
> declaration to double-check type, if you are unsure.
>
> Using (void *) + implicit cast to target pointer type is not
> unprecedented in libbpf:
>
> $ rg ' = \((const )?struct \w+ \*\)' tools/lib/bpf/ | wc -l
> 52
> $ rg ' = \((const )?void \*\)' tools/lib/bpf/ | wc -l
> 35
>
> 52 vs 35 is majority overall, but not by a landslide.
>
>> (void *) feels like a bigger problem.
>
> Why do you feel it's a problem? void * conveys that we have a piece of
> memory that we will need to reinterpret as some concrete pointer type.
> That's what we are doing, skipping btf_type and then interpreting
> memory after common btf_type prefix is some other type, depending on
> actual BTF kind. I don't think void * is misleading in any way.
(void *) hides some problem. For example:
struct type_a *ptr_a = NULL;
struct type_b *ptr_b = NULL;
/* we want this */
ptr_a = (struct type_a *)data;
ptr_b = (struct type_b *)(data + offset);
/* typo, should be ptr_b, compiler will complain */
ptr_a = (struct type_b *)(data + offset);
/* typo, should be ptr_b, compiler will ignore */
ptr_a = (void *)(data + offset);
Such typo is not very rare. And it may be really painful to debug.
That being said, I think we have spent too much time on this. I will
let you make the final call. Either way:
Acked-by: Song Liu <songliubraving@fb.com>
next prev parent reply other threads:[~2019-07-31 19:23 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-30 19:53 [PATCH v2 bpf-next 00/12] CO-RE offset relocations Andrii Nakryiko
2019-07-30 19:53 ` [PATCH v2 bpf-next 01/12] libbpf: add .BTF.ext offset relocation section loading Andrii Nakryiko
2019-07-30 21:19 ` Song Liu
2019-07-30 19:53 ` [PATCH v2 bpf-next 02/12] libbpf: implement BPF CO-RE offset relocation algorithm Andrii Nakryiko
2019-07-30 23:43 ` Song Liu
2019-07-30 23:55 ` Andrii Nakryiko
2019-07-31 0:16 ` Song Liu
2019-07-31 0:39 ` Song Liu
2019-07-31 1:00 ` Andrii Nakryiko
2019-07-31 5:19 ` Song Liu
2019-07-31 6:52 ` Andrii Nakryiko
2019-07-31 8:29 ` Song Liu
2019-07-31 17:18 ` Andrii Nakryiko
2019-07-31 17:46 ` Song Liu [this message]
2019-07-30 19:53 ` [PATCH v2 bpf-next 03/12] selftests/bpf: add BPF_CORE_READ relocatable read macro Andrii Nakryiko
2019-07-30 21:24 ` Song Liu
2019-07-30 21:26 ` Andrii Nakryiko
2019-07-30 21:33 ` Song Liu
2019-07-30 19:54 ` [PATCH v2 bpf-next 04/12] selftests/bpf: add CO-RE relocs testing setup Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 05/12] selftests/bpf: add CO-RE relocs struct flavors tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 06/12] selftests/bpf: add CO-RE relocs nesting tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 07/12] selftests/bpf: add CO-RE relocs array tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 08/12] selftests/bpf: add CO-RE relocs enum/ptr/func_proto tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 09/12] selftests/bpf: add CO-RE relocs modifiers/typedef tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 10/12] selftests/bpf: add CO-RE relocs ptr-as-array tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 11/12] selftests/bpf: add CO-RE relocs ints tests Andrii Nakryiko
2019-07-30 19:54 ` [PATCH v2 bpf-next 12/12] selftests/bpf: add CO-RE relocs misc tests 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=989ABCA7-D84C-43A8-853B-4C9E25FF133E@fb.com \
--to=songliubraving@fb.com \
--cc=Kernel-team@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=netdev@vger.kernel.org \
--cc=yhs@fb.com \
/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).