elf: don't clamp PerfEventArray.MaxEntries at parse time #1094
Conversation
lmb
force-pushed
the
elf-no-fixup
branch
3 times, most recently
from
4bd7fd1
to
b3cc0d1
Compare
map.go
Outdated
| var err error | ||
| maxEntries, err = fixupPerfEventArraySize(maxEntries) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Should we wrap this? Otherwise, the caller will receive the raw error from internal.PossibleCPUs without any context.
There was a problem hiding this comment.
Wrapped in fixupPerfEventArraySize instead.
| @@ -376,12 +384,9 @@ func (spec *MapSpec) createMap(inner *sys.FD, opts MapOptions) (_ *Map, err erro | |||
| spec.KeySize = 4 | |||
| spec.ValueSize = 4 | |||
|
|
|||
| if spec.MaxEntries == 0 { | |||
There was a problem hiding this comment.
This will leave a 0 in spec.MaxEntries if getting possible CPUs fails. I know it's annoying, but we shouldn't do this. (also the Go tenet of not using the other value(s) if err != nil, etc..)
In the spirit of this PR, I'm also not sure if we should mutate spec at all. Could we delay this even further and write this value into the bpf attr instead of the spec?
There was a problem hiding this comment.
I know it's annoying, but we shouldn't do this. (also the Go tenet of not using the other value(s) if err != nil, etc..)
I don't understand, sorry.
I'm also not sure if we should mutate spec at all.
That's why we do spec.Copy above.
There was a problem hiding this comment.
Can't remember what the proposed change looked like before, not sure how I missed the copy. I guess it would've been fine.
My point was: if a function returns a non-nil error, all other return values (other than the error itself, of course) should be ignored. IIRC this said something like spec.MaxEntries, err = ..., so I was just pointing out the obvious.
map.go
Outdated
| case !(ms.Type == PerfEventArray && ms.MaxEntries == 0) && | ||
| m.maxEntries != ms.MaxEntries: | ||
| return fmt.Errorf("expected max entries %v, got %v: %w", ms.MaxEntries, m.maxEntries, ErrMapIncompatible) | ||
| case maxEntries != ms.MaxEntries: |
There was a problem hiding this comment.
I think this is supposed to be m.maxEntries != maxEntries? Comparing spec to itself otherwise.
There was a problem hiding this comment.
Good catch! I added a test.
One key idea of the ELF reader is that it is deterministic: it should always produce the same output regardless of which system it runs on. The PerfEventArray clamping logic violates this since it substitutes the number of possible CPUs. Move the logic into map creation and adjust MapSpec.Compatible to do the same fixup. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
… size MapSpec.Compatible currently returns an error when comparing a PerfEventArray created from a MapSpec without explicit key or value sizes. This is because we only do the fixup just before executing the BPF_MAP_CREATE syscall. Move the fixup logic into a separate function so that we can invoke it from MapSpec.Compatible. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
The test calls t.Fatal on the wrong (parent) t since it reuses qt.New from the parent scope. Remove qt.New. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
elf: don't clamp PerfEventArray.MaxEntries at parse time
map: fix MapSpec.Compatible for PerfEventArray with zero key or value size
map: fix buggy TestMapLoadReusePinned