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 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.