On 10/18/21 5:22 PM, Andrii Nakryiko wrote:
> On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky <davemarchevsky@xxxxxx> 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@xxxxxx>
>> ---
>> 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.
