cilium/ebpf

Add BatchLookup support for a pointer to a slice

brycekahle opened this issue · 8 comments

BatchLookup claims it doesn't support a pointer to a slice, but sysenc.SyscallOutput does support pointers to slices:

// We're dealing with a pointer to a slice. Dereference and
// handle it like a regular slice.
value = value.Elem()

Could batchCount and sliceLen be updated to support pointer to slices?

Could it be that it's the intended outcome? I read it as "keysOut" and "valuesOut" must be of type slice. A pointer to a slice or buffer will not work

You are right, I misread the comment.

That being said, the later functions do work with a pointer to a slice:

// We're dealing with a pointer to a slice. Dereference and
// handle it like a regular slice.
value = value.Elem()

Reworded to be a feature request

One of the motivations here is to avoid the 24 byte allocation necessary to pass the slice by value.

lmb commented

Can you add an example? The idea with BatchLookup is to use PossibleCPU() to create an appropriately sized slice and then reuse that for calls to the method. See #1271 though.

I think you are talking specifically about per-CPU, but my request applies even for normal maps.

Example:

// m is *ebpf.Map
keys := make([]uint32, 10)
values := make([]uint32, 10)

// current usage: allocates 24 bytes x 2 for the slice headers, but no more because of sysenc.SyscallOutput and unsafeBackingMemory
m.BatchLookup(keys, values, nil)

// proposed usage with no allocations
m.BatchLookup(&keys, &values, nil)
lmb commented

Okay, I guess that is because the slice headers need to go onto the heap since we turn them into any. Why are those allocations a problem for you? In my mind

ebpf/map_test.go

Lines 156 to 164 in 394be60

lookupKeys := make([]uint32, n)
lookupValues := make([]uint32, n*possibleCPU)
var cursor BatchCursor
var total int
for {
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
total += count
if errors.Is(err, ErrKeyNotExist) {
will lead to two allocations per entire iteration of the map, which doesn't seem too bad to me. Maybe you can add an example?

The problem with pointer-to-slice for per-CPU maps is that they already have semantics for non-batch lookups. For legacy reasons they will cause the library to allocate the target slice. If we added pointer-to-slice for BatchLookup it would have to match those semantics so that we don't fully descend into madness (I already find it difficult to keep the rules straight). That in turn means it wouldn't be very useful for you due to additional allocations.