On Mon, Nov 11, 2024 at 1:53 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Mon, Nov 11, 2024 at 10:46 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Wed, Oct 30, 2024 at 4:51 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > >
> > > Async callback state enqueing, while logically detached from parent
> >
> > typo. enqueuing
>
> yep, tricky word :) should be "enqueueing", fixed
>
> >
> > > -static int get_prev_insn_idx(struct bpf_verifier_state *st, int i,
> > > - u32 *history)
> > > +static int get_prev_insn_idx(const struct bpf_verifier_env *env,
> > > + struct bpf_verifier_state *st,
> > > + int insn_idx, u32 hist_start, u32 *hist_endp)
> > > {
> > > - u32 cnt = *history;
> > > + u32 hist_end = *hist_endp;
> > > + u32 cnt = hist_end - hist_start;
> > >
> > > - if (i == st->first_insn_idx) {
> > > + if (insn_idx == st->first_insn_idx) {
> > > if (cnt == 0)
> > > return -ENOENT;
> > > - if (cnt == 1 && st->jmp_history[0].idx == i)
> > > + if (cnt == 1 && env->insn_hist[hist_end - 1].idx == insn_idx)
> > > return -ENOENT;
> > > }
> >
> > I think the above bit would be easier to understand if it was
> > env->insn_hist[hist_start].
> >
> > When cnt==1 it's the same as hist_end-1, but it took me more time
> > to grok that part. With [hist_start] would have been easier.
> > Not a big deal.
>
> yep, I agree. Originally I didn't pass hist_start directly, so I would
> have to use st->insn_hist_start, and it felt too verbose. But now
> that's not a problem, I'll use hist_start everywhere.
>
> >
> > Another minor suggestion...
> > wouldn't it be cleaner to take hist_start/end from 'st' both
> > in get_prev_insn_idx() and in get_insn_hist_entry() ?
> >
> > So that __mark_chain_precision() doesn't need to reach out into
> > details of 'st' just to pass hist_start/end values into other helpers.
>
> Note that for get_prev_insn_idx() we modify (but only locally!)
> hist_end, as we process instruction history for the currently
> processed state (we do a virtual stack pop for each entry). So we
> can't just use st->insn_hist_end, we need a local copy for hist_end
> that will be updated without touching the actual insn_hist_end. That's
> the reason I have `u32 hist_end = st->insn_hist_end;`, to pass
> &hist_end into get_prev_insn_idx().
>
> Having said that, if you prefer, I can fetch insn_hist_{start, end}
> from st, always, but then maintain local hist_cnt as input argument
> for get_insn_hist_enrty() and in/out argument for get_prev_insn_idx().
> Would you prefer that? something like below:
>
Argh, gmail messed this up. See [0] for better formatting.
[0] https://gist.github.com/anakryiko/25228b0ae2760f78b7ae7f0160faa5c1
