I would return here if we are not able to get the settings. There is the risk we will write junk at the next update

Makes sense, done!

it sounds like pman_update_capture_settings with an initial struct capture_settings settings = {}; maybe we can avoid duplication

Good catch, implemented your suggestion

This is the main reason we chose a memory-mapped global variable initially. Calling a bpf helper many times (more than 20 Millions/s) could decrease perf. We are probably talking about a few ns, but under heavy loads, it could have a little impact

As far as we get it, BPF_F_MMAPABLE flag mmaps the eBPF maps to userspace memory, increasing perf for userspace only; in eBPF, the read and writes still pass through the helpers.
Perhaps we are wrong though?
See:
https://docs.kernel.org/bpf/map_array.html

Since kernel 5.5, memory mapping may be enabled for BPF_MAP_TYPE_ARRAY by setting the flag BPF_F_MMAPABLE. The map definition is page-aligned and starts on the first page. Sufficient page-sized and page-aligned blocks of memory are allocated to store all array values, starting on the second page, which in some cases will result in over-allocation of memory. The benefit of using this is increased performance and ease of use since userspace programs would not be required to use helper functions to access and mutate data.

https://docs.ebpf.io/linux/map-type/BPF_MAP_TYPE_ARRAY/#bpf_f_mmapable

Setting this flag on a BPF_MAP_TYPE_ARRAY will allow userspace programs to mmap the array values into the userspace process, effectively making a shared memory region between eBPF programs and a userspace program.
This can significantly improve read and write performance since there is no syscall overhead to access the map.

https://patchwork.ozlabs.org/project/netdev/patch/20191117172806.2195367-4-andriin@fb.com/

Add ability to memory-map contents of BPF array map. This is extremely useful
for working with BPF global data from userspace programs. It allows to avoid
typical bpf_map_{lookup,update}_elem operations, improving both performance
and usability.

@FedeDP is right, that's what we figured during the analysis for it. The only doubt I had about this is some extra perf optimization that might happen because of how libbpf handles .bss, .rodata and .data with a special instruction: https://docs.ebpf.io/linux/map-type/BPF_MAP_TYPE_ARRAY/#global-data .

It is unclear whether that has performance impact, I agree that the easiest way is to keep an eye on perf after it's merged.

Uhm if I recall it correctly, the global variables are accessed without helpers but just using direct memory access. I quickly tried it

char LICENSE[] SEC("license") = "Dual BSD/GPL";

unsigned long stop = 0;

SEC("tp_btf/sys_enter")
int global(void *ctx) {
	stop++;
	return 0;
}

struct {
	__uint(type, BPF_MAP_TYPE_ARRAY);
	__uint(max_entries, 1);
	__type(key, uint32_t);
	__type(value, unsigned long);
} stop_map SEC(".maps");

SEC("tp_btf/sys_enter")
int map_array(void *ctx) {
	uint32_t key = 0;
	unsigned long *value = bpf_map_lookup_elem(&stop_map, &key);
	*value++;
	return 0;
}

Globals:

0: R1=ctx() R10=fp0
; stop++; @ globals.bpf.c:12
0: (18) r1 = 0xffffbfb7c1226000       ; R1_w=map_value(map=globals_.bss,ks=4,vs=8)
2: (79) r2 = *(u64 *)(r1 +0)          ; R1_w=map_value(map=globals_.bss,ks=4,vs=8) R2_w=scalar()
3: (07) r2 += 1                       ; R2_w=scalar()
4: (7b) *(u64 *)(r1 +0) = r2          ; R1_w=map_value(map=globals_.bss,ks=4,vs=8) R2_w=scalar()
; return 0; @ globals.bpf.c:13
5: (b7) r0 = 0                        ; R0_w=0
6: (95) exit
processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

Map:

0: R1=ctx() R10=fp0
; int map_array(void *ctx) { @ globals.bpf.c:24
0: (b7) r1 = 0                        ; R1_w=0
; uint32_t key = 0; @ globals.bpf.c:25
1: (63) *(u32 *)(r10 -4) = r1
mark_precise: frame0: last_idx 1 first_idx 0 subseq_idx -1 
mark_precise: frame0: regs=r1 stack= before 0: (b7) r1 = 0
2: R1_w=0 R10=fp0 fp-8=0000????
2: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
;  @ globals.bpf.c:0
3: (07) r2 += -4                      ; R2_w=fp-4
; unsigned long *value = bpf_map_lookup_elem(&stop_map, &key); @ globals.bpf.c:26
4: (18) r1 = 0xffff9a8a4296b400       ; R1_w=map_ptr(map=stop_map,ks=4,vs=8)
6: (85) call bpf_map_lookup_elem#1    ; R0_w=map_value_or_null(id=1,map=stop_map,ks=4,vs=8)
; return 0; @ globals.bpf.c:28
7: (b7) r0 = 0                        ; R0_w=0
8: (95) exit
processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

Probably looking at the bytecode of our probe before/after we should see the same

Cool thanks for clarifying it! I guess you are right then, the emitted code is more optimized with the globals.

Hi @Andreagit97 , makes sense, but fortunately the kernel is helping us by optimizing it away.

Indeed, the two compiled programs look different:

Using a map (i.e. this patch):

0000000000000090 <LBB0_26>:
      18:       63 7a f8 ff 00 00 00 00 *(u32 *)(r10 - 0x8) = r7
      19:       bf a2 00 00 00 00 00 00 r2 = r10
      20:       07 02 00 00 f8 ff ff ff r2 += -0x8
      21:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
      23:       85 00 00 00 01 00 00 00 call 0x1

Using .bss:

0000000000000090 <LBB0_26>:
      18:       bf 81 00 00 00 00 00 00 r1 = r8
      19:       57 01 00 00 ff 01 00 00 r1 &= 0x1ff
      20:       18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x0 ll
      22:       0f 12 00 00 00 00 00 00 r2 += r1
      23:       71 22 00 00 00 00 00 00 r2 = *(u8 *)(r2 + 0x0)

In the compiled object file. There is obviously a call. However, at load time the kernel is able to optimize this since it's an (almost)fixed-size array map; this uses more instructions than the .bss version but I guess it's not as bad as an actual call, let's take a look:

# bpftool prog dump xlated name sys_enter opcodes
...
  17: (18) r1 = map[id:8834]
       18 11 00 00 82 22 00 00 00 00 00 00 00 00 00 00
  19: (07) r1 += 272
       07 01 00 00 10 01 00 00
  20: (61) r0 = *(u32 *)(r2 +0)
       61 20 00 00 00 00 00 00
  21: (35) if r0 >= 0x200 goto pc+3
       35 00 03 00 00 02 00 00
  22: (67) r0 <<= 3
       67 00 00 00 03 00 00 00
  23: (0f) r0 += r1
       0f 10 00 00 00 00 00 00
  24: (05) goto pc+1
       05 00 01 00 00 00 00 00
  25: (b7) r0 = 0
       b7 00 00 00 00 00 00 00
; if(ret == NULL) {
  26: (55) if r0 != 0x0 goto pc+30
       55 00 1e 00 00 00 00 00
...
; return *ret;
  57: (71) r1 = *(u8 *)(r0 +0)
       71 01 00 00 00 00 00 00

Originally we had 5 instructions, in the map version we have 10 instructions at the eBPF level. Not great but acceptable I think, also I'm not sure what happens at native code jit compile time or which kernel versions are able to perform this optimization.

uhm, I see, I never noticed that! Yes, in this case, the overhead seems negligible

If this is called frequently it could have an impact as well but AFAIK we don't call it so frequently at runtime

Exactly , this is not a problem (as far as i know, actually nobody calls this at runtime, only at engine init time. Also given we have ~500 ppm_sc codes, i don't expect people to call it more than like ~100 times for each batch).

There's no noticeable difference in startup times, and even if the client would dynamically set the interesting syscall set it wouldn't do it that frequently. I toyed with the idea of updating more elements in a single call but I don't think it's worth it now