Convert existing gadgets to use buffer.h API #2323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eiffel-fl
requested review from
mauriciovasquezbernal and
alban
as code owners
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
d1bd17b
to
2428e52
Compare
Merged
There was a problem hiding this comment.
Did you try to build the dns gadget after this change? It fails with a lot of previous definition is here to me:
$ $ sudo -E ig image build .
INFO[0000] Experimental features enabled
Builder container logs start:
clang -target bpf -Wall -g -O2 -D __TARGET_ARCH_x86 \
-c /work/program.bpf.c -I /usr/include/gadget/amd64/ -o /out/amd64.bpf.o
clang -target bpf -Wall -g -O2 -D __TARGET_ARCH_arm64 \
-c /work/program.bpf.c -I /usr/include/gadget/arm64/ -o /out/arm64.bpf.o
tinygo build -o /out/program.wasm -target=wasi --no-debug /work/program.go
go: downloading github.com/wapc/wapc-guest-tinygo v0.3.3
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:51:26: error: typedef redefinition with different types ('__kernel_ulong_t' (aka 'unsigned long') vs 'unsigned int')
typedef __kernel_ulong_t __kernel_size_t;
^
/usr/include/asm-generic/posix_types.h:68:22: note: previous definition is here
typedef unsigned int __kernel_size_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:53:25: error: typedef redefinition with different types ('__kernel_long_t' (aka 'long') vs 'int')
typedef __kernel_long_t __kernel_ssize_t;
^
/usr/include/asm-generic/posix_types.h:69:14: note: previous definition is here
typedef int __kernel_ssize_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:73:24: error: typedef redefinition with different types ('__kernel_dev_t' (aka 'unsigned int') vs '__dev_t' (aka 'unsigned long long'))
typedef __kernel_dev_t dev_t;
^
/usr/include/sys/types.h:59:17: note: previous definition is here
typedef __dev_t dev_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:91:25: error: typedef redefinition with different types ('__kernel_size_t' (aka 'unsigned int') vs 'unsigned long')
typedef __kernel_size_t size_t;
^
/usr/lib/llvm-15/lib/clang/15.0.6/include/stddef.h:46:23: note: previous definition is here
typedef __SIZE_TYPE__ size_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:101:13: error: typedef redefinition with different types ('u64' (aka 'unsigned long long') vs '__blkcnt_t' (aka 'long'))
typedef u64 blkcnt_t;
^
/usr/include/sys/types.h:192:20: note: previous definition is here
typedef __blkcnt_t blkcnt_t; /* Type to count number of disk blocks. */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:714:3: error: typedef redefinition with different types ('struct sigset_t' vs '__sigset_t')
} sigset_t;
^
/usr/include/bits/types/sigset_t.h:7:20: note: previous definition is here
typedef __sigset_t sigset_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:55:26: error: typedef redefinition with different types ('__kernel_ulong_t' (aka 'unsigned long') vs 'unsigned int')
typedef __kernel_ulong_t __kernel_size_t;
^
/usr/include/asm-generic/posix_types.h:68:22: note: previous definition is here
typedef unsigned int __kernel_size_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:57:25: error: typedef redefinition with different types ('__kernel_long_t' (aka 'long') vs 'int')
typedef __kernel_long_t __kernel_ssize_t;
^
/usr/include/asm-generic/posix_types.h:69:14: note: previous definition is here
typedef int __kernel_ssize_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:73:24: error: typedef redefinition with different types ('__kernel_dev_t' (aka 'unsigned int') vs '__dev_t' (aka 'unsigned long long'))
typedef __kernel_dev_t dev_t;
^
/usr/include/sys/types.h:59:17: note: previous definition is here
typedef __dev_t dev_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:89:25: error: typedef redefinition with different types ('__kernel_size_t' (aka 'unsigned int') vs 'unsigned long')
typedef __kernel_size_t size_t;
^
/usr/lib/llvm-15/lib/clang/15.0.6/include/stddef.h:46:23: note: previous definition is here
typedef __SIZE_TYPE__ size_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:99:13: error: typedef redefinition with different types ('u64' (aka 'unsigned long long') vs '__blkcnt_t' (aka 'long'))
typedef u64 blkcnt_t;
^
/usr/include/sys/types.h:192:20: note: previous definition is here
typedef __blkcnt_t blkcnt_t; /* Type to count number of disk blocks. */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:646:3: error: typedef redefinition with different types ('struct sigset_t' vs '__sigset_t')
} sigset_t;
^
/usr/include/bits/types/sigset_t.h:7:20: note: previous definition is here
typedef __sigset_t sigset_t;
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:4458:8: error: redefinition of 'iovec'
struct iovec {
^
/usr/include/bits/types/struct_iovec.h:26:8: note: previous definition is here
struct iovec
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8778:2: error: redefinition of enumerator 'BPF_REG_0'
BPF_REG_0 = 0,
^
/usr/include/linux/bpf.h:54:2: note: previous definition is here
BPF_REG_0 = 0,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8779:2: error: redefinition of enumerator 'BPF_REG_1'
BPF_REG_1 = 1,
^
/usr/include/linux/bpf.h:55:2: note: previous definition is here
BPF_REG_1,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8780:2: error: redefinition of enumerator 'BPF_REG_2'
BPF_REG_2 = 2,
^
/usr/include/linux/bpf.h:56:2: note: previous definition is here
BPF_REG_2,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8781:2: error: redefinition of enumerator 'BPF_REG_3'
BPF_REG_3 = 3,
^
/usr/include/linux/bpf.h:57:2: note: previous definition is here
BPF_REG_3,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8782:2: error: redefinition of enumerator 'BPF_REG_4'
BPF_REG_4 = 4,
^
/usr/include/linux/bpf.h:58:2: note: previous definition is here
BPF_REG_4,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8783:2: error: redefinition of enumerator 'BPF_REG_5'
BPF_REG_5 = 5,
^
/usr/include/linux/bpf.h:59:2: note: previous definition is here
BPF_REG_5,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8784:2: error: redefinition of enumerator 'BPF_REG_6'
BPF_REG_6 = 6,
^
/usr/include/linux/bpf.h:60:2: note: previous definition is here
BPF_REG_6,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8785:2: error: redefinition of enumerator 'BPF_REG_7'
BPF_REG_7 = 7,
^
/usr/include/linux/bpf.h:61:2: note: previous definition is here
BPF_REG_7,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8786:2: error: redefinition of enumerator 'BPF_REG_8'
BPF_REG_8 = 8,
^
/usr/include/linux/bpf.h:62:2: note: previous definition is here
BPF_REG_8,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8787:2: error: redefinition of enumerator 'BPF_REG_9'
BPF_REG_9 = 9,
^
/usr/include/linux/bpf.h:63:2: note: previous definition is here
BPF_REG_9,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8788:2: error: redefinition of enumerator 'BPF_REG_10'
BPF_REG_10 = 10,
^
/usr/include/linux/bpf.h:64:2: note: previous definition is here
BPF_REG_10,
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/arm64/vmlinux.h:8789:2: error: redefinition of enumerator '__MAX_BPF_REG'
__MAX_BPF_REG = 11,
^
/usr/include/linux/bpf.h:65:2: note: previous definition is here
__MAX_BPF_REG,
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:8557:8: error: redefinition of 'iovec'
struct iovec {
^
/usr/include/bits/types/struct_iovec.h:26:8: note: previous definition is here
struct iovec
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:10811:8: error: redefinition of 'sockaddr'
struct sockaddr {
^
/usr/include/bits/socket.h:183:8: note: previous definition is here
struct sockaddr
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:10816:8: error: redefinition of 'msghdr'
struct msghdr {
^
/usr/include/bits/socket.h:262:8: note: previous definition is here
struct msghdr
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11242:2: error: redefinition of enumerator 'IPPROTO_IP'
IPPROTO_IP = 0,
^
/usr/include/linux/in.h:31:21: note: expanded from macro 'IPPROTO_IP'
#define IPPROTO_IP IPPROTO_IP
^
/usr/include/linux/in.h:30:3: note: previous definition is here
IPPROTO_IP = 0, /* Dummy protocol for TCP */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11243:2: error: redefinition of enumerator 'IPPROTO_ICMP'
IPPROTO_ICMP = 1,
^
/usr/include/linux/in.h:33:23: note: expanded from macro 'IPPROTO_ICMP'
#define IPPROTO_ICMP IPPROTO_ICMP
^
/usr/include/linux/in.h:32:3: note: previous definition is here
IPPROTO_ICMP = 1, /* Internet Control Message Protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11244:2: error: redefinition of enumerator 'IPPROTO_IGMP'
IPPROTO_IGMP = 2,
^
/usr/include/linux/in.h:35:23: note: expanded from macro 'IPPROTO_IGMP'
#define IPPROTO_IGMP IPPROTO_IGMP
^
/usr/include/linux/in.h:34:3: note: previous definition is here
IPPROTO_IGMP = 2, /* Internet Group Management Protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11245:2: error: redefinition of enumerator 'IPPROTO_IPIP'
IPPROTO_IPIP = 4,
^
/usr/include/linux/in.h:37:23: note: expanded from macro 'IPPROTO_IPIP'
#define IPPROTO_IPIP IPPROTO_IPIP
^
/usr/include/linux/in.h:36:3: note: previous definition is here
IPPROTO_IPIP = 4, /* IPIP tunnels (older KA9Q tunnels use 94) */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11246:2: error: redefinition of enumerator 'IPPROTO_TCP'
IPPROTO_TCP = 6,
^
/usr/include/linux/in.h:39:22: note: expanded from macro 'IPPROTO_TCP'
#define IPPROTO_TCP IPPROTO_TCP
^
/usr/include/linux/in.h:38:3: note: previous definition is here
IPPROTO_TCP = 6, /* Transmission Control Protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11247:2: error: redefinition of enumerator 'IPPROTO_EGP'
IPPROTO_EGP = 8,
^
/usr/include/linux/in.h:41:22: note: expanded from macro 'IPPROTO_EGP'
#define IPPROTO_EGP IPPROTO_EGP
^
/usr/include/linux/in.h:40:3: note: previous definition is here
IPPROTO_EGP = 8, /* Exterior Gateway Protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11248:2: error: redefinition of enumerator 'IPPROTO_PUP'
IPPROTO_PUP = 12,
^
/usr/include/linux/in.h:43:22: note: expanded from macro 'IPPROTO_PUP'
#define IPPROTO_PUP IPPROTO_PUP
^
/usr/include/linux/in.h:42:3: note: previous definition is here
IPPROTO_PUP = 12, /* PUP protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11249:2: error: redefinition of enumerator 'IPPROTO_UDP'
IPPROTO_UDP = 17,
^
/usr/include/linux/in.h:45:22: note: expanded from macro 'IPPROTO_UDP'
#define IPPROTO_UDP IPPROTO_UDP
^
/usr/include/linux/in.h:44:3: note: previous definition is here
IPPROTO_UDP = 17, /* User Datagram Protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11250:2: error: redefinition of enumerator 'IPPROTO_IDP'
IPPROTO_IDP = 22,
^
/usr/include/linux/in.h:47:22: note: expanded from macro 'IPPROTO_IDP'
#define IPPROTO_IDP IPPROTO_IDP
^
/usr/include/linux/in.h:46:3: note: previous definition is here
IPPROTO_IDP = 22, /* XNS IDP protocol */
^
In file included from /work/program.bpf.c:14:
In file included from /usr/include/gadget/buffer.h:7:
/usr/include/gadget/amd64/vmlinux.h:11251:2: error: redefinition of enumerator 'IPPROTO_TP'
IPPROTO_TP = 29,
^
/usr/include/linux/in.h:49:21: note: expanded from macro 'IPPROTO_TP'
#define IPPROTO_TP IPPROTO_TP
^
/usr/include/linux/in.h:48:3: note: previous definition is here
IPPROTO_TP = 29, /* SO Transport Protocol Class 4 */
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
make: *** [/Makefile:19: /out/amd64.bpf.o] Error 1
make: *** Waiting for unfinished jobs....
20 errors generated.
make: *** [/Makefile:19: /out/arm64.bpf.o] Error 1
Builder container logs end
Error: builder container exited with status 2I think the main issue is buffer.h including vmlinux.h
Hum... |
|
For the moment, I will remove it. diff --git a/gadgets/trace_dns/program.bpf.c b/gadgets/trace_dns/program.bpf.c
index 2ac175d6..055f9a3c 100644
--- a/gadgets/trace_dns/program.bpf.c
+++ b/gadgets/trace_dns/program.bpf.c
@@ -1,12 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2021 The Inspektor Gadget authors */
-#include <linux/bpf.h>
-#include <linux/if_ether.h>
-#include <linux/ip.h>
-#include <linux/in.h>
-#include <linux/udp.h>
-#include <sys/socket.h>
+#include <vmlinux.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>
@@ -19,6 +14,7 @@
#define GADGET_TYPE_NETWORKING
#include <gadget/sockets-map.h>
+
// Max DNS name length: 255
// https://datatracker.ietf.org/doc/html/rfc1034#section-3.1
#define MAX_DNS_NAME 255
@@ -223,7 +219,6 @@ static __always_inline int output_dns_event(struct __sk_buff *skb,
union dnsflags flags,
__u32 name_len, __u16 ancount)
{
- __u32 zero = 0;
struct event_t *event;
event = gadget_reserve_buf(&events, sizeof(*event));
@@ -349,7 +344,7 @@ static __always_inline int output_dns_event(struct __sk_buff *skb,
// size of full structure - addresses + only used addresses
unsigned long long size =
sizeof(*event); // - MAX_ADDR_ANSWERS * 16 + anaddrcount * 16;
- gadget_submit_buf(ctx, &events, event, size);
+ gadget_submit_buf(NULL, &events, event, size);
return 0;
}
diff --git a/include/gadget/buffer.h b/include/gadget/buffer.h
index 8c17ba88..b517e7fc 100644
--- a/include/gadget/buffer.h
+++ b/include/gadget/buffer.h
@@ -4,8 +4,8 @@
#ifndef __BUFFER_BPF_H
#define __BUFFER_BPF_H
-#include <vmlinux.h>
-#include <bpf/bpf_helpers.h>
+// #include <vmlinux.h>
+// #include <bpf/bpf_helpers.h>
#ifndef MAX_EVENT_SIZE
#define MAX_EVENT_SIZE 10240
diff --git a/include/gadget/sockets-map.h b/include/gadget/sockets-map.h
index 9a10007b..71df3f2d 100644
--- a/include/gadget/sockets-map.h
+++ b/include/gadget/sockets-map.h
@@ -4,13 +4,13 @@
#define SOCKETS_MAP_H
#ifdef GADGET_TYPE_NETWORKING
-
-#include <linux/if_ether.h>
-#include <linux/ip.h>
-#include <linux/ipv6.h>
-#include <linux/tcp.h>
-#include <linux/udp.h>
-
+//
+// #include <linux/if_ether.h>
+// #include <linux/ip.h>
+// #include <linux/ipv6.h>
+// #include <linux/tcp.h>
+// #include <linux/udp.h>
+//
#endif
#ifndef PACKET_HOST
@@ -29,6 +29,10 @@
#define ETH_P_IP 0x0800 /* Internet Protocol packet */
#endif
+#ifndef ETH_P_IPV6
+#define ETH_P_IPV6 0x42 /* TODO */
+#endif
+
#ifndef AF_INET
#define AF_INET 2 /* Internet IP Protocol */
#endifBut I am still getting some errors: As |
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
2428e52
to
63649b0
Compare
There was a problem hiding this comment.
trace exec is broken, it's not possible to use submit without reserving before. Hence in this case a new wrapper to use bpf_ringbuf_output() is needed.
diff --git gadgets/trace_exec/program.bpf.c gadgets/trace_exec/program.bpf.c
index 6d02631d..aa7be378 100644
--- gadgets/trace_exec/program.bpf.c
+++ gadgets/trace_exec/program.bpf.c
@@ -168,7 +168,7 @@ int ig_execve_x(struct trace_event_raw_sys_exit *ctx)
bpf_get_current_comm(&event->comm, sizeof(event->comm));
diff --git gadgets/trace_exec/program.bpf.c gadgets/trace_exec/program.bpf.c
index 6d02631d..aa7be378 100644
--- gadgets/trace_exec/program.bpf.c
+++ gadgets/trace_exec/program.bpf.c
@@ -168,7 +168,7 @@ int ig_execve_x(struct trace_event_raw_sys_exit *ctx)
bpf_get_current_comm(&event->comm, sizeof(event->comm));
size_t len = EVENT_SIZE(event);
if (len <= sizeof(*event))
- gadget_submit_buf(ctx, &events, event, len);
+ gadget_output_buf(ctx, &events, event, len);
cleanup:
bpf_map_delete_elem(&execs, &pid);
return 0;
diff --git include/gadget/buffer.h include/gadget/buffer.h
index 8c17ba88..cf90fd43 100644
--- include/gadget/buffer.h
+++ include/gadget/buffer.h
@@ -47,4 +47,14 @@ static __always_inline long gadget_submit_buf(void *ctx, void *map, void *buf, _
return bpf_perf_event_output(ctx, map, BPF_F_CURRENT_CPU, buf, size);
}
+static __always_inline long gadget_output_buf(void *ctx, void *map, void *buf, __u64 size)
+{
+ if (bpf_core_type_exists(struct bpf_ringbuf)) {
+ bpf_ringbuf_output(map, buf, size, 0);
+ return 0;
+ }
+
+ return bpf_perf_event_output(ctx, map, BPF_F_CURRENT_CPU, buf, size);
+}
+
#endif /* __BUFFER_BPF_H */
What about going the other way around? i.e. avoid using vmlinux in buffer.h? IIUC the only purpose if to have a definition of $ git diff include/
diff --git include/gadget/buffer.h include/gadget/buffer.h
index 8c17ba88..9a883af8 100644
--- include/gadget/buffer.h
+++ include/gadget/buffer.h
@@ -4,7 +4,6 @@
#ifndef __BUFFER_BPF_H
#define __BUFFER_BPF_H
-#include <vmlinux.h>
#include <bpf/bpf_helpers.h>
#ifndef MAX_EVENT_SIZE
@@ -26,11 +25,16 @@ struct {
__uint(value_size, MAX_EVENT_SIZE);
} gadget_heap SEC(".maps");
+// empty definition of ringbuf. We don't include vmlinux.h to avoid causing collisions with
+// networking gadgets that don't include this file. Also, ___ is used to avoid a name clash when
+// vmlinux.h is used by the gadget.
+struct bpf_ringbuf___compat {};
+
static __always_inline void *gadget_reserve_buf(void *map, __u64 size)
{
static const int zero = 0;
- if (bpf_core_type_exists(struct bpf_ringbuf))
+ if (bpf_core_type_exists(struct bpf_ringbuf___compat))
return bpf_ringbuf_reserve(map, size, 0);
return bpf_map_lookup_elem(&gadget_heap, &zero);
@@ -39,7 +43,7 @@ static __always_inline void *gadget_reserve_buf(void *map, __u64 size)
static __always_inline long gadget_submit_buf(void *ctx, void *map, void *buf, __u64 size)
{
- if (bpf_core_type_exists(struct bpf_ringbuf)) {
+ if (bpf_core_type_exists(struct bpf_ringbuf___compat)) {
bpf_ringbuf_submit(buf, 0);
return 0;
}
@@ -47,4 +51,14 @@ static __always_inline long gadget_submit_buf(void *ctx, void *map, void *buf, _
return bpf_perf_event_output(ctx, map, BPF_F_CURRENT_CPU, buf, size);
}It seems to be working for a kernel with ringbuf available, I haven't tried in a kernel without this support. |
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
63649b0
to
a9adb7f
Compare
Oops! Indeed! I just added your helper to the commit regarding
If we define it ourselves, we will be in troubles on kernel without ringbuf support as the branch will be taken but the ringbuf related functions would not exist on this specific kernel? Maybe one way would be to check that the helper exists with |
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
a9adb7f
to
9f4477b
Compare
|
OK, I am able to compile everything, but I am getting runtime error with |
bpf_core_type_exists() checks for the existence of a type in the kernel, hence we can define it without any issues.
|
include/gadget/buffer.h
Outdated
| @@ -18,6 +18,7 @@ | |||
| } name SEC(".maps"); \ | |||
| const void *gadget_map_tracer_##name __attribute__((unused)); | |||
|
|
|||
| #ifndef GADGET_NO_RESERVE | |||
There was a problem hiding this comment.
I'm wondering about this API. It seems it's complicated for users having another knob to set.
I'm wondering if we can control all of it by using MAX_EVENT_SIZE, we can define it to be 1 by default (very small overhead) and then gadget developers can change it based on their specific event:
#define MAX_EVENT_SIZE (sizeof(struct event))
#include <gadget/buffer.h>There was a problem hiding this comment.
I would rather stick with the NO_RESERVE.
This is indeed not easy, but I do not think it will be used by plenty of people, so far we only need it for one gadget and I can deal without but at the expense of extra memory, and should be an "expert" flag.
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
9f4477b
to
8ea8338
Compare
|
I am splitting this PR as it becomes far too big and begins to address several things at once, so:
|
eiffel-fl
force-pushed
the
francis/extend-buffer-api
branch
2 times, most recently
from
9a84766
to
8f67af9
Compare
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
8ea8338
to
c38ae36
Compare
Merged
| @@ -9,6 +9,8 @@ | |||
| #include <bpf/bpf_tracing.h> | |||
| #include <bpf/bpf_endian.h> | |||
|
|
|||
| #define MAX_EVENT_SIZE 512 | |||
There was a problem hiding this comment.
What about using the actual size of the struct as suggested in #2323 (comment)?
There was a problem hiding this comment.
Please, see: #2331 (comment)
There was a problem hiding this comment.
If we aren't setting this to the real value, I think it's better not to define it at all.
There was a problem hiding this comment.
Without define, it is set to be 10240 which I find quite large.
512 is far less than the default value, while enabling to store an event without problem.
I will stick with it until done in golang.
There was a problem hiding this comment.
I still think it's a bad idea to define this to a value that is not the exact one, that's very misleading. However, I won't block the PR for this.
There was a problem hiding this comment.
What about a comment // Note: this is not the exact size of the event due to ...?
eiffel-fl
force-pushed
the
francis/extend-buffer-api
branch
3 times, most recently
from
331a648
to
3855c80
Compare
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
2 times, most recently
from
14fd863
to
40bafdd
Compare
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
eiffel-fl
force-pushed
the
francis/run-existing-buffer
branch
from
40bafdd
to
6275f4c
Compare
alban
reviewed
Jan 8, 2024
| __uint(key_size, sizeof(u32)); | ||
| __uint(value_size, sizeof(u32)); | ||
| } events SEC(".maps"); | ||
| GADGET_TRACER_MAP(events, 1024 * 256); |
There was a problem hiding this comment.
gadget_submit_buf uses CORE function bpf_core_enum_value_exists to detect at run-time if it needs to use a perf ring buffer or a bpf ring buffer.
However, GADGET_TRACER_MAP unconditionally defines a BPF_MAP_TYPE_RINGBUF. How does the fallback to perf ring buffer work then?
Maybe that should have been a question for #2331.
There was a problem hiding this comment.
How does the fallback to perf ring buffer work then?
It is done in golang in:
ba2bfca#diff-ba7859f5f9e30f2ff15de408ba39bab4978697da537d7bc1c965e896620906f5R224
Like it is done in C in the inspiration for this API:
iovisor/bcc@527e2e6#diff-80dae29367274bdfa0b46233e80a506ac7f0f562501990513576bc9a03ca6f5bR43
|
Does this change cause the gadgets to require a newer kernel version than the ones described in requirements.md? |
No. |
Thanks. I dug more how it works. In case it's interesting:
So kernel does not have to support anything new since it is handled in userspace. It is possible to do relocations in kernel since Linux 5.17 during
|
mauriciovasquezbernal
approved these changes
Jan 9, 2024
| @@ -9,6 +9,8 @@ | |||
| #include <bpf/bpf_tracing.h> | |||
| #include <bpf/bpf_endian.h> | |||
|
|
|||
| #define MAX_EVENT_SIZE 512 | |||
There was a problem hiding this comment.
I still think it's a bad idea to define this to a value that is not the exact one, that's very misleading. However, I won't block the PR for this.
|
Thank you for the reviews! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi.
This PR converts existing gadgets to use buffer.h API.
Best regards.