On Fri, Oct 9, 2020 at 5:10 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 10/10/20 1:01 AM, Andrii Nakryiko wrote:
> > On Fri, Oct 9, 2020 at 3:40 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> [...]
> >> *insn++ = BPF_ALU64_IMM(BPF_ADD, map_ptr, offsetof(struct bpf_array, value));
> >> *insn++ = BPF_LDX_MEM(BPF_W, ret, index, 0);
> >> if (!map->bypass_spec_v1) {
> >> @@ -496,8 +499,10 @@ static int array_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
> >> static bool array_map_meta_equal(const struct bpf_map *meta0,
> >> const struct bpf_map *meta1)
> >> {
> >> - return meta0->max_entries == meta1->max_entries &&
> >> - bpf_map_meta_equal(meta0, meta1);
> >> + if (!bpf_map_meta_equal(meta0, meta1))
> >> + return false;
> >> + return meta0->map_flags & BPF_F_INNER_MAP ? true :
> >> + meta0->max_entries == meta1->max_entries;
> >
> > even if meta1 doesn't have BPF_F_INNER_MAP, it's ok, because all the
> > accesses for map returned from outer map lookup will not inline, is
> > that right? So this flag only matters for the inner map's prototype.
>
> Not right now, we would have to open code bpf_map_meta_equal() to cut out that
> bit from the meta0/1 flags comparison. I wouldn't change bpf_map_meta_equal()
> itself given that bit can be reused for different purpose for other map types.
>
> > You also mentioned that not inlining array access should still be
> > fast. So I wonder, what if we just force non-inlined access for inner
> > maps of ARRAY type? Would it be too bad of a hit for existing
> > applications?
>
> Fast in the sense of that we can avoid a retpoline given the direct call
Ah, ok, then probably an extra flag is necessary.
> to array_map_lookup_elem() as opposed to bpf_map_lookup_elem(). In the
> array_map_gen_lookup() we even have insn level optimizations such as
> replacing BPF_MUL with BPF_LSH with immediate elem size on power of 2
> #elems as well as avoiding spectre masking (which the call one has not),
> presumably for cases like XDP we might want the best implementation if
> usage allows it.
>
> > The benefit would be that everything would just work without a special
> > flag. If perf hit isn't prohibitive, it might be worthwhile to
> > simplify user experience?
>
> Taking the above penalty aside for same sized-elems, simplest one would have
> been to just set inner_map_meta->ops to &array_map_no_inline_ops inside the
> bpf_map_meta_alloc().
