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 3f7c817
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 68 deletions.
18 changes: 15 additions & 3 deletions map.go
View file
Original file line number Diff line number Diff line change
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
106 changes: 41 additions & 65 deletions map_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,10 @@ 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,40 +109,28 @@ 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()

hashPerCpu := mustNewMap(PerCPUHash, hashMax)
defer hashPerCpu.Close()

arrayPerCpu := mustNewMap(PerCPUArray, arrayMax)
defer arrayPerCpu.Close()

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, Hash, PerCPUArray, PerCPUHash} {
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")
}

m := mustNewMap(t, typ, uint32(len(contents)))

possibleCPU := 1
if m.Type().hasPerCPUValue() {
if typ.hasPerCPUValue() {
possibleCPU = MustPossibleCPU()
}
var keys, values []uint32

keys := make([]uint32, 0, len(contents))
values := make([]uint32, 0, len(contents)*possibleCPU)
for key, value := range contents {
keys = append(keys, key)
keys = append(keys, uint32(key))
for i := 0; i < possibleCPU; i++ {
values = append(values, value*uint32((i+1)))
}
Expand All @@ -152,56 +140,44 @@ func TestMapBatch(t *testing.T) {
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)
// 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
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))
}
count, err = m.BatchLookup(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.ErrorIs(err, ErrKeyNotExist))
qt.Assert(t, qt.Equals(count, len(contents)))

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

if m.Type() == Array || m.Type() == PerCPUArray {
if typ == Array || typ == PerCPUArray {
// Arrays don't support batch delete
return
}

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))
}
count, err = m.BatchLookupAndDelete(&cursor, lookupKeys, lookupValues, nil)
qt.Assert(t, qt.ErrorIs(err, ErrKeyNotExist))
qt.Assert(t, qt.Equals(count, len(contents)))

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

if possibleCPU > 1 {
values := make([]uint32, possibleCPU)
Expand Down

0 comments on commit 3f7c817

Please sign in to comment.