optimize xDS bpf map #1029

nlgwcy · 2024-11-10T14:01:01Z

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
As discussed at the community meeting, the current xDS bpf map has the following problems:

Map-in-map update is slow. To support a large number of map records, a batch of map records need to be prepared in advance, causing memory waste and complex implementation logic.
The value length of inner-map is 1300, which is a waste in most scenarios.

Solution:

The map-in-map is no longer used to manage xDS data storage.
Define a group of bpf hash map (64/192/296/1600), select a proper bpf map for storing the xDS data based on the length, set the map flag to NO_PREMALLOC, and apply for the memory as required.
For data of the string type, the size cannot exceed MAP_VAL_STR_SIZE(192). For data of the repeat type, the size cannot exceed MAP_VAL_REPEAT_SIZE(1600). That is, for the repeat member of the xDS, the number of instances cannot exceed 200.

After the modification, the content of the xDS pointer is as follows:
ptr(outer_key):
Reserved(4 bytes) + map_type(1 byte) + inner_idx(3 bytes)

Known issues:

In extreme scenarios, the repeat member has a large number of instances (more than 200), and a bpf map cannot store all the instances. Therefore, multiple records need to be designed for storage.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Signed-off-by: wuchangye <wuchangye@huawei.com>

codecov · 2024-11-10T14:26:12Z

Codecov Report

Attention: Patch coverage is 48.27586% with 15 lines in your changes missing coverage. Please review.

Project coverage is 47.85%. Comparing base (cd1579d) to head (e57194b).
Report is 228 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/bpf/ads/loader.go	33.33%	2 Missing and 6 partials ⚠️
pkg/bpf/workload/loader.go	36.36%	2 Missing and 5 partials ⚠️

Files with missing lines	Coverage Δ
pkg/bpf/ads/sock_connection.go	`56.52% <ø> (-0.38%)`	⬇️
pkg/bpf/bpf.go	`40.93% <100.00%> (ø)`
pkg/bpf/utils/bpf_helper.go	`100.00% <100.00%> (ø)`
pkg/bpf/workload/sock_connection.go	`53.67% <ø> (-0.34%)`	⬇️
pkg/bpf/workload/sock_ops.go	`55.44% <ø> (-0.44%)`	⬇️
pkg/bpf/workload/xdp.go	`47.82% <ø> (-0.75%)`	⬇️
pkg/bpf/workload/loader.go	`24.48% <36.36%> (-5.91%)`	⬇️
pkg/bpf/ads/loader.go	`36.48% <33.33%> (-9.20%)`	⬇️

... and 30 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52d8f9e...e57194b. Read the comment docs.

nlgwcy · 2024-11-10T14:36:25Z

@hzxuzhonghu @lec-bit

hzxuzhonghu

Overall LGTM, but this is not compatable with arm ithink. i would approve after a test report though cc @lec-bit

hzxuzhonghu · 2024-11-11T01:16:57Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+        "Map64",
+        "Map192",
+        "Map296",
+        "Map1600",


nit: I am not sure why select 296 and 1600, is there byte alignments then we need tp adjust to 2^N

hzxuzhonghu · 2024-11-11T01:22:18Z

bpf/include/bpf_common.h

+    __uint(value_size, MAP_VAL_SIZE_64);
+    __uint(max_entries, MAP_MAX_ENTRIES);
+    __uint(map_flags, BPF_F_NO_PREALLOC);
+} map64 SEC(".maps");


Add a kmesh_ prefix to all the maps

LiZhenCheng9527 · 2024-11-11T01:38:47Z

bpf/include/bpf_common.h

+    __u32 outer_key = (__u32)(uintptr_t)ptr;
+
+    kmesh_parse_outer_key(outer_key, &type, &inner_idx);
+    if (type != map_type)


I think the error log is needed here.

LiZhenCheng9527 · 2024-11-11T01:40:32Z

bpf/include/bpf_common.h

+            else if (__builtin_types_compatible_p(type, void *))                                                       \
+                val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_1600, ptr);                                          \
+            else if (__builtin_types_compatible_p(type, void **))                                                      \
+                val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_1600, ptr);                                          \


Why is this the same as line 204?

LiZhenCheng9527 · 2024-11-11T01:40:53Z

bpf/include/bpf_common.h

+            if (__builtin_types_compatible_p(type, char *))                                                            \
+                val_tmp = get_ptr_val_from_map(&map192, MAP_TYPE_192, ptr);                                            \
+            else if (__builtin_types_compatible_p(type, void *))                                                       \
+                val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_1600, ptr);                                          \


Suggested change

val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_1600, ptr); \

val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_296, ptr); \

Signed-off-by: wuchangye <wuchangye@huawei.com>

hzxuzhonghu · 2024-11-12T01:30:36Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

 static inline int selected_oneof_field(void *value, const ProtobufCFieldDescriptor *field)
 {
-    uint32_t n = *(uint32_t *)((char *)value + field->quantifier_offset);
+    unsigned int n = *(unsigned int *)((char *)value + field->quantifier_offset);


why not use uint32 anymore, aren't they same?

hzxuzhonghu · 2024-11-12T01:33:29Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

 static void free_elem(void *ptr)
 {
-    if ((uintptr_t)ptr < MAX_OUTTER_MAP_ENTRIES)
+    if ((uintptr_t)ptr < UINT32_MAX)


What does this mean

hzxuzhonghu · 2024-11-12T01:36:41Z

bpf/include/bpf_common.h

+    __uint(max_entries, MAP_MAX_ENTRIES);
+    __uint(map_flags, BPF_F_NO_PREALLOC);
+} kmesh_map296 SEC(".maps");
+struct {


add a blank line before

hzxuzhonghu · 2024-11-12T01:36:52Z

bpf/include/bpf_common.h

+    __uint(max_entries, MAP_MAX_ENTRIES);
+    __uint(map_flags, BPF_F_NO_PREALLOC);
+} kmesh_map192 SEC(".maps");
+struct {


add a blank line

hzxuzhonghu · 2024-11-12T01:40:24Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

-    struct bpf_map_info info = {};
-    __u32 info_len = sizeof(info);
+    unsigned char type = MAP_GET_TYPE(*outer_key);
+    unsigned int inner_idx = MAP_GET_INDEX(*outer_key);


somewhere you use the parse function, should keep consistent

hzxuzhonghu

YOu should also update the enahanced bpf2go package

hzxuzhonghu · 2024-11-12T01:42:11Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+struct map_mng {
+    int inner_fds[MAP_TYPE_MAX];
+    struct bpf_map_info inner_infos[MAP_TYPE_MAX];
+    unsigned char mim_used_bitmap[MAP_TYPE_MAX][MIM_BITMAP_SIZE];


what does mim abbrev for

hzxuzhonghu · 2024-11-12T01:50:51Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

 }

-static int free_outter_map_entry(struct op_context *ctx, void *outter_key)
+static int bitmap_find_first_clear(unsigned char *bitmap, int len)


This is a little too slow, i am thinking if we can use optimistic algorithm, record last allocated bit postion. And at the next time, we allocate from the last position, this can largely reduce the loop and compare

Good idea~ It will be optimized.

hzxuzhonghu · 2024-11-12T01:58:05Z

pkg/bpf/workload/loader.go

 		return fmt.Errorf("failed to set api env")
 	}

-	ret := C.deserial_init(restart.GetStartType() == restart.Restart)


so this does not support restart now

Signed-off-by: wuchangye <wuchangye@huawei.com>

bitcoffeeiux · 2024-11-14T07:16:56Z

bpf/include/bpf_common.h

 }
-#endif
+
+#define KMESH_GET_PTR_VAL(ptr, type)                                                                                   \


Defined as whether a function can reduce the number of instructions？

Signed-off-by: wuchangye <wuchangye@huawei.com>

hzxuzhonghu

much vbetter

/lgtm

kmesh-bot · 2024-11-18T01:54:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kmesh-bot requested review from LiZhenCheng9527 and supercharge-xsy November 10, 2024 14:01

kmesh-bot added the size/XXL label Nov 10, 2024

nlgwcy force-pushed the optimize branch from 91fbe54 to 99fd10c Compare November 10, 2024 14:10

optimize xDS bpf map

5910904

Signed-off-by: wuchangye <wuchangye@huawei.com>

nlgwcy force-pushed the optimize branch from 99fd10c to 5910904 Compare November 10, 2024 14:16

nlgwcy mentioned this pull request Nov 10, 2024

kernel-native mode support restart #987

Merged

hzxuzhonghu reviewed Nov 11, 2024

View reviewed changes

LiZhenCheng9527 reviewed Nov 11, 2024

View reviewed changes

nlgwcy added 2 commits November 11, 2024 22:19

fix repeat field outer_key

80b2439

Signed-off-by: wuchangye <wuchangye@huawei.com>

fix issue

506101f

Signed-off-by: wuchangye <wuchangye@huawei.com>

hzxuzhonghu reviewed Nov 12, 2024

View reviewed changes

fix bpf load err

9ff142d

Signed-off-by: wuchangye <wuchangye@huawei.com>

bitcoffeeiux reviewed Nov 14, 2024

View reviewed changes

fix issue

e57194b

Signed-off-by: wuchangye <wuchangye@huawei.com>

hzxuzhonghu approved these changes Nov 18, 2024

View reviewed changes

kmesh-bot assigned hzxuzhonghu Nov 18, 2024

kmesh-bot added the lgtm label Nov 18, 2024

kmesh-bot added the approved label Nov 18, 2024

kmesh-bot merged commit 47b3d1a into kmesh-net:main Nov 18, 2024
9 checks passed

LiZhenCheng9527 mentioned this pull request Feb 24, 2025

Proposal for tcp_long_connection_metrics #1224

Merged

	val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_1600, ptr); \
	val_tmp = get_ptr_val_from_map(&map1600, MAP_TYPE_296, ptr); \

optimize xDS bpf map #1029

optimize xDS bpf map #1029

Uh oh!

Conversation

nlgwcy commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nlgwcy commented Nov 10, 2024

Uh oh!

hzxuzhonghu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

kmesh-bot commented Nov 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nlgwcy commented Nov 10, 2024 •

edited

Loading

codecov bot commented Nov 10, 2024 •

edited

Loading

hzxuzhonghu left a comment •

edited

Loading