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

[Part-1] native module changes for per-instance load #4175

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

saxena-anurag
Copy link
Contributor

@saxena-anurag saxena-anurag commented Feb 3, 2025

Fixes part of #2667

Description

This PR contains part-1 of the changes needed to support multiple-load of native modules.

  1. This PR mainly contains the required bpf2c changes (and corresponding ebpfcore changes), but functionally, there is still no change.
  2. The PR changes the native code format so that the native BPF program also gets a pointer to a "runtime context" that contains the addresses of the maps and helper functions. This enables multiple load of BPF programs, where each instance of the BPF program has its own runtime context.

Corresponding user mode changes, and the multiple load support will be added in a follow-up PR.

P.S.: Since 0.21 will have breaking changes for native module NMR interface, and this change it also disruptive, frontloading this part of the change, so that this can go in 0.21 release.

Testing

No functional change. Existing CICD should cover any regressions.

Documentation

No

Installation

No.

@saxena-anurag saxena-anurag changed the title [DRAFT] native module changes for per-instance load [DRAFT] [Part-1] native module changes for per-instance load Feb 4, 2025
@saxena-anurag saxena-anurag changed the title [DRAFT] [Part-1] native module changes for per-instance load [Part-1] native module changes for per-instance load Feb 4, 2025
@saxena-anurag saxena-anurag marked this pull request as ready for review February 4, 2025 16:47
@saxena-anurag
Copy link
Contributor Author

Still working on making bpf2c_conformance tests green, but the PR can be reviewed.

@saxena-anurag
Copy link
Contributor Author

Still working on making bpf2c_conformance tests green, but the PR can be reviewed.

The tests are all fixed now.

@@ -1233,7 +1233,8 @@ bpf_code_generator::bpf_code_generator_program::encode_instructions(
throw bpf_code_generator_exception(
"Map " + output.relocation + " doesn't exist", output.instruction_offset);
}
source = std::format("_maps[{}].address", std::to_string(map_definition->second.index));
source =
std::format("runtime_context->map_data[{}].address", std::to_string(map_definition->second.index));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since generated code is changing, please reflect that in the NativeCodeGeneration.md.


static void
_ebpf_native_clean_up_programs(
_In_reads_(count_of_programs) ebpf_native_program_t** programs, size_t count_of_programs, bool close_handles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking - are we guaranteed that programs is always non-null?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like it. If it isn't, the SAL annotation here should cause the compiler to complain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and all callers as passing non-null.

} helper_function_entry_t;

typedef struct _helper_function_data
{
Copy link
Member

Choose a reason for hiding this comment

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

Why are helper addresses per instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, right now it does not make much difference. But to keep the code future proof (say, for scenarios when we want to support helper function per attach type), it is better to keep the per-instance data in the runtime context.

libs/execution_context/ebpf_program.h Outdated Show resolved Hide resolved
libs/execution_context/ebpf_native.c Outdated Show resolved Hide resolved

static void
_ebpf_native_clean_up_programs(
_In_reads_(count_of_programs) ebpf_native_program_t** programs, size_t count_of_programs, bool close_handles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like it. If it isn't, the SAL annotation here should cause the compiler to complain.

libs/execution_context/ebpf_native.c Show resolved Hide resolved
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.

5 participants