Re: [PATCH v2 bpf-next 1/2] bpf: add verified_insns to bpf_prog_info and fdinfo

From: Dave Marchevsky <davemarchevsky@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>
Subject: Re: [PATCH v2 bpf-next 1/2] bpf: add verified_insns to bpf_prog_info and fdinfo
Date: Wed, 20 Oct 2021 04:03:55 -0400	[thread overview]
Message-ID: <549f7100-ce3f-4754-a048-a2c824139a02@fb.com> (raw)
In-Reply-To: <CAEf4BzaMgv5otr9AQGHZW=sUCuBdt_Vkv_GqB9n8BYVcZWHjAQ@mail.gmail.com>

On 10/18/21 5:22 PM, Andrii Nakryiko wrote:   
> On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>>
>> This stat is currently printed in the verifier log and not stored
>> anywhere. To ease consumption of this data, add a field to bpf_prog_aux
>> so it can be exposed via BPF_OBJ_GET_INFO_BY_FD and fdinfo.
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>> ---
>>  include/linux/bpf.h            | 1 +
>>  include/uapi/linux/bpf.h       | 2 +-
>>  kernel/bpf/syscall.c           | 8 ++++++--
>>  kernel/bpf/verifier.c          | 1 +
>>  tools/include/uapi/linux/bpf.h | 2 +-
>>  5 files changed, 10 insertions(+), 4 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index 6fc59d61937a..d053fc7e7995 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -5591,7 +5591,7 @@ struct bpf_prog_info {
>>         char name[BPF_OBJ_NAME_LEN];
>>         __u32 ifindex;
>>         __u32 gpl_compatible:1;
>> -       __u32 :31; /* alignment pad */
>> +       __u32 verified_insns:31;
> 
> These 31 unused bits seem like a good place to add extra generic
> flags, not new counters. E.g., like a sleepable flag. So I wonder if
> it's better to use a dedicated u32 field for counters like
> verified_insns and keep these reserved fields for boolean flags?
> 
> Daniel, I know you proposed to reuse those 31 bits. How strong do you
> feel about this? For any other kind of counter we seem to be using a
> complete dedicated integer field, so it would be consistent to keep
> doing that?
> 
> Having a sleepable bit still seems like a good idea, btw :) but it's a
> separate change from Dave's.

Re: use padding vs new field, I don't have a strong feeling either way,
but if there are proper flags that could use that space in the near 
future, this combined with consistency with other counters leans me 
towards adding a new field.

Also, when using the bitfield space, clang complains about types in 
selftest assert:

tools/testing/selftests/bpf/prog_tests/verif_stats.c:23:17: error: ‘typeof’ applied to a bit-field
   23 |  if (!ASSERT_GT(info.verified_insns, 0, "verified_insns"))
      |                 ^~~~
./test_progs.h:227:9: note: in definition of macro ‘ASSERT_GT’
  227 |  typeof(actual) ___act = (actual);    \

Which necessitated a __u32 cast in this version of the patchset. Don't think
it would cause issues outside of this specific selftest, but worth noting.

Anyways, sent a v3 of the patchset with 'new field' and other comments
addressed.