Add per-CPU Map Support to Batch Operations

[alxn]

Fixes #251

Follow up to #229 (and I took the test code from that diff, if @nathanjsweet want's to I'll back out the test change).

There's been significant underlying changes since the previous attempt.

I will patch this locally and check my end-to-end example works with this, but the existing tests are passing.

[lmb]

As the person that wrote the per cpu marshaling code: it's some of the gnarlier bits of the library. So thanks for wading through that!

After reading #229 (comment) there are some open questions for me:

• You propose BatchLookup(keys []T, values []T) while per CPU lookup is Lookup(keys T, values *[]T). This is something we need to resolve. Probably the right approach is to support Lookup(values []T) in addition to what we have, and not allow *[]T for batch lookups.
• Why did you go with BatchLookup(values []T) instead of BatchLookup(values [][]T)? The latter seems more ergonomic to use.
• Have you benchmarked your code? My worry with the per CPU stuff was always that it's so hideously expensive that batch or no batch doesn't make a difference.

[lmb]

Can you get rid of the sorting if you use a per cpu array?

[alxn]

Yes, but there is a quirk.

m.BatchLookup() will now not return ErrKeyNotExist because it's not a "key", it's a value in an array. (This was a head scratcher for a while for me).

So that does mean we drop the testing of batchLookupCmd()

	sysErr = wrapMapError(sysErr)
	if sysErr != nil && !errors.Is(sysErr, unix.ENOENT) {
		return 0, sysErr
	}

[lmb]

Then my vote would be to have a simple testcase for ErrKeyNotExist if you think it's necessary?

[alxn]

Yeah, I think I'll fix around this. I do realize the Batch API is one of those interesting dual return APIs that can be confusing, i.e. count>0 and error is set.

There should be a case where batch size == number of keys and that should not return any error I think, it's only when we try to request one more that it fails.

[lmb]

Take a look at the rewritten TestMapBatch: that now uses a contents map to assert that we get the correct values out. We don't test the order anymore, but for hashes that isn't defined and for arrays I'm not bothered enough to care. Net result is that the test is a lot more compact while testing both array and hash.

[alxn]

I can indeed confirm that the CPU marshaling code is hard work!

I meant to follow up on #229 (comment)

You propose BatchLookup(keys []T, values []T) while per CPU lookup is Lookup(keys T, values *[]T). This is something we need to resolve. Probably the right approach is to support Lookup(values []T) in addition to what we have, and not allow *[]T for batch lookups.

The batch APIs BatchLookup and BatchLookupAndDelete have the following comment:

"keysOut" and "valuesOut" must be of type slice, a pointer to a slice or buffer will not work.

With BatchUpdate:

"keys" and "values" must be of type slice, a pointer to a slice or buffer will not work.

Whereas looking at the Lookup() API:

var v uint32
m.Lookup(uint32(0), &v)

var slice []byte
m.Lookup(uint32(0), &slice)

I realize you obviously cannot write in-place to a scalar passed by value, as you can with an slice passed by value.

This shakes out to lookupPerCPU and lookupAndDeletePerCPU in:

// slicePtr must be a pointer to a slice.
func unmarshalPerCPUValue(slicePtr any, elemLength int, buf []byte) error {

So supporting Lookup(values []T) would mean:

var v uint32
m.Lookup(uint32(0), &v) // Lookup a single value

var v []uint32
m.Lookup(uint32(0), v) // Lookup a per-CPU value

I suspect the right answer would have been to only support BatchLookup(keys []T, values *[]T) originally but here we are.

Why did you go with BatchLookup(values []T) instead of BatchLookup(values [][]T)? The latter seems more ergonomic to use.

I actually agree that [batch][cpus]T is a lot more ergonomic to use from a user perspective, but I was wary of changing the existing pattern.

var v []uint32
m.Lookup(uint32(0), v) // Lookup per-CPU

var (
    nextKey uint32
    lookupKeys   = make([]uint32, 2)
    lookupValues = make([]uint32, 2)
)
m.BatchLookup(nil, &nextKey, lookupKeys, lookupValues, nil) // Lookup a Batch

Also because of pass by value, uses will have to do the following that we'll need to validate.

lookupValues := make([][]uint32, batchSize)
for i, _ := range lookupValues {
    lookupValues[i] = make([]uint32, possibleCPUs)
}

I also partly wonder if []T makes more sense because it aligns with the BPF syscall.

Have you benchmarked your code? My worry with the per CPU stuff was always that it's so hideously expensive that batch or no batch doesn't make a difference.

No not yet. I had assumed that crossing the kernel boundary was the most expensive part of the operation, I have some near production code I wanted to use this API on.

[alxn]

$ go test -exec sudo  . -bench BenchmarkPerCPUMarshalling
goos: linux
goarch: arm64
pkg: github.com/cilium/ebpf
BenchmarkPerCPUMarshalling/Lookup-4         	    1766	    677450 ns/op	   98304 B/op	    4096 allocs/op
BenchmarkPerCPUMarshalling/BatchLookup-4    	    5134	    238291 ns/op	   65600 B/op	    1029 allocs/op
PASS
ok  	github.com/cilium/ebpf	3.507s

[alxn]

Also I believe I need to solve the issue of libbpf_num_possible_cpus().

[lmb]

I've given this some more thought and I think we need to expand the scope of the work to get per-CPU batch ops done. And because of the increased scope we'd have to split it into multiple PRs. Something like the following:

1. Support doing a per-CPU lookup into pre-allocated slices via Lookup(values []T). This will involve figuring out an equivalent of libbpf_num_possible_cpus() as you point out. We'll also have to stop allocating slices via reflect in the marshal / unmarshal path. Instead we should always write into the backing buffer and then have special casing for *[]T in Lookup, etc. (or as close to the top of the stack as possible).
2. On top of clean per-CPU semantics for []T add batch support for []T * n (more likely) or [][]T.

I realise that isn't what you signed up for, sorry.

So supporting Lookup(values []T) would mean:

var v uint32
m.Lookup(uint32(0), &v) // Lookup a single value

var v []uint32
m.Lookup(uint32(0), v) // Lookup a per-CPU value

It would be a bit more complicated / subtle: m.Lookup(uint32(0), &v) must also still work. Semantics:

• m.Lookup on a per cpu map with a *[]T will have the current behaviour: the lib reallocates the backing slice.
• m.Lookup on a per cpu map with []T will only write into the pre-allocated buffer.

Otherwise the two should behave the same.

I suspect the right answer would have been to only support BatchLookup(keys []T, values *[]T) originally but here we are.

Actually, requiring *[]T in Lookup was the mistake. Baking allocation decisions into an API always comes back to haunt you...

I also partly wonder if []T makes more sense because it aligns with the BPF syscall.

That is a good point. It only makes a difference if we were ever to directly pass the buffer to the syscall though, which is doable but pretty tricky. Overall []T will be faster though, just less nice to use.

Also I believe I need to solve the issue of libbpf_num_possible_cpus().

We already have internal.PossibleCPUs() but that in practice is annoying to use since if forces error checking when allocating a slice.

n, err := ebpf.PossibleCPUs()
// ...
buf := make([]uint32, n)

This is the original reason for taking *[]T.

Maybe the easiest would be to add two functions:

func PossibleCPUs() (int, error)
func MustPossibleCPUs() int

The latter would panic if an error occurred.

[alxn]

Lookup(values []T) makes a lot of sense, I'll rework it so the *[]T is only dealt with right at the top of the stack, and then the rest of the code will assume a correctly sized slice. (i.e. this means top level validation that the slice is correct, and not needing to grow deep in the unmarshal).

I still wonder if BatchLookup([][]T) is better, I again got burnt by that in my integration code with this patch. It would be a little annoying for the users and validation though.

I assume for PossibleCPUs() that I'll pull the implementation up a level and rewrite the existing uses cases (rather than put a public Must decorator over the internal).

• It should be a sync.Once function as well I assume.

[lmb]

I still wonder if BatchLookup([][]T) is better, I again got burnt by that in my integration code with this patch. It would be a little annoying for the users and validation though.

Maybe we add an allocation function for a typed slice? func AllocBatch[T any, I constraints.Int](n I) []T or similar.

Another tricky case: batch lookup of []byte in a per CPU map. Not even sure what that looks like at the moment.

I assume for PossibleCPUs() that I'll pull the implementation up a level and rewrite the existing uses cases

Yes, as long as that doesn't run into import cycles.

[alxn]

I've given this some more thought and I think we need to expand the scope of the work to get per-CPU batch ops done. And because of the increased scope we'd have to split it into multiple PRs. Something like the following:

1. Support doing a per-CPU lookup into pre-allocated slices via `Lookup(values []T)`. This will involve figuring out an equivalent of `libbpf_num_possible_cpus()` as you point out. We'll also have to stop allocating slices via reflect in the marshal / unmarshal path. Instead we should always write into the backing buffer and then have special casing for `*[]T` in `Lookup`, etc. (or as close to the top of the stack as possible).

Ok these two are in:

• bcca828
• e1bf3cc

2. On top of clean per-CPU semantics for `[]T` 

So supporting Lookup(values []T) would mean:

var v uint32
m.Lookup(uint32(0), &v) // Lookup a single value

var v []uint32
m.Lookup(uint32(0), v) // Lookup a per-CPU value

It would be a bit more complicated / subtle: m.Lookup(uint32(0), &v) must also still work. Semantics:

* `m.Lookup` on a per cpu map with a `*[]T` will have the current behaviour: the lib reallocates the backing slice.

* `m.Lookup` on a per cpu map with `[]T` will only write into the pre-allocated buffer.

Otherwise the two should behave the same.

And now all of the above is true.

add batch support for []T * n (more likely) or [][]T.

I suspect the right answer would have been to only support BatchLookup(keys []T, values *[]T) originally but here we are.

Actually, requiring *[]T in Lookup was the mistake. Baking allocation decisions into an API always comes back to haunt you...

I also partly wonder if []T makes more sense because it aligns with the BPF syscall.

That is a good point. It only makes a difference if we were ever to directly pass the buffer to the syscall though, which is doable but pretty tricky. Overall []T will be faster though, just less nice to use.

Maybe we add an allocation function for a typed slice? func AllocBatch[T any, I constraints.Int](n I) []T or similar.

So I'm just looking at this support now:

BatchLookup(keys []T, values []T
MakeBatch[T any, I constraints.Int](n I) []T

• with an Append() style.

But I did notice that MakeBatch() needs to know if it's PerCPU or not (and then there's an err for PossibleCPU(), unless passed in.

[lmb]

Let's drop the allocation helper MakeBatch or whatever for now and focus on the implementation. In the best case it's a bit of syntax sugar.

One thing I wondered about when reading your summary: in your previous PR you added a test which does a []byte lookup in a regular map (IIRC). What does it currently look like if you want to look up []byte in a per CPU map without batch support? That might inform our decision of []T vs [][]T.

[alxn]

One thing I wondered about when reading your summary: in your previous PR you added a test which does a []byte lookup in a regular map (IIRC). What does it currently look like if you want to look up []byte in a per CPU map without batch support? That might inform our decision of []T vs [][]T.

I didn't add a []byte lookup in #1220, that was already there in TestMap():

var slice []byte
qt.Assert(t, m.Lookup(uint32(0), &slice), qt.IsNil)

I added the slice lookup for the same data:

sliceVal := make([]uint32, 1)
qt.Assert(t, m.Lookup(uint32(0), sliceVal), qt.IsNil)

I note that the []byte array does return the correct response [42 0 0 0]

If I make this a PerCPUArray and try it

const valueSize = 4
m, err := NewMap(&MapSpec{
	Type:       PerCPUArray,
	KeySize:    4,
	ValueSize:  valueSize,
	MaxEntries: 2,
})
...
v := make([]byte, valueSize*possibleCPUs)

Passing the slice by value:

if err := m.Lookup(uint32(0), v); err != nil {

=>

Can't lookup 0: per-CPU slice has incorrect length, expected 4, got 16

Passing the pointer:

Can't lookup 0: cpu 0: unmarshaling *uint8 doesn't consume all data

Passing make([]uint32, possibleCPUs) works by ref or value.

I guess []byte is interesting, and the C viewpoint that everything is just a char *. It probably makes no sense to support looking up a uint32 array using a half length sized uint64 array.

[alxn]

I finally had enough contiguous time to figure out "append()" style.

Also quick question, is it possible to get a buf out of sys.Pointer{}... I hacked an API work around so that I could write an e2e test for the marshalers. (061c8b7)

[alxn]

Error: marshalers.go:8:2: package slices is not in GOROOT (/opt/hostedtoolcache/go/1.20.11/x64/src/slices)

Ahem. I can manually write this I guess?

[lmb]

I can manually write this I guess?

Use x/exp/slices.

[lmb]

Also quick question, is it possible to get a buf out of sys.Pointer{}

Not 100% sure what you mean, but sys.Pointer has no length information. So I'd be reluctant to add API which turns it into a []byte if that is what you mean.

[alxn]

Also quick question, is it possible to get a buf out of sys.Pointer{}

Not 100% sure what you mean, but sys.Pointer has no length information. So I'd be reluctant to add API which turns it into a []byte if that is what you mean.

I'd written a full e2e in (061c8b7), but marshalBatchPerCPUValue() returns sys.Pointer{buf}... which I can't pass to unmarshalBatchPerCPUValue().

[alxn]

Oh my the merge conflicts on d9f8904

[alxn]

@lmb one question - I did switch the checks to != rather than magical auto padding. Updated the commit comment with that as well.

[lmb]

I did switch the checks to != rather than magical auto padding. Updated the commit comment with that as well.

You mean for batch per-CPU marshaling? Yes, I think that is the right thing to do.

[lmb]

Thanks for all your hard work!