Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loader: remove CompileAndLoad #31792

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

lmb
Copy link
Contributor

@lmb lmb commented Apr 5, 2024

loader: remove unused CompileAndLoad mode

Endpoint calls into the loader in different "modes", depending on how much
the configuration has changed.

1. ReloadDatapath: re-attach programs without compiling them.

2. CompileOrLoad: re-attach a program, possibly retrieving it
  a cache.

3. CompileAndLoad: re-attach a program without retrieving it
  from cache.

CompileAndLoad does completely bypass the cache, but at the same time
doesn't invalidate it. The following sequence of calls would therefore reuse
an old (cached) program:

    CompileOrLoad()
   CompileAndLoad()
   CompileOrLoad() // back to the cached program from above

According to Joe Stringer, CompileAndLoad mode was meant as an escape hatch
to debug caching bugs. This never really materialised so we can simplify the
loader by removing the code path outright.

Tests for CompileAndLoad are simply changed to call CompileOrLoad.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

loader: use require helpers for errors

Use require.Error and require.NoError instead of the Nil variants. This
leads to more readable error messages.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

@lmb lmb added sig/loader Impacts the loading of BPF programs into the kernel. release-note/misc This PR makes changes that have no direct user impact. labels Apr 5, 2024
@lmb
Copy link
Contributor Author

lmb commented Apr 5, 2024

/test

@lmb lmb force-pushed the pr/lmb/remove-compile-and-load branch 2 times, most recently from 0b175a3 to dca9971 Compare April 5, 2024 11:00
@lmb
Copy link
Contributor Author

lmb commented Apr 5, 2024

/test

lmb added 2 commits April 17, 2024 14:37
Endpoint calls into the loader in different "modes", depending on
how much the configuration has changed.

1. ReloadDatapath: re-attach programs without compiling them.

2. CompileOrLoad: re-attach a program, possibly retrieving it
   a cache.

3. CompileAndLoad: re-attach a program without retrieving it
   from cache.

CompileAndLoad does completely bypass the cache, but at the same
time doesn't invalidate it. The following sequence of calls would
therefore reuse an old (cached) program:

    CompileOrLoad()
    CompileAndLoad()
    CompileOrLoad() // back to the cached program from above

According to Joe Stringer, CompileAndLoad mode was meant as an
escape hatch to debug caching bugs. This never really materialised
so we can simplify the loader by removing the code path outright.

Tests for CompileAndLoad are simply changed to call CompileOrLoad.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Use require.Error and require.NoError instead of the Nil variants.
This leads to more readable error messages.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb force-pushed the pr/lmb/remove-compile-and-load branch from dca9971 to 9e581ec Compare April 17, 2024 12:40
@lmb
Copy link
Contributor Author

lmb commented Apr 17, 2024

/test

@lmb lmb marked this pull request as ready for review April 17, 2024 13:07
@lmb lmb requested review from a team as code owners April 17, 2024 13:07
@lmb lmb requested review from aditighag and rgo3 April 17, 2024 13:07
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 22, 2024
@aditighag aditighag added this pull request to the merge queue Apr 22, 2024
Merged via the queue into cilium:main with commit 5ef9bcc Apr 22, 2024
61 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants