On 07/19, Alexei Starovoitov wrote:
On Tue, Jul 19, 2022 at 2:41 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On Tue, Jul 19, 2022 at 1:33 PM Stanislav Fomichev <sdf@xxxxxxxxxx>
wrote:
> >
> > On Tue, Jul 19, 2022 at 1:21 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Mon, Jul 18, 2022 at 12:07 PM Stanislav Fomichev
<sdf@xxxxxxxxxx> wrote:
> > > >
> > > > Motivation:
> > > >
> > > > Our bpf programs have a bunch of options which are set at the
loading
> > > > time. After loading, they don't change. We currently use array map
> > > > to store them and bpf program does the following:
> > > >
> > > > val = bpf_map_lookup_elem(&config_map, &key);
> > > > if (likely(val && *val)) {
> > > > // do some optional feature
> > > > }
> > > >
> > > > Since the configuration is static and we have a lot of those
features,
> > > > I feel like we're wasting precious cycles doing dynamic lookups
> > > > (and stalling on memory loads).
> > > >
> > > > I was assuming that converting those to some fake kconfig options
> > > > would solve it, but it still seems like kconfig is stored in the
> > > > global map and kconfig entries are resolved dynamically.
> > > >
> > > > Proposal:
> > > >
> > > > Resolve kconfig options statically upon loading. Basically rewrite
> > > > ld+ldx to two nops and 'mov val, x'.
> > > >
> > > > I'm also trying to rewrite conditional jump when the condition is
> > > > !imm. This seems to be catching all the cases in my program, but
> > > > it's probably too hacky.
> > > >
> > > > I've attached very raw RFC patch to demonstrate the idea. Anything
> > > > I'm missing? Any potential problems with this approach?
> > >
> > > Have you considered using global variables for that?
> > > With skeleton the user space has a natural way to set
> > > all of these knobs after doing skel_open and before skel_load.
> > > Then the verifier sees them as readonly vars and
> > > automatically converts LDX into fixed constants and if the code
> > > looks like if (my_config_var) then the verifier will remove
> > > all the dead code too.
> >
> > Hm, that's a good alternative, let me try it out. Thanks!
>
> Turns out we already freeze kconfig map in libbpf:
> if (map_type == LIBBPF_MAP_RODATA || map_type == LIBBPF_MAP_KCONFIG) {
> err = bpf_map_freeze(map->fd);
>
> And I've verified that I do hit bpf_map_direct_read in the verifier.
>
> But the code still stays the same (bpftool dump xlated):
> 72: (18) r1 = map[id:24][0]+20
> 74: (61) r1 = *(u32 *)(r1 +0)
> 75: (bf) r2 = r9
> 76: (b7) r0 = 0
> 77: (15) if r1 == 0x0 goto pc+9
>
> I guess there is nothing for sanitize_dead_code to do because my
> conditional is "if (likely(some_condition)) { do something }" and the
> branch instruction itself is '.seen' by the verifier.
I bet your variable is not 'const'.
Please see any of the progs in selftests that do:
const volatile int var = 123;
to express configs.
Yeah, I was testing against the following:
extern int CONFIG_XYZ __kconfig __weak;
But ended up writing this small reproducer:
struct __sk_buff;
const volatile int CONFIG_DROP = 1; // volatile so it's not
// clang-optimized
__attribute__((section("tc"), used))
int my_config(struct __sk_buff *skb)
{
int ret = 0; /*TC_ACT_OK*/
if (CONFIG_DROP)
ret = 2 /*TC_ACT_SHOT*/;
return ret;
}
$ bpftool map dump name my_confi.rodata
[{
"value": {
".rodata": [{
"CONFIG_DROP": 1
}
]
}
}
]
$ bpftool prog dump xlated name my_config
int my_config(struct __sk_buff * skb):
; if (CONFIG_DROP)
0: (18) r1 = map[id:3][0]+0
2: (61) r1 = *(u32 *)(r1 +0)
3: (b4) w0 = 1
; if (CONFIG_DROP)
4: (64) w0 <<= 1
; return ret;
5: (95) exit
The branch is gone, but the map lookup is still there :-(
