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

Add static initializer support to bpf2c - part 1 #2728

Merged
merged 12 commits into from
Aug 9, 2023

Conversation

Alan-Jowett
Copy link
Member

@Alan-Jowett Alan-Jowett commented Aug 4, 2023

Description

Add support for static map initialization via BTF map definitions.

This is part 1, which only changes bpf2c to emit the map definitions in the compiled PE image and to the native loader to consume it.

Part 2 will update the ELF loader to consume this data and set the initial values in the maps.

Testing

CI/CD

Documentation

No.

Installation

No.

Map's data is stored in the ".maps" section of the ELF file. The symbols for the ".maps" section gives the starting and ending offset of each map. The relocations for the ".maps" section give the offset of the initial values for each map. Each relocation record is a pair of (offset, symbol) where the symbol is the map value to insert. To convert offset to index in the "values" field, the first step is to determine which map the offset is for. This is done by finding the map whose range contains the offset. Then the offset is converted to an index by subtracting the offset of the values array and dividing by the size of a pointer. Finally, the value is inserted into the map's initial values vector at the computed index.

As an example, assume a map declared as:

{
    __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
    __uint(max_entries, 3);
    __uint(key_size, sizeof(uint32_t));
    __array(values, int(struct xdp_md* ctx));
} map SEC(".maps") = {
    // First and last entries are NULL to test that we can handle cases where
    // the initial values are not at the beginning of the array.
    .values =
        {
            NULL,
            recurse,
            NULL,
        },
};

This produces the following symbol records:

D:\ebpf-for-windows\x64\Debug>llvm-objdump --syms tail_call_recursive.o --section=.maps | findstr .maps
llvm-objdump.exe: warning: section '.maps' mentioned in a -j/--section option, but not found in any input file
0000000000000030 g     O .maps  0000000000000020 canary
0000000000000000 g     O .maps  0000000000000030 map

So, the maps are as follows:
(0x0, 0x30) -> map
(0x30, 0x50) -> canary.

The relocation data for the .maps section then contains:


tail_call_recursive.o:  file format elf64-bpf

RELOCATION RECORDS FOR [.maps]:
OFFSET           TYPE                     VALUE
0000000000000020 R_BPF_64_64              recurse

So, the relocation is at offset 0x20, which is within the map called "map".
The offset of the "values" field is 0x18 (from the BTF data, not shown here).

Index is then:
(0x20 - 0x18) / sizeof(uintptr) -> 1

So, the map value at index 1 should be "recurse".

There are no relocation records for NULL entries.

Partially resolves: #2555

Alan Jowett added 6 commits August 4, 2023 13:25
Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #2728 (9fe2555) into main (535bfe2) will increase coverage by 0.02%.
Report is 11 commits behind head on main.
The diff coverage is 86.78%.

❗ Current head 9fe2555 differs from pull request most recent head 86ca22d. Consider uploading reports for the commit 86ca22d to get more accurate results

@@            Coverage Diff             @@
##             main    #2728      +/-   ##
==========================================
+ Coverage   87.19%   87.21%   +0.02%     
==========================================
  Files         129      129              
  Lines       25184    25312     +128     
==========================================
+ Hits        21958    22077     +119     
- Misses       3226     3235       +9     
Files Changed Coverage Δ
include/bpf2c.h 100.00% <ø> (ø)
tests/sample/map_in_map_btf.c 100.00% <ø> (ø)
tests/sample/tail_call_sequential.c 96.96% <ø> (ø)
tools/bpf2c/bpf_code_generator.h 93.54% <ø> (ø)
libs/execution_context/ebpf_native.c 88.00% <77.02%> (-0.98%) ⬇️
libs/execution_context/ebpf_core.c 90.79% <90.00%> (-0.01%) ⬇️
tools/bpf2c/bpf_code_generator.cpp 95.87% <94.04%> (-0.21%) ⬇️
tests/sample/tail_call_recursive.c 100.00% <100.00%> (ø)
tests/unit/libbpf_test.cpp 99.94% <100.00%> (-0.01%) ⬇️

... and 16 files with indirect coverage changes

Signed-off-by: Alan Jowett <[email protected]>
dthaler
dthaler previously approved these changes Aug 8, 2023
tools/bpf2c/bpf_code_generator.cpp Outdated Show resolved Hide resolved
dthaler
dthaler previously approved these changes Aug 8, 2023
Copy link
Collaborator

@dthaler dthaler left a comment

Choose a reason for hiding this comment

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

Ok other than a grammar nit in a comment

tools/bpf2c/bpf_code_generator.cpp Outdated Show resolved Hide resolved
dthaler
dthaler previously approved these changes Aug 8, 2023
Copy link
Collaborator

@shpalani shpalani left a comment

Choose a reason for hiding this comment

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

  1. For my understanding, what is the 'recurse' type in the .values array?

Is it a function pointer? Or if it is an enum, what are the various values it can take? In short, trying to understand how the value is consumed.

  1. Is the .values an optional array?

dv-msft
dv-msft previously approved these changes Aug 9, 2023
@Alan-Jowett
Copy link
Member Author

  1. For my understanding, what is the 'recurse' type in the .values array?

Is it a function pointer? Or if it is an enum, what are the various values it can take? In short, trying to understand how the value is consumed.

  1. Is the .values an optional array?

The "values" field is defined by BPF convention as a BTF map field. It is how the types of values stored in a BPF map declared via BTF. Optionally it can also be used to declare the initial values for the BPF map that will be set when the map is created.

If you take a look at the "tests/sample/tail_call_sequential.c" sample, it should be clearer that the .values field contains the list of programs to be inserted into the BPF map. Likewise, the "tests/sample/map_in_map_btf.c" there is an example of initializing a map in map with an initial value.

libs/execution_context/ebpf_native.c Show resolved Hide resolved
sequential7, sequential8, sequential9, sequential10, sequential11, sequential12, sequential13,
sequential14, sequential15, sequential16, sequential17, sequential18, sequential19, sequential20,
sequential21, sequential22, sequential23, sequential24, sequential25, sequential26, sequential27,
sequential28, sequential29, sequential30, sequential31, sequential32,
Copy link
Contributor

Choose a reason for hiding this comment

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

The test case exercising this sample program (TEST_CASE("sequential_tail_call", "[libbpf]")) is overwriting the map entries manually. I think we should:

  1. Update the test case to not do so, else the new code path will not be tested.
  2. Instead of updating this sample, have a new sample program so that one can test normal map creation, and other can test creation + static initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test was updated to not initialize the map and instead rely on the statically initialized values.

Copy link
Collaborator

@shpalani shpalani Aug 9, 2023

Choose a reason for hiding this comment

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

From the above, with this new field,
__array(values, int(struct xdp_md* ctx));

  1. Do the user program need to do bpf_map_update_elem for each tail call program?
    Eg, for (...) {
    bpf_map_update_elem(map_fd, &key, &program_fd, 0);
    }

  2. If yes, What happens if the map elements are not done in that order (index), matching the .values array?

libs/execution_context/ebpf_native.c Show resolved Hide resolved
Signed-off-by: Alan Jowett <[email protected]>
@Alan-Jowett Alan-Jowett added this pull request to the merge queue Aug 9, 2023
Merged via the queue into microsoft:main with commit 3f3b834 Aug 9, 2023
66 checks passed
@Alan-Jowett Alan-Jowett deleted the static_initializer branch August 9, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for static map initialization
5 participants