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

Allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map with integer as key_size #2952

Conversation

shpalani
Copy link
Collaborator

@shpalani shpalani commented Oct 5, 2023

Description

  1. In ebpf_native.c : Check map-in-map is Hash of Maps with integer as key_size. If true, allow initialization.
  2. Added test cases in libbpf_tests.cpp.

Testing

_Do any existing tests cover this change? No
Are new tests needed? Yes

Manual test:

PS C:\Temp> netsh ebpf add program .\hash_of_map_in_array_map.sys
Loaded with ID 196609

PS C:\Temp> dir .\hash_of_map_in_array_map.sys


    Directory: C:\Temp


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----        10/5/2023   9:09 AM          11664 hash_of_map_in_array_map.sys


PS C:\Temp> netsh ebpf show programs

    ID  Pins  Links  Mode       Type           Name
======  ====  =====  =========  =============  ====================
196609     1      1  NATIVE     xdp            lookup

PS C:\Temp> netsh ebpf show maps

                              Key  Value      Max  Inner
     ID            Map Type  Size   Size  Entries     ID  Pins  Name
=======  ==================  ====  =====  =======  =====  ====  ========
 131073        hash_of_maps     2      4    65536  65537     0  outer_map

PS C:\Temp>

Documentation

_Is there any documentation impact for this change? No

Installation

_Is there any installer impact for this change? No

dthaler
dthaler previously approved these changes Oct 5, 2023
ebpf_map_definition_in_file_t map_def = native_map_to_update->entry->definition;
uint32_t key_size = map_def.key_size;
bool is_hash_of_maps_with_int_key = (map_def.type == BPF_MAP_TYPE_HASH_OF_MAPS) &&
(key_size == sizeof(uint8_t) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if key size is uint8_t and max_entries exceeds 256 etc.? Does this code handle that? @Alan-Jowett can you confirm if Linux mandates key_size to be strictly 4-bytes in this case?

Copy link
Collaborator Author

@shpalani shpalani Oct 6, 2023

Choose a reason for hiding this comment

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

Can you please clarify the concern on the max_entries (number of entries in the hash table) and key_size (size of the key) relation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shankarseal : Could you please help clarify the discussed suggestion for the value_size?

For static initialization of hash-of-maps,

  1. Add key_size == sizeof(int) or sizeof(long) (so, it is only 4 bytes allowed).
  2. value_size == ???

Copy link
Collaborator Author

@shpalani shpalani Oct 12, 2023

Choose a reason for hiding this comment

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

With the suggested change of key_size == sizeof(int), adding value into the inner map array is failing. Hence the suggestion does not work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the key size is x, there can be at most 2^8x entries in the map which must be checked.
Also, it is not enough to only check the length of the key; but the type should be an integer. Not sure if/how that can be verified here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If bpf2c has that info (from parsing btf data), I think we need to change bpf2c to add that information in native driver, and then this code can check it.

Copy link
Collaborator

@shankarseal shankarseal Oct 16, 2023

Choose a reason for hiding this comment

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

Static initialization of hashmap of maps is not possible unless its a BTF map.

Make change in bpf2c to fail converstion if static initialized map in maps dont use integer as key type. Similar changes need to be made for the JIT-ed case as well.

@shpalani shpalani marked this pull request as draft November 29, 2023 23:17
@Alan-Jowett
Copy link
Member

@shpalani What is the status of this PR? Has it been abandoned? If so, please close. If not, please address any comments and mark as ready for review.

@shpalani
Copy link
Collaborator Author

Closing this PR, as it is committed through
#3211

@shpalani shpalani closed this Feb 15, 2024
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.

Allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map with integer as key_size
5 participants