Skip to content

Conversation

learnitall
Copy link
Contributor

This commit modifies the MapSpec.fixupMagicFields method to support the following for the CPU map:

  • Validation of KeySize and ValueSize
  • Setting KeySize, ValueSize and MaxEntries when set to zero
  • Clamping MaxEntries to the number of possible CPUs

This enables to write more portable code when utilizing the CPU map, since kernel-specific and node-specific arguments no longer have to be supplied at compile-time.

@learnitall learnitall force-pushed the pr/learnitall/magic-cpumap branch 7 times, most recently from e53f445 to 1ffca47 Compare December 13, 2024 19:02
@learnitall learnitall marked this pull request as ready for review December 13, 2024 19:12
@learnitall learnitall requested a review from a team as a code owner December 13, 2024 19:13
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the PossibleCPU() changes, please.

The key/val change doesn't make much sense IMO.

  • Changing the key or value size without the knowledge of the bpf program will result in the program potentially reading past the end of the value on kernels where the value is 4 bytes, resulting in a verifier error. In order to remain portable, an application (the Cilium agent, in this case) needs to target the lowest common denominator of any given feature. When support for older kernels is dropped, the 8-byte value can be used (should be fine for Cilium since we target 5.4+), unless the program is compiled specifically for the running kernel (which we're trying to get rid of in Cilium).

  • Kernel versions are meaningless in bpf land; vendors (mostly RH) backport tons of features to older kernels, while disabling others for security reasons. Reacting to the availability of certain features must be done using probes, e.g. in this case by trying to create a map of a particular size. Tests are the only exception, since we use only mainline kernels and need some way of defining when a test needs to pass and when it can be skipped.

Also, for our purposes, a simple map creation test will do with a check on Map.MaxEntries(). Don't overthink it. 😉

@learnitall learnitall force-pushed the pr/learnitall/magic-cpumap branch 2 times, most recently from 408e89a to 033b2d8 Compare December 17, 2024 19:56
@learnitall learnitall requested a review from ti-mo December 17, 2024 20:02
@learnitall
Copy link
Contributor Author

Done deal @ti-mo, thanks for the explanation!

@ti-mo ti-mo force-pushed the pr/learnitall/magic-cpumap branch from 033b2d8 to 37d4da2 Compare December 18, 2024 11:25
@ti-mo ti-mo changed the title map: Handle magic fields for CPU map map: automatically set CPUMap MaxEntries based on possible CPUs Dec 18, 2024
This commit automatically populates a CPUMap's MaxEntries with the amount of
possible CPUs on the machine. This keeps code portable across different hosts.

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
@ti-mo ti-mo force-pushed the pr/learnitall/magic-cpumap branch from 37d4da2 to a3d4835 Compare December 18, 2024 11:29
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ti-mo ti-mo merged commit 97cfce5 into cilium:main Dec 18, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants