On Fri, Feb 17, 2023 at 8:19 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Feb 17, 2023 at 2:57 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
> >
> > On Thu, 2023-02-16 at 10:03 -0800, Stanislav Fomichev wrote:
> > > On 02/16, Alexei Starovoitov wrote:
> > > > On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > > wrote:
> > > > >
> > > > > On 02/16, Alexei Starovoitov wrote:
> > > > > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich
> > > > > > <iii@xxxxxxxxxxxxx>
> > > > > > wrote:
> > > > > > >
> > > > > > > Make the code more readable by introducing a symbolic
> > > > > > > constant
> > > > > > > instead of using 0.
> > > > > > >
> > > > > > > Suggested-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > > > > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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@xxxxxxxxxxxxx/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b
> >
> > FWIW the helper checks before this series had inconsistent style:
> >
> > - !insn->src_reg
> > - insn->src_reg == 0
> > - insn->src_reg != BPF_REG_0
> > - insn[i].src_reg != BPF_PSEUDO_CALL
> >
> > Now at least it's the same style everywhere, and also it's easy to
> > grep for "where do we check for helper calls".
>
> The above checks are not equivalent.
> Comparing src_reg with BPF_REG_0 makes sense in one context
> and doesn't in the other.
> It's never ok to add stuff to uapi when it works as-is.
> I also don't buy theoretical arguments about future additions
> and how something will be cleaner in the future because
> we predicted it so well today.
SG! Then let's maybe respin without this part? I might have derailed
the conversation too much from the actual issue :-[
