On Fri, Apr 26, 2024 at 9:12 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Apr 26, 2024 at 3:20 AM Cupertino Miranda > <cupertino.miranda@xxxxxxxxxx> wrote: > > > > > > Andrii Nakryiko writes: > > > > > On Wed, Apr 24, 2024 at 3:41 PM Cupertino Miranda > > > <cupertino.miranda@xxxxxxxxxx> wrote: > > >> > > >> Split range computation checks in its own function, isolating pessimitic > > >> range set for dst_reg and failing return to a single point. > > >> > > >> Signed-off-by: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx> > > >> Cc: Yonghong Song <yonghong.song@xxxxxxxxx> > > >> Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> > > >> Cc: David Faust <david.faust@xxxxxxxxxx> > > >> Cc: Jose Marchesi <jose.marchesi@xxxxxxxxxx> > > >> Cc: Elena Zannoni <elena.zannoni@xxxxxxxxxx> > > >> --- > > >> kernel/bpf/verifier.c | 141 +++++++++++++++++++++++------------------- > > >> 1 file changed, 77 insertions(+), 64 deletions(-) > > >> > > > > > > I know you are moving around pre-existing code, so a bunch of nits > > > below are to pre-existing code, but let's use this as an opportunity > > > to clean it up a bit. > > > > > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > >> index 6fe641c8ae33..829a12d263a5 100644 > > >> --- a/kernel/bpf/verifier.c > > >> +++ b/kernel/bpf/verifier.c > > >> @@ -13695,6 +13695,82 @@ static void scalar_min_max_arsh(struct bpf_reg_state *dst_reg, > > >> __update_reg_bounds(dst_reg); > > >> } > > >> > > >> +static bool is_const_reg_and_valid(struct bpf_reg_state reg, bool alu32, > > > > > > hm.. why passing reg_state by value? Use pointer? > > > > > Someone mentioned this in a review already and I forgot to change it. > > Apologies if I did not reply on this. > > > > The reason why I pass by value, is more of an approach to programming. > > I do it as guarantee to the caller that there is no mutation of > > the value. > > If it is better or worst from a performance point of view it is > > arguable, since although it might appear to copy the value it also provides > > more information to the compiler of the intent of the callee function, > > allowing it to optimize further. > > I personally would leave the copy by value, but I understand if you want > > to keep having the same code style. > > It's a pretty big 120-byte structure, so maybe the compiler can > optimize it very well, but I'd still be concerned. Hopefully it can > optimize well even with (const) pointer, if inlining. > > But I do insist, if you look at (most? I haven't checked every single > function, of course) other uses in verifier.c, we pass things like > that by pointer. I understand the desire to specify the intent to not > modify it, but that's why you are passing `const struct bpf_reg_state > *reg`, so I think you don't lose anything with that. +1 that "struct bpf_reg_state src_reg" code was written 7 years ago when bpf_reg_state was small. We definitely need to fix it. It might even bring a noticeable runtime improvement.
