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

bpf: static data: use inline asm to access static data #27589

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Aug 18, 2023

The new static data approach is fully documented in the code. Using a volatile inline asm block makes for a more predictable outcome in the bytecode, meaning we can simplify the substitution logic on the Go side and start using runtime-provided constants for user space dead code elimination.

Part of: #27320

bpf: static data: use inline asm to access static data

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 18, 2023
@ti-mo ti-mo added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 18, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 18, 2023
@ti-mo ti-mo changed the title bpf: static data: use inline asm to emit ldimm64 with direct symbol relocation bpf: static data: use inline asm to access static data Aug 22, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 22, 2023

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 23, 2023

/test

@ti-mo ti-mo added sig/loader Impacts the loading of BPF programs into the kernel. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Aug 23, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 23, 2023

/test

@ti-mo ti-mo force-pushed the tb/new-static-data branch 2 times, most recently from 287b37d to 1d5d2c1 Compare August 23, 2023 18:47
@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 23, 2023

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 25, 2023

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 25, 2023

/test

@ti-mo ti-mo marked this pull request as ready for review August 25, 2023 11:30
@ti-mo ti-mo requested review from a team as code owners August 25, 2023 11:30
@jibi jibi removed their request for review August 30, 2023 12:45
@ti-mo
Copy link
Contributor Author

ti-mo commented Aug 30, 2023

/test

@aojea
Copy link
Contributor

aojea commented Sep 21, 2023

@yasz24 can you PTAL and see the areas where you can help in follow ups?

@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 5, 2023

/test

@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 Oct 5, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 5, 2023

This will conflict with #28417, will wait for that to land first.

@ti-mo ti-mo removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 5, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 6, 2023

/test

ti-mo and others added 4 commits October 19, 2023 11:05
dst_id is often populated by LXC_ID, which is only theoretically a compile-time
constant, and generally only when it is compiled using the default ep_config.h
or in bpf unit tests.

The actual value used for LXC_ID in the bytecode varies per endpoint, so
asserting it to be a constant is not very useful.

A subsequent commit will make LXC_ID evaluate to an (inlined) function call,
so even the theoretical constant-ness assertion no longer holds.

Signed-off-by: Timo Beckers <timo@isovalent.com>
The new static data approach is fully documented in the code. Using an inline
asm block makes for a more predictable outcome in the bytecode, meaning we can
simplify the substitution logic on the Go side and start using runtime-provided
constants for user space dead code elimination.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Co-authored-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
Due to the new implementation of static data substitution in a prior commit,
static data symbols now appear with local binding. Rename isGlobalData and
only make it tolerate OBJECT-type symbols with local or global visibility.

Signed-off-by: Timo Beckers <timo@isovalent.com>
To reduce the amount of potential interference on the resulting bytecode
loaded into the kernel, remove .bss inlining from inlineGlobalData.

Since a prior commit, all global data is now explicitly placed in .rodata.config,
so symbols should never end up in .bss even if they're zero-initialized.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 19, 2023

/test

@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 Oct 19, 2023
@ti-mo ti-mo merged commit cb32b23 into cilium:main Oct 19, 2023
58 of 62 checks passed
@ti-mo ti-mo deleted the tb/new-static-data branch October 19, 2023 12:05
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/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. 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