On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> This is a pure cleanup patchset that tries to use flag to mark whether
> an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> so allows us to embed the properties of arguments in the struct, which
> is a more scalable solution than introducing a new enum. This patchset
> performs this transformation only on arg_type. If it looks good,
> follow-up patches will do the same on reg_type and ret_type.
>
> The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
Nice. Thank you for working on it!
The enum->struct conversion works for bpf_arg_type, but applying
the same technique to bpf_reg_type could be problematic.
Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
Growing enum from 4 bytes to 8 byte struct will consume quite
a lot of extra memory.
> 19 files changed, 932 insertions(+), 780 deletions(-)
Just bpf_arg_type refactoring adds a lot of churn which could make
backports of future fixes not automatic anymore.
Similar converstion for bpf_reg_type and bpf_return_type will
be even more churn.
Have you considered using upper bits to represent flags?
Instead of diff:
- .arg1_type = ARG_CONST_MAP_PTR,
- .arg2_type = ARG_PTR_TO_FUNC,
- .arg3_type = ARG_PTR_TO_STACK_OR_NULL,
- .arg4_type = ARG_ANYTHING,
+ .arg1 = { .type = ARG_CONST_MAP_PTR },
+ .arg2 = { .type = ARG_PTR_TO_FUNC },
+ .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL },
+ .arg4 = { .type = ARG_ANYTHING },
can we make it look like:
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_FUNC,
- .arg3_type = ARG_PTR_TO_STACK_OR_NULL,
+ .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL,
.arg4_type = ARG_ANYTHING,
Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
would share the same flag bit: MAYBE_NULL.
Then static bool arg_type_may_be_null() will be comparing only single bit ?
While
if (arg_type == ARG_PTR_TO_MAP_VALUE ||
arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
will become:
arg_type &= FLAG_MASK;
if (arg_type == ARG_PTR_TO_MAP_VALUE ||
arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
Most of the time I would prefer explicit .type and .flag structure,
but saving memory is important for bpf_reg_type, so explicit bit
operations are probably justified.
