On Thu, May 4, 2023 at 9:28 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Thu, May 4, 2023 at 9:04 AM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Tue, Apr 25, 2023 at 04:49:07PM -0700, Andrii Nakryiko wrote: > > > + > > > + err = mark_chain_precision_batch(env, old->curframe); > > > > I think I'm sort-of starting to get it, but > > above should be env->cur_state->curframe instead of old->curframe, no? > > mark_chain_precision_batch will be using branch history of current frame. > > __mark_chain_precision() always operates on > > struct bpf_verifier_state *st = env->cur_state; > > wait. patch 5 made __mark_chain_precision() to ignore 'frame' argument. > Why did you keep it in mark_chain_precision_batch() and pass it here? Ok, few things here. 1. We don't ignore frame, bt_init(bt, frame) is what sets that for backtrack_state. Currently we get it as input argument, but see below, we can get it from the current state now. 2. old->curframe and cur_state->curframe should be the same because two states are equivalent. I chose to pass old->curframe because that's what existing for loop was using for iteration. But you are right, it's a missed opportunity to simplify. __mark_chain_precision() always works with env->cur_state and should always start from its deepest frame, so I'll drop the frame argument from __mark_chain_precision() completely and will be getting it from cur_state. Good observation!
