On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > 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!
> > >
> > > No problem. :)
> > >
> > > >
> > > > 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.
> > >
> > > Acknowledged.
> > >
> > > > Have you considered using upper bits to represent flags?
> > >
> > > Yes, I thought about that. Some of my thoughts are:
> > >
> > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> > > - What if we run out of flag bits in future?
> > > - We could fold btf_id in the structure in this patchset. And new
> > > fields could be easily added if needed.
> > >
> > > So with these questions, I didn't pursue that approach in the first
> > > place. But I admit that it does look better by writing
> > >
> > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL,
> > >
> > > Instead of
> > >
> > > + .arg3 = {
> > > + .type = ARG_PTR_TO_MAP_VALUE,
> > > + .flag = ARG_FLAG_MAYBE_NULL,
> > > + },
> > >
> > > Let's see if there is any further comment. I can go take a look and
> > > prepare for that approach in the next revision.
> > >
> >
> > +1 on staying within a single enum and using upper bits
> >
> > >
> > >
> > > >
> > > > 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.
> >
> > I support using the same bit value, but should we use the exact same
> > enum name for three different enums? Like MAYBE_NULL, which enum is it
> > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
> > REG_MAYBE_NULL be more explicit about what they apply to?
>
> argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition
> to REG_MAYBE_NULL".
Why differentiate? What difference do you see?
The flag looks the same to me in return, reg and arg.
> >
> > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
> > the same thing, are they? Is the plan to use two different bits for
> > them or pretend that CONST_OR_ZERO "may be null"?
What's the difference ?
I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly
to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM.
