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

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

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

struct bpf_loop_inline_state {
	int initialized:1; /* set to true upon first entry */
	int not_fit_for_inline:1; /* false if callback function is thesame
				   * at each call and flags are always zero
				   */
	u32 callback_subprogno; /* valid when fit_for_inline is true */
};

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 =
		register_is_const(flags_reg) && flags_reg->var_off.value == 0;

	if (state->not_fit_for_inline)
		return;

	if (state->initialized) {
		state->not_fit_for_inline |=
			!flags_is_zero ||
			state->callback_subprogno != subprogno;
	} else {
		state->initialized = 1;
		state->not_fit_for_inline = !flags_is_zero;
		state->callback_subprogno = subprogno;
	}
}

// ...

static int optimize_bpf_loop(struct bpf_verifier_env *env)
{
	// ...
		if (is_bpf_loop_call(insn) && !inline_state->not_fit_for_inline) {
	// ...
}

IMO, the code is less clear after such rewrite, also
`update_loop_inline_state` is not on a hot path (it is called from
`check_helper_call` only when helper is `bpf_loop`). Are you sure this
rewrite is necessary?

Thanks,
Eduard