Re: [PATCH bpf-next v4 3/5] bpf: Inline calls to bpf_loop when callback is known

[Song Liu <song@xxxxxxxxxx>]

On Fri, Jun 10, 2022 at 2:55 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> > On Fri, 2022-06-10 at 13:54 -0700, Song Liu wrote:
>
> > > +
> > > +void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno)
> >
> > static void ...
> >
> > > +{
> > > +       struct bpf_loop_inline_state *state = &cur_aux(env)->loop_inline_state;
> > > +       struct bpf_reg_state *regs = cur_regs(env);
> > > +       struct bpf_reg_state *flags_reg = &regs[BPF_REG_4];
> > > +
> >
> > nit: we usually don't have empty lines here.
> >
> > > +       int flags_is_zero =
> > > +               register_is_const(flags_reg) && flags_reg->var_off.value == 0;
> >
> > If we replace "fit_for_inline" with "not_fit_for_inline", we can make the cannot
> > inline case faster with:
> >
> >   if (state->not_fit_for_inline)
> >       return;
> >
> > > +
> > > +       if (state->initialized) {
> > > +               state->fit_for_inline &=
> > > +                       flags_is_zero &&
> > > +                       state->callback_subprogno == subprogno;
> > > +       } else {
> > > +               state->initialized = 1;
> > > +               state->fit_for_inline = flags_is_zero;
> > > +               state->callback_subprogno = subprogno;
> > > +       }
> > > +}
> > > +
>
> Sorry, I'm not sure that I understand you correctly. Do you want me to
> rewrite the code as follows:

Yes, I was thinking about this change. I guess it can also be clear:

static void update_loop_inline_state(struct bpf_verifier_env *env, u32
subprogno)
{
        struct bpf_loop_inline_state *state = &cur_aux(env)->loop_inline_state;
        struct bpf_reg_state *regs = cur_regs(env);
        struct bpf_reg_state *flags_reg = &regs[BPF_REG_4];
        int flags_is_zero;

        if (state->cannot_inline)
                return;

        flags_is_zero = register_is_const(flags_reg) &&
flags_reg->var_off.value == 0;

        if (!state->initialized) {
                state->initialized = 1;
                state->cannot_inline = !flags_is_zero;
                state->callback_subprogno = subprogno;
                return;
        }

        state->cannot_inline = !flags_is_zero ||
                state->callback_subprogno != subprogno;
}

What do you think about this version?

Thanks,
Song