Re: [PATCH net-next 4/8] net: psample: add tracepoint
From: Adrian Moreno
Date: Thu Apr 25 2024 - 04:06:33 EST
On 4/25/24 09:18, Ido Schimmel wrote:
On Wed, Apr 24, 2024 at 03:50:51PM +0200, Adrian Moreno wrote:
Currently there are no widely-available tools to dump the metadata and
group information when a packet is sampled, making it difficult to
troubleshoot related issues.
This makes psample use the event tracing framework to log the sampling
of a packet so that it's easier to quickly identify the source
(i.e: group) and context (i.e: metadata) of a packet being sampled.
This patch creates some checkpatch splats, but the style of the
tracepoint definition mimics that of other modules so it seems
acceptable.
I don't see a good reason to add this tracepoint (which we won't be able
to remove) when you can easily do that with bpftrace which by now should
be widely available:
#!/usr/bin/bpftrace
kfunc:psample_sample_packet
{
$ts_us = nsecs() / 1000;
$secs = $ts_us / 1000000;
$us = $ts_us % 1000000;
$group = args.group;
$skb = args.skb;
$md = args.md;
printf("%-16s %-6d %6llu.%6llu group_num = %u refcount=%u seq=%u skbaddr=%p len=%u data_len=%u sample_rate=%u in_ifindex=%d out_ifindex=%d user_cookie=%rx\n",
comm, pid, $secs, $us, $group->group_num, $group->refcount, $group->seq,
$skb, $skb->len, $skb->data_len, args.sample_rate,
$md->in_ifindex, $md->out_ifindex,
buf($md->user_cookie, $md->user_cookie_len));
}
Example output:
mausezahn 984 3299.200626 group_num = 1 refcount=1 seq=13775 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
\xde\xad\xbe\xef
mausezahn 984 3299.281424 group_num = 1 refcount=1 seq=13776 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
\xde\xad\xbe\xef
Note that it prints the cookie itself unlike the tracepoint which only
prints the hashed pointer.
I agree that bpftrace can do the work relying on kfuncs/kprobes. But I guess
that also true for many other tracepoints out there, right?
For development and labs bpftrace is perfectly fine, but using kfuncs and
requiring recompilation is harder in production systems compared with using
smaller CO-RE tools.
If OVS starts using psample heavily and users need to troubleshoot or merely
observe packets as they are sampled in a more efficient way, they are likely to
use ebpf for that. I think making it a bit easier (as in, providing a sligthly
more stable tracepoint) is worth considering.
Can you please expand on your concerns about the tracepoint? It's on the main
internal function of the module so, even though the function name or its
arguments might change, it doesn't seem probable that it'll disappear
altogether. Why else would we want to remove the tracepoint?
Signed-off-by: Adrian Moreno <amorenoz@xxxxxxxxxx>
---
net/psample/psample.c | 9 +++++++
net/psample/trace.h | 62 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 net/psample/trace.h
diff --git a/net/psample/psample.c b/net/psample/psample.c
index 476aaad7a885..92db8ffa2ba2 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -18,6 +18,12 @@
#include <net/ip_tunnels.h>
#include <net/dst_metadata.h>
+#ifndef __CHECKER__
+#define CREATE_TRACE_POINTS
+#endif
+
+#include "trace.h"
+
#define PSAMPLE_MAX_PACKET_SIZE 0xffff
static LIST_HEAD(psample_groups_list);
@@ -470,6 +476,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
void *data;
int ret;
+ if (trace_psample_sample_packet_enabled())
+ trace_psample_sample_packet(group, skb, sample_rate, md);
My understanding is that trace_x_enabled() should only be used if you
need to do some work to prepare parameters for the tracepoint.
Oh, thanks for that! I was not aware.
+
meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) +
(out_ifindex ? nla_total_size(sizeof(u16)) : 0) +
(md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) +