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 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().
