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

Reduce BPF memory consumption #620

Merged
merged 11 commits into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bpf/go_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "map_sizing.h"
#include "bpf_dbg.h"
#include "http_trace.h"
#include "ringbuf.h"
#include "tracing.h"
#include "trace_util.h"
#include "go_traceparent.h"
Expand Down
1 change: 1 addition & 0 deletions bpf/go_grpc.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "go_common.h"
#include "go_traceparent.h"
#include "hpack.h"
#include "ringbuf.h"

typedef struct grpc_srv_func_invocation {
u64 start_monotime_ns;
Expand Down
88 changes: 88 additions & 0 deletions bpf/go_nethttp.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Copyright The OpenTelemetry Authors
// Copyright Grafana Labs
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
Expand All @@ -21,6 +24,7 @@
#include "http_types.h"
#include "tracing.h"
#include "hpack.h"
#include "ringbuf.h"

typedef struct http_func_invocation {
u64 start_monotime_ns;
Expand Down Expand Up @@ -693,3 +697,87 @@ int uprobe_persistConnRoundTrip(struct pt_regs *ctx) {

return 0;
}

// SQL support
// This implementation was inspired by https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/ca1afccea6ec520d18238c3865024a9f5b9c17fe/internal/pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c
// and has been modified since.

typedef struct sql_func_invocation {
u64 start_monotime_ns;
u64 sql_param;
u64 query_len;
tp_info_t tp;
} sql_func_invocation_t;

struct {
__uint(type, BPF_MAP_TYPE_HASH);
__type(key, void *); // key: pointer to the request goroutine
__type(value, sql_func_invocation_t);
__uint(max_entries, MAX_CONCURRENT_REQUESTS);
} ongoing_sql_queries SEC(".maps");

SEC("uprobe/queryDC")
int uprobe_queryDC(struct pt_regs *ctx) {
bpf_dbg_printk("=== uprobe/queryDC === ");
void *goroutine_addr = GOROUTINE_PTR(ctx);
bpf_dbg_printk("goroutine_addr %lx", goroutine_addr);

void *sql_param = GO_PARAM8(ctx);
void *query_len = GO_PARAM9(ctx);

sql_func_invocation_t invocation = {
.start_monotime_ns = bpf_ktime_get_ns(),
.sql_param = (u64)sql_param,
.query_len = (u64)query_len,
.tp = {0}
};

// We don't look up in the headers, no http/grpc request, therefore 0 as last argument
client_trace_parent(goroutine_addr, &invocation.tp, 0);

// Write event
if (bpf_map_update_elem(&ongoing_sql_queries, &goroutine_addr, &invocation, BPF_ANY)) {
bpf_dbg_printk("can't update map element");
}

return 0;
}

SEC("uprobe/queryDC")
int uprobe_queryDCReturn(struct pt_regs *ctx) {

bpf_dbg_printk("=== uprobe/queryDC return === ");
void *goroutine_addr = GOROUTINE_PTR(ctx);
bpf_dbg_printk("goroutine_addr %lx", goroutine_addr);

sql_func_invocation_t *invocation = bpf_map_lookup_elem(&ongoing_sql_queries, &goroutine_addr);
if (invocation == NULL) {
bpf_dbg_printk("Request not found for this goroutine");
return 0;
}
bpf_map_delete_elem(&ongoing_sql_queries, &goroutine_addr);

sql_request_trace *trace = bpf_ringbuf_reserve(&events, sizeof(sql_request_trace), 0);
if (trace) {
task_pid(&trace->pid);
trace->type = EVENT_SQL_CLIENT;
trace->start_monotime_ns = invocation->start_monotime_ns;
trace->end_monotime_ns = bpf_ktime_get_ns();

void *resp_ptr = GO_PARAM1(ctx);
trace->status = (resp_ptr == NULL);
trace->tp = invocation->tp;

u64 query_len = invocation->query_len;
if (query_len > sizeof(trace->sql)) {
query_len = sizeof(trace->sql);
}
bpf_probe_read(trace->sql, query_len, (void*)invocation->sql_param);
bpf_dbg_printk("Found sql statement %s", trace->sql);
// submit the completed trace via ringbuffer
bpf_ringbuf_submit(trace, get_flags());
} else {
bpf_dbg_printk("can't reserve space in the ringbuffer");
}
return 0;
}
104 changes: 0 additions & 104 deletions bpf/go_sql.c

This file was deleted.

2 changes: 1 addition & 1 deletion bpf/http_sock.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include "pid.h"
#include "sockaddr.h"
#include "tcp_info.h"
#include "ringbuf.h"
#include "kringbuf.h"
#include "http_sock.h"
#include "http_ssl.h"

Expand Down
4 changes: 2 additions & 2 deletions bpf/http_sock.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include "bpf_helpers.h"
#include "bpf_builtins.h"
#include "http_types.h"
#include "ringbuf.h"
#include "kringbuf.h"
#include "pid.h"
#include "trace_common.h"

Expand All @@ -30,7 +30,7 @@ struct {
__uint(type, BPF_MAP_TYPE_LRU_HASH);
__type(key, connection_info_t);
__type(value, http_info_t);
__uint(max_entries, MAX_CONCURRENT_SHARED_REQUESTS);
__uint(max_entries, 1024);
__uint(pinning, LIBBPF_PIN_BY_NAME);
} ongoing_http_fallback SEC(".maps");

Expand Down
44 changes: 44 additions & 0 deletions bpf/kringbuf.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#ifndef KRINGBUF_H
#define KRINGBUF_H

#include "utils.h"

// These need to line up with some Go identifiers:
// EventTypeHTTP, EventTypeGRPC, EventTypeHTTPClient, EventTypeGRPCClient, EventTypeSQLClient
#define EVENT_HTTP_REQUEST 1
#define EVENT_GRPC_REQUEST 2
#define EVENT_HTTP_CLIENT 3
#define EVENT_GRPC_CLIENT 4
#define EVENT_SQL_CLIENT 5

// setting here the following map definitions without pinning them to a global namespace
// would lead that services running both HTTP and GRPC server would duplicate
// the events ringbuffer and goroutines map.
// This is an edge inefficiency that allows us avoiding the gotchas of
// pinning maps to the global namespace (e.g. like not cleaning them up when
// the autoinstrumenter ends abruptly)
// https://ants-gitlab.inf.um.es/jorgegm/xdp-tutorial/-/blob/master/basic04-pinning-maps/README.org
// we can share them later if we find is worth not including code per duplicate
struct {
__uint(type, BPF_MAP_TYPE_RINGBUF);
__uint(max_entries, 1 << 24);
} events SEC(".maps");

// To be Injected from the user space during the eBPF program load & initialization
volatile const u32 wakeup_data_bytes;

// get_flags prevents waking the userspace process up on each ringbuf message.
// If wakeup_data_bytes > 0, it will wait until wakeup_data_bytes are accumulated
// into the buffer before waking the userspace.
static __always_inline long get_flags()
{
long sz;

if (!wakeup_data_bytes)
return 0;

sz = bpf_ringbuf_query(&events, BPF_RB_AVAIL_DATA);
return sz >= wakeup_data_bytes ? BPF_RB_FORCE_WAKEUP : BPF_RB_NO_WAKEUP;
}

#endif
2 changes: 1 addition & 1 deletion bpf/ringbuf.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
// we can share them later if we find is worth not including code per duplicate
struct {
__uint(type, BPF_MAP_TYPE_RINGBUF);
__uint(max_entries, 1 << 24);
__uint(max_entries, 1 << 16);
Copy link
Contributor

Choose a reason for hiding this comment

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

As max_entries are allocated dynamically as long as more space is needed, this should not affect in terms of memory (unless we end up having more than 2^16 events in the queue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I printed BPF metrics, I see the whole ring buffer pre-allocated in locked memory.

Before:

870: ringbuf  name events  flags 0x0
	key 0B  value 0B  max_entries 16777216  memlock 16,855,384B

After:

1079: ringbuf  name events  flags 0x0
	key 0B  value 0B  max_entries 65536  memlock 78,424B

There are more issues with what we do. One example is that we'll load two separate programs when we have gin. Once for the gin functions, but we'll load the go http too, because it's there in the binary. So in a sense, we'll duplicate the memory and have 2 ring buffers when we instrument with gin.

Another issue is grpc, golang.org/x/net/http2/hpack.(*Encoder).WriteField belongs to the gosdk, so we load both http and grpc for grpc only applications, again two ring buffers, all maps are duplicate :(.

} events SEC(".maps");

// To be Injected from the user space during the eBPF program load & initialization
Expand Down
2 changes: 1 addition & 1 deletion bpf/watch_helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const watch_info_t *unused_2 __attribute__((unused));

struct {
__uint(type, BPF_MAP_TYPE_RINGBUF);
__uint(max_entries, 1 << 24);
__uint(max_entries, 1 << 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

The minimum size should be multiple of 4096, according to this: https://stackoverflow.com/questions/63415220/bpf-ring-buffer-invalid-argument-22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I will increase it, although even with 256 I see the events flowing...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok! So maybe the reference link is wrong. The documentation doesn't say anything but "must be power of 2".

} watch_events SEC(".maps");

SEC("kprobe/sys_bind")
Expand Down
2 changes: 0 additions & 2 deletions pkg/internal/discover/finder.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/grafana/beyla/pkg/beyla"
"github.com/grafana/beyla/pkg/internal/ebpf"
"github.com/grafana/beyla/pkg/internal/ebpf/goruntime"
"github.com/grafana/beyla/pkg/internal/ebpf/gosql"
"github.com/grafana/beyla/pkg/internal/ebpf/grpc"
"github.com/grafana/beyla/pkg/internal/ebpf/httpfltr"
"github.com/grafana/beyla/pkg/internal/ebpf/httpssl"
Expand Down Expand Up @@ -81,7 +80,6 @@ func newGoTracersGroup(cfg *beyla.Config, metrics imetrics.Reporter) []ebpf.Trac
&nethttp.GinTracer{Tracer: *nethttp.New(&cfg.EBPF, metrics)},
grpc.New(&cfg.EBPF, metrics),
goruntime.New(&cfg.EBPF, metrics),
gosql.New(&cfg.EBPF, metrics),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to re-add gosql?

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 removed it temporarily, the whole separate file and EBPF program, because it required more headers refactor in the BPF side, so we don't include collections we don't need twice. I'm going to follow-up with a PR to split that off again after I clean up the headers more.

}
}

Expand Down
3 changes: 0 additions & 3 deletions pkg/internal/ebpf/goruntime/bpf_bpfel_arm64.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Binary file modified pkg/internal/ebpf/goruntime/bpf_bpfel_arm64.o
Binary file not shown.
3 changes: 0 additions & 3 deletions pkg/internal/ebpf/goruntime/bpf_bpfel_x86.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Binary file modified pkg/internal/ebpf/goruntime/bpf_bpfel_x86.o
Binary file not shown.
Loading
Loading