Use struct with pid and Go routine addr for Go BPF maps #1182

marctc · 2024-09-19T15:01:22Z

After merging #1158, we change behaviour for Go tracer to be shared among different processes.
Currently we are using the goroutine address as key to store data in BPF maps.
This PR changes the key to use the goroutine address and PID of the process in order to avoid
colissions when storing data to maps.

Fixes #1159

codecov-commenter · 2024-09-19T15:08:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.05%. Comparing base (bd8952e) to head (627cff4).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1182       +/-   ##
===========================================
+ Coverage   65.50%   81.05%   +15.54%     
===========================================
  Files         135      136        +1     
  Lines       11492    11501        +9     
===========================================
+ Hits         7528     9322     +1794     
+ Misses       3295     1646     -1649     
+ Partials      669      533      -136

Flag	Coverage Δ
integration-test	`57.04% <ø> (?)`
k8s-integration-test	`58.75% <ø> (-0.07%)`	⬇️
oats-test	`35.81% <ø> (ø)`
unittests	`53.17% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mariomac · 2024-09-24T12:58:09Z

I'm missing something. Shouldn't be each goroutine address unique in the system? I would have expected that the goroutine pointer contains a unique virtual memory address

grcevski

Looks great Marc! There are few other maps we need to fix, but please let me know if you don't want to use this PR and you want to follow-up with another instead and I'll flip this to Approve.

grcevski · 2024-09-24T12:53:32Z

bpf/go_common.h

@@ -32,60 +32,80 @@ char __license[] SEC("license") = "Dual MIT/GPL";
 // Then it is retrieved in the return uprobes and used to know the HTTP call duration as well as its
 // attributes (method, path, and status code).

+typedef struct goroutine_key {


I think it would be better to rename this data structure to something like go_addr_key. Because we'll be using it for more than goroutines. I'll show some examples below. Essentially, we need to replace all maps in the Go support where we use a Go address (or value) straight, to be keyed by the PID too.

grcevski · 2024-09-24T12:54:28Z

bpf/go_grpc.h

@@ -56,21 +56,21 @@ struct {

 struct {
    __uint(type, BPF_MAP_TYPE_LRU_HASH);
-    __type(key, void *);   // key: goroutine
-    __type(value, void *); // the transport *
+    __type(key, goroutine_key_t); // key: goroutine


We need to do the same for ongoing_grpc_transports and ongoing_streams.

grcevski · 2024-09-24T12:54:51Z

bpf/go_grpc.h

    __type(value, grpc_client_func_invocation_t);
    __uint(max_entries, MAX_CONCURRENT_REQUESTS);
 } ongoing_grpc_client_requests SEC(".maps");

 struct {
    __uint(type, BPF_MAP_TYPE_LRU_HASH);
-    __type(key, void *); // key: pointer to the request goroutine
+    __type(key, goroutine_key_t); // key: pointer to the request goroutine


ongoing_grpc_header_writes needs to be fixed up too.

Same for grpc_framer_invocation_map too.

grcevski · 2024-09-24T12:58:55Z

bpf/go_kafka_go.h

@@ -36,8 +36,8 @@ struct {

 struct {
    __uint(type, BPF_MAP_TYPE_LRU_HASH);
-    __type(key, void *);    // goroutine
-    __type(value, topic_t); // topic info
+    __type(key, goroutine_key_t); // goroutine


We need to wrap the key for produce_traceparents and ongoing_produce_messages.

grcevski · 2024-09-24T13:00:06Z

bpf/go_redis.h

-    __type(key, void *);   // key: goroutine id
-    __type(value, void *); // the *Conn
+    __type(key, goroutine_key_t); // key: goroutine id
+    __type(value, void *);        // the *Conn


We need to do the same for http2_req_map and framer_invocation_map.

grcevski · 2024-09-24T13:00:58Z

bpf/go_sarama.h

@@ -25,8 +25,8 @@ struct {

 struct {
    __uint(type, BPF_MAP_TYPE_LRU_HASH);
-    __type(key, void *); // key: goroutine id
-    __type(value, u32);  // correlation id
+    __type(key, goroutine_key_t); // key: goroutine id


We need to do the same to kafka_requests.

marctc · 2024-09-24T13:08:58Z

Looks great Marc! There are few other maps we need to fix, but please let me know if you don't want to use this PR and you want to follow-up with another instead and I'll flip this to Approve.

I see, thanks for the feedback. Yeah, I think it'd be better in a different PR

grcevski · 2024-09-24T13:13:24Z

I'm missing something. Shouldn't be each goroutine address unique in the system? I would have expected that the goroutine pointer contains a unique virtual memory address

You are right, but Go can grown and shrink the allocated memory, so theoretically, can't we have stale data from arena of the virtual address space that belonged to process A and then that same memory now is part of process B? Like one process reduces the available heap and frees that page, then another ends up taking it. Since we have a single tracer now, we can potentially access stale data, since a lot of our maps are LRU. I mean it's very unlikely, but it might happen. Same goes for process ending, and new one reclaiming the memory or part of it, e.g restart.

We must do it for the streamID keyed variables, this is a bug anyway even today.

grcevski

LGTM!

rafaelroquetto · 2024-09-24T14:04:13Z

Please don't merge this yet, it seems to be tracking/comitting binary .o files.

This reverts commit cf6843c.

This reverts commit 4ce7ab5.

marctc force-pushed the use_struct_go_maps branch from 697984a to 5237475 Compare September 23, 2024 08:54

marctc marked this pull request as ready for review September 24, 2024 12:18

marctc requested review from grcevski, mariomac and rafaelroquetto as code owners September 24, 2024 12:18

grcevski reviewed Sep 24, 2024

View reviewed changes

grcevski approved these changes Sep 24, 2024

View reviewed changes

marctc and others added 18 commits September 24, 2024 08:14

Use struct with pid and Go routine addr for Go BPF maps

a5e4d21

refactor go_nethttp.h

8061a3e

refactor go_nethttp.h - fix uprobe_writeSubset

b617a50

refactor go_nethttp.h - framer_invocation

81c9db8

refactor go_grpc.h

aed84db

refactor go_common.h

771fb4f

refactor go_common.h - fix client_trace_parent

6c7d082

refactor go_common.h - fix find_parent_goroutine

ce7988b

Revert "refactor go_common.h - fix find_parent_goroutine"

01491bf

This reverts commit cf6843c.

refactor go_common.h - fix some stuff

1c030da

refactor go_runtime.h

ca99c12

refactor go_kafka_go.h

b14e532

refactor go_redis.h

7648a7f

refactor go_sarama.h

b2753f3

refactor go_sql.h

ea9688f

fix linter

b27d67d

Revert "refactor go_redis.h"

c738aa4

This reverts commit 4ce7ab5.

Update ebpf binary files

627cff4

rafaelroquetto force-pushed the use_struct_go_maps branch from 2f76f1c to 627cff4 Compare September 24, 2024 14:21

marctc merged commit fb456f1 into main Sep 24, 2024
12 checks passed

marctc deleted the use_struct_go_maps branch September 24, 2024 16:31

marctc mentioned this pull request Sep 26, 2024

Use struct with pid and Go routine addr for Go BPF maps #1201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use struct with pid and Go routine addr for Go BPF maps #1182

Use struct with pid and Go routine addr for Go BPF maps #1182

marctc commented Sep 19, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading

mariomac commented Sep 24, 2024

grcevski left a comment

grcevski Sep 24, 2024

grcevski Sep 24, 2024

grcevski Sep 24, 2024

grcevski Sep 24, 2024

grcevski Sep 24, 2024

grcevski Sep 24, 2024

grcevski Sep 24, 2024

marctc commented Sep 24, 2024

grcevski commented Sep 24, 2024 •

edited

Loading

grcevski left a comment

rafaelroquetto commented Sep 24, 2024

Use struct with pid and Go routine addr for Go BPF maps #1182

Use struct with pid and Go routine addr for Go BPF maps #1182

Conversation

marctc commented Sep 19, 2024 • edited Loading

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

mariomac commented Sep 24, 2024

grcevski left a comment

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

grcevski Sep 24, 2024

Choose a reason for hiding this comment

marctc commented Sep 24, 2024

grcevski commented Sep 24, 2024 • edited Loading

grcevski left a comment

Choose a reason for hiding this comment

rafaelroquetto commented Sep 24, 2024

marctc commented Sep 19, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading

grcevski commented Sep 24, 2024 •

edited

Loading