Skip to content

Commit

Permalink
map: fix TestMapBatch failing with ENOSPC for hash maps
Browse files Browse the repository at this point in the history 
The batch lookup API for hash maps returns ENOSPC when a bucket doesn't
fit into the provided batch. This is unfortunate since user space has no
way to figure out what the largest bucket size is. This makes the API
very awkward to use.
It also seems that hash tables don't deal well with sequential keys,
hashing them into the same bucket with fairly high probability.

Fix the flaky test by always making the batch size equal to the map
size, which means we can never trigger ENOSPC. Also make the error
message more descriptive.

Fixes: #1390
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
  • Loading branch information
@lmb
lmb committed Jun 10, 2024
1 parent 3eb58b2 commit f705846
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 85 deletions.
24 changes: 18 additions & 6 deletions map.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ func (m *Map) LookupBytes(key interface{}) ([]byte, error) {
}

func (m *Map) lookupPerCPU(key, valueOut any, flags MapLookupFlags) error {
slice, err := ensurePerCPUSlice(valueOut, int(m.valueSize))
slice, err := ensurePerCPUSlice(valueOut)
if err != nil {
return err
}
Expand Down Expand Up @@ -683,7 +683,7 @@ func (m *Map) lookup(key interface{}, valueOut sys.Pointer, flags MapLookupFlags
}

func (m *Map) lookupAndDeletePerCPU(key, valueOut any, flags MapLookupFlags) error {
slice, err := ensurePerCPUSlice(valueOut, int(m.valueSize))
slice, err := ensurePerCPUSlice(valueOut)
if err != nil {
return err
}
Expand All @@ -695,7 +695,7 @@ func (m *Map) lookupAndDeletePerCPU(key, valueOut any, flags MapLookupFlags) err
}

// ensurePerCPUSlice allocates a slice for a per-CPU value if necessary.
func ensurePerCPUSlice(sliceOrPtr any, elemLength int) (any, error) {
func ensurePerCPUSlice(sliceOrPtr any) (any, error) {
sliceOrPtrType := reflect.TypeOf(sliceOrPtr)
if sliceOrPtrType.Kind() == reflect.Slice {
// The target is a slice, the caller is responsible for ensuring that
Expand Down Expand Up @@ -985,7 +985,11 @@ func (m *Map) guessNonExistentKey() ([]byte, error) {
// the end of all possible results, even when partial results
// are returned. It should be used to evaluate when lookup is "done".
func (m *Map) BatchLookup(cursor *MapBatchCursor, keysOut, valuesOut interface{}, opts *BatchOptions) (int, error) {
return m.batchLookup(sys.BPF_MAP_LOOKUP_BATCH, cursor, keysOut, valuesOut, opts)
n, err := m.batchLookup(sys.BPF_MAP_LOOKUP_BATCH, cursor, keysOut, valuesOut, opts)
if err != nil {
return n, fmt.Errorf("map batch lookup: %w", err)
}
return n, nil
}

// BatchLookupAndDelete looks up many elements in a map at once,
Expand All @@ -1005,7 +1009,11 @@ func (m *Map) BatchLookup(cursor *MapBatchCursor, keysOut, valuesOut interface{}
// the end of all possible results, even when partial results
// are returned. It should be used to evaluate when lookup is "done".
func (m *Map) BatchLookupAndDelete(cursor *MapBatchCursor, keysOut, valuesOut interface{}, opts *BatchOptions) (int, error) {
return m.batchLookup(sys.BPF_MAP_LOOKUP_AND_DELETE_BATCH, cursor, keysOut, valuesOut, opts)
n, err := m.batchLookup(sys.BPF_MAP_LOOKUP_AND_DELETE_BATCH, cursor, keysOut, valuesOut, opts)
if err != nil {
return n, fmt.Errorf("map batch lookup and delete: %w", err)
}
return n, nil
}

// MapBatchCursor represents a starting point for a batch operation.
Expand All @@ -1027,7 +1035,11 @@ func (m *Map) batchLookup(cmd sys.Cmd, cursor *MapBatchCursor, keysOut, valuesOu
valueBuf := sysenc.SyscallOutput(valuesOut, count*int(m.fullValueSize))

n, err := m.batchLookupCmd(cmd, cursor, count, keysOut, valueBuf.Pointer(), opts)
if err != nil {
if errors.Is(err, unix.ENOSPC) {
// Hash tables return ENOSPC when the size of the batch is smaller than
// any bucket.
return n, fmt.Errorf("%w (batch size too small?)", err)
} else if err != nil {
return n, err
}

Expand Down
148 changes: 69 additions & 79 deletions map_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,11 @@ func TestMapBatch(t *testing.T) {
t.Skipf("batch api not available: %v", err)
}

contents := map[uint32]uint32{
0: 42, 1: 4242, 2: 23, 3: 2323,
contents := []uint32{
42, 4242, 23, 2323,
}
mustNewMap := func(mapType MapType, max uint32) *Map {

mustNewMap := func(t *testing.T, mapType MapType, max uint32) *Map {
m, err := NewMap(&MapSpec{
Type: mapType,
KeySize: 4,
Expand All @@ -109,102 +110,91 @@ func TestMapBatch(t *testing.T) {
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() { m.Close() })
return m
}

var (
// Make the map large enough to avoid ENOSPC.
hashMax uint32 = uint32(len(contents)) * 10
arrayMax uint32 = 4
)

hash := mustNewMap(Hash, hashMax)
defer hash.Close()

array := mustNewMap(Array, arrayMax)
defer array.Close()
keysAndValuesForMap := func(m *Map, contents []uint32) (keys, values []uint32, stride int) {
possibleCPU := 1
if m.Type().hasPerCPUValue() {
possibleCPU = MustPossibleCPU()
}

hashPerCpu := mustNewMap(PerCPUHash, hashMax)
defer hashPerCpu.Close()
keys = make([]uint32, 0, len(contents))
values = make([]uint32, 0, len(contents)*possibleCPU)
for key, value := range contents {
keys = append(keys, uint32(key))
for i := 0; i < possibleCPU; i++ {
values = append(values, value*uint32((i+1)))
}
}

arrayPerCpu := mustNewMap(PerCPUArray, arrayMax)
defer arrayPerCpu.Close()
return keys, values, possibleCPU
}

for _, m := range []*Map{array, hash, arrayPerCpu, hashPerCpu} {
t.Run(m.Type().String(), func(t *testing.T) {
if m.Type() == PerCPUArray {
for _, typ := range []MapType{Array, PerCPUArray} {
t.Run(typ.String(), func(t *testing.T) {
if typ == PerCPUArray {
// https://lore.kernel.org/bpf/20210424214510.806627-2-pctammela@mojatatu.com/
testutils.SkipOnOldKernel(t, "5.13", "batched ops support for percpu array")
}
possibleCPU := 1
if m.Type().hasPerCPUValue() {
possibleCPU = MustPossibleCPU()
}
var keys, values []uint32
for key, value := range contents {
keys = append(keys, key)
for i := 0; i < possibleCPU; i++ {
values = append(values, value*uint32((i+1)))
}
}

m := mustNewMap(t, typ, uint32(len(contents)))
keys, values, _ := keysAndValuesForMap(m, contents)
count, err := m.BatchUpdate(keys, values, nil)
qt.Assert(t, qt.IsNil(err))
qt.Assert(t, qt.Equals(count, len(contents)))

n := len(contents) / 2 // cut buffer in half
lookupKeys := make([]uint32, n)
lookupValues := make([]uint32, n*possibleCPU)
lookupKeys := make([]uint32, len(keys))
lookupValues := make([]uint32, len(values))

var cursor MapBatchCursor
var total int
for {
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
total += count
if errors.Is(err, ErrKeyNotExist) {
break
}
qt.Assert(t, qt.IsNil(err))

qt.Assert(t, qt.IsTrue(count <= len(lookupKeys)))
for i, key := range lookupKeys[:count] {
for j := 0; j < possibleCPU; j++ {
value := lookupValues[i*possibleCPU+j]
expected := contents[key] * uint32(j+1)
qt.Assert(t, qt.Equals(value, expected), qt.Commentf("value for key %d should match", key))
}
}
}
qt.Assert(t, qt.Equals(total, len(contents)))
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.IsNil(err))
qt.Assert(t, qt.Equals(count, len(contents)))
qt.Assert(t, qt.ContentEquals(lookupKeys, keys))
qt.Assert(t, qt.ContentEquals(lookupValues, values))

if m.Type() == Array || m.Type() == PerCPUArray {
// Arrays don't support batch delete
return
}
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.ErrorIs(err, ErrKeyNotExist))
qt.Assert(t, qt.Equals(count, 0))
})
}

for _, typ := range []MapType{Hash, PerCPUHash} {
t.Run(typ.String(), func(t *testing.T) {
m := mustNewMap(t, typ, uint32(len(contents)))
keys, values, stride := keysAndValuesForMap(m, contents)
count, err := m.BatchUpdate(keys, values, nil)
qt.Assert(t, qt.IsNil(err))
qt.Assert(t, qt.Equals(count, len(contents)))

// BPF hash tables seem to have lots of collisions when keys
// are following a sequence.
// This causes ENOSPC since a single large bucket may be larger
// than the batch size. We work around this by making the batch size
// equal to the map size.
lookupKeys := make([]uint32, len(keys))
lookupValues := make([]uint32, len(values))

var cursor MapBatchCursor
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.ErrorIs(err, ErrKeyNotExist))
qt.Assert(t, qt.Equals(count, len(contents)))

qt.Assert(t, qt.ContentEquals(lookupKeys, keys))
qt.Assert(t, qt.ContentEquals(lookupValues, values))

cursor = MapBatchCursor{}
total = 0
for {
count, err = m.BatchLookupAndDelete(&cursor, lookupKeys, lookupValues, nil)
total += count
if errors.Is(err, ErrKeyNotExist) {
break
}
qt.Assert(t, qt.IsNil(err))

qt.Assert(t, qt.IsTrue(count <= len(lookupKeys)))
for i, key := range lookupKeys[:count] {
for j := 0; j < possibleCPU; j++ {
value := lookupValues[i*possibleCPU+j]
expected := contents[key] * uint32(j+1)
qt.Assert(t, qt.Equals(value, expected), qt.Commentf("value for key %d should match", key))
}
}
}
qt.Assert(t, qt.Equals(total, len(contents)))
count, err = m.BatchLookupAndDelete(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.ErrorIs(err, ErrKeyNotExist))
qt.Assert(t, qt.Equals(count, len(contents)))

qt.Assert(t, qt.ContentEquals(lookupKeys, keys))
qt.Assert(t, qt.ContentEquals(lookupValues, values))

if possibleCPU > 1 {
values := make([]uint32, possibleCPU)
if stride > 1 {
values := make([]uint32, stride)
qt.Assert(t, qt.ErrorIs(m.Lookup(uint32(0), values), ErrKeyNotExist))
} else {
var v uint32
Expand Down

0 comments on commit f705846

Please sign in to comment.