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

On 10/18/21 5:22 PM, Andrii Nakryiko wrote:   

On Mon, Oct 11, 2021 at 1:54 PM Dave Marchevsky [off-list ref] wrote:

quoted

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 <redacted>
---
 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(-)

[...]

quoted

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.