Re: [PATCH RFC bpf-next v2 1/4] bpf: Introduce BPF_HELPER_CALL

[Stanislav Fomichev <sdf@google.com>]

On 02/16, Alexei Starovoitov wrote:
> On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On 02/16, Alexei Starovoitov wrote:
> > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > > wrote:
> > > >
> > > > Make the code more readable by introducing a symbolic constant
> > > > instead of using 0.
> > > >
> > > > Suggested-by: Stanislav Fomichev <sdf@google.com>
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       |  4 ++++
> > > >  kernel/bpf/disasm.c            |  2 +-
> > > >  kernel/bpf/verifier.c          | 12 +++++++-----
> > > >  tools/include/linux/filter.h   |  2 +-
> > > >  tools/include/uapi/linux/bpf.h |  4 ++++
> > > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index 1503f61336b6..37f7588d5b2f 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type {
> > > >   */
> > > >  #define BPF_PSEUDO_FUNC                4
> > > >
> > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm ==  
> index
> > > of a bpf
> > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full list)
> > > > + */
> > > > +#define BPF_HELPER_CALL                0
> >
> > > I don't like this "cleanup".
> > > The code reads fine as-is.
> >
> > Even in the context of patch 4? There would be the following switch
> > without BPF_HELPER_CALL:
> >
> > switch (insn->src_reg) {
> > case 0:
> >         ...
> >         break;
> >
> > case BPF_PSEUDO_CALL:
> >         ...
> >         break;
> >
> > case BPF_PSEUDO_KFUNC_CALL:
> >         ...
> >         break;
> > }
> >
> > That 'case 0' feels like it deserves a name. But up to you, I'm fine
> > either way.

> It's philosophical.
> Some people insist on if (ptr == NULL). I insist on if (!ptr).
> That's why canonical bpf progs are written as:
> val = bpf_map_lookup();
> if (!val) ...
> zero is zero. It doesn't need #define.

Are you sure we still want to apply the same logic here for src_reg? I
agree that doing src_reg vs !src_reg made sense when we had a "helper"
vs "non-helper" (bpf2bpf) situation. However now this src_reg feels more
like an enum. And since we have an enum value for 1 and 2, it feels
natural to have another one for 0?

That second patch from the series ([0]) might be a good example on why
we actually need it. I'm assuming at some point we've had:
#define BPF_PSEUDO_CALL 1

So we ended up writing `src_reg != BPF_PSEUDO_CALL` instead of actually
doing `src_reg == BPF_HELPER_CALL` (aka `src_reg == 0`).
Afterwards, we've added BPF_PSEUDO_KFUNC_CALL=2 which broke our previous
src_reg vs !src_reg assumptions...

[0]:  
https://lore.kernel.org/bpf/20230215235931.380197-1-iii@linux.ibm.com/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b