fix: optimize packet struct in DropReason plugin (#637)

Fixes #645 
`DropReason::packet` struct is copied to and from the bpf perf array for
each packet intercepted by the eBPF program for pod-level metrics,
making it a prime target for improving throughput and latency. We can
reduce its size `from 56 to 32 bytes` by:
- removing unecessary padding,
- unusued member fields, and 
- too large member types.

Removing `14 bytes` of padding and `10 bytes` of unused member fields -
new struct layout can be confirmed by looking at
`pkg/plugin/dropreason/kprobe_bpfel_x86.go`

## Performance
As measured with `k8s-netperf`, latency and throughput gain is unclear,
due to large sample variance of 5-10%.
To follow up with latency/throughput numbers in a packet drop-instensive
scenario.

## Details on member fields changes: 
- `direction` is not currently used by this plugin
- `skb_len` now matches `sk_buff::len` from `vmlinux.h` (4 vs. 8 bytes)
- `drop_type` in `metrics_map_key` is very unlikely to need >65k
different values, so can be reduced to a `uint16_t`
- `metrics_map_key` is used both as a map key, and a wrapper struct for
the `return_val` and `drop_type` fields in `struct packet` passed to the
BPF perf array - the latter is not necessary and breaking it up combined
with shrinking `drop_type` allows us to fit `proto` and `in_filtermap`
in the new empty space; (just shrinking `drop_type` doesn't reduce size
because of padding for nested structs:
https://www.reddit.com/r/Compilers/comments/1dwmyus/would_replacing_a_nested_struct_by_its_members/)

As additional reference, below is a sequence of changes I made to the
`DropReason::packet` struct:

56B- baseline
```
uint32_t src_ip;
uint32_t dst_ip;
uint16_t src_port;
uint16_t dst_port;
uint8_t proto;
uint64_t skb_len;
direction_type direction;
struct metrics_map_key key;
uint64_t ts; // timestamp in nanoseconds
bool in_filtermap;
```
48B - reorder `in_filtermap`
```
uint32_t src_ip;
uint32_t dst_ip;
uint16_t src_port;
uint16_t dst_port;
uint8_t proto;
bool in_filtermap;
uint64_t skb_len;
direction_type direction;
struct metrics_map_key key;
uint64_t ts; // timestamp in nanoseconds
```
40B - remove `direction` + its padding
```
uint32_t src_ip;
uint32_t dst_ip;
uint16_t src_port;
uint16_t dst_port;
uint8_t proto;
bool in_filtermap;
uint64_t skb_len;
struct metrics_map_key key;
uint64_t ts; // timestamp in nanoseconds
```
32B - shrink `skb_len` to match `sk_buff`, shrink `drop_type` to
uint16_t from uint32_t, split up `metrics_map_key`
```
__u32 src_ip;
__u32 dst_ip;
__u16 src_port;
__u16 dst_port;
__u32 skb_len;
__u32 return_val;
__u16 drop_type;
__u8 proto;
bool in_filtermap;
__u64 ts; // timestamp in nanoseconds
```

---------

Signed-off-by: Igor Klemenski <[email protected]>

pkg/plugin/dropreason/_cprog/drop_reason.c

@@ -23,16 +23,9 @@ char __license[] SEC("license") = "Dual MIT/GPL";
 #define PACKET_HOST 0     // Incomming packets
 #define PACKET_OUTGOING 4 // Outgoing packets
 
-typedef enum
-{
-    UNKNOWN_DIRECTION = 0,
-    INGRESS_KEY,
-    EGRESS_KEY,
-} direction_type;
-
 struct metrics_map_key
 {
-    __u32 drop_type;
+    __u16 drop_type;
     __u32 return_val;
 };
 struct metrics_map_value
@@ -47,15 +40,15 @@ struct packet
     __u32 dst_ip;
     __u16 src_port;
     __u16 dst_port;
+    __u32 skb_len;
+    __u32 return_val;
+    __u16 drop_type;
     __u8 proto;
-    __u64 skb_len;
-    direction_type direction;
-    struct metrics_map_key key;
-    __u64 ts; // timestamp in nanoseconds
     // in_filtermap defines if a given packet is of interest to us
     // and added to the filtermap. is this is set then dropreason
     // will send a perf event along with the usual aggregation in metricsmap
     bool in_filtermap;
+    __u64 ts; // timestamp in nanoseconds
 };
 struct
 {
@@ -116,9 +109,10 @@ struct
 void update_metrics_map(void *ctx, drop_reason_t drop_type, int ret_val, struct packet *p)
 {
     struct metrics_map_value *entry, new_entry = {};
-    struct metrics_map_key key = {
-        .drop_type = drop_type,
-        .return_val = ret_val};
+    struct metrics_map_key key;
+    __builtin_memset(&key, 0, sizeof(p));
+    key.drop_type = drop_type;
+    key.return_val = ret_val;
 
     entry = bpf_map_lookup_elem(&metrics_map, &key);
     if (entry)
@@ -137,10 +131,8 @@ void update_metrics_map(void *ctx, drop_reason_t drop_type, int ret_val, struct
 #if ADVANCED_METRICS == 1
     if (p->in_filtermap)
     {
-        struct metrics_map_key key2 = {
-            .drop_type = drop_type,
-            .return_val = ret_val};
-        p->key = key2;
+        p->drop_type = drop_type;
+        p->return_val = ret_val;
         bpf_perf_event_output(ctx, &dropreason_events, BPF_F_CURRENT_CPU, p, sizeof(struct packet));
     };
 #endif

pkg/plugin/dropreason/dropreason_linux.go

@@ -350,7 +350,6 @@ func (dr *dropReason) processRecord(ctx context.Context, id int) {
 			}
 			sourcePortShort := uint32(utils.HostToNetShort(bpfEvent.SrcPort))
 			destinationPortShort := uint32(utils.HostToNetShort(bpfEvent.DstPort))
-			dropKey := (dropMetricKey)(bpfEvent.Key)
 
 			fl := utils.ToFlow(
 				ktime.MonotonicOffset.Nanoseconds()+int64(bpfEvent.Ts),
@@ -370,7 +369,7 @@ func (dr *dropReason) processRecord(ctx context.Context, id int) {
 			meta := &utils.RetinaMetadata{}
 
 			// Add drop reason to the flow's metadata.
-			utils.AddDropReason(fl, meta, dropKey.DropType)
+			utils.AddDropReason(fl, meta, bpfEvent.DropType)
 
 			// Add packet size to the flow's metadata.
 			utils.AddPacketSize(meta, bpfEvent.SkbLen)
@@ -380,7 +379,7 @@ func (dr *dropReason) processRecord(ctx context.Context, id int) {
 
 			// This is only for development purposes.
 			// Removing this makes logs way too chatter-y.
-			dr.l.Debug("DropReason Packet Received", zap.Any("flow", fl), zap.Any("Raw Bpf Event", bpfEvent), zap.Uint32("drop type", dropKey.DropType))
+			dr.l.Debug("DropReason Packet Received", zap.Any("flow", fl), zap.Any("Raw Bpf Event", bpfEvent), zap.Uint16("drop type", bpfEvent.DropType))
 
 			// Write the event to the enricher.
 			ev := &hubblev1.Event{

pkg/plugin/dropreason/dropreason_linux_test.go

@@ -12,6 +12,7 @@ import (
 	"runtime"
 	"testing"
 	"time"
+	"unsafe"
 
 	"github.com/cilium/ebpf/perf"
 	kcfg "github.com/microsoft/retina/pkg/config"
@@ -207,8 +208,8 @@ func TestDropReasonRun(t *testing.T) {
 	mockedMapIterator.EXPECT().Err().Return(nil).MinTimes(1)
 	mockedMapIterator.EXPECT().Next(gomock.Any(), gomock.Any()).Return(false).MinTimes(1)
 
-	// create a rawSample slice and fill it with 56 bytes of data. 56 is the size of the rawSample in perf.Record (kprobePacket)
-	rawSample := make([]byte, 56)
+	// create a rawSample slice and fill it with `unsafe.Sizeof(kprobePacket{})`
+	rawSample := make([]byte, unsafe.Sizeof(kprobePacket{}))
 	for i := range rawSample {
 		rawSample[i] = byte(i)
 	}
@@ -261,8 +262,8 @@ func TestDropReasonReadDataPodLevelEnabled(t *testing.T) {
 	mockedPerfReader := mocks.NewMockIPerfReader(ctrl)
 	menricher := enricher.NewMockEnricherInterface(ctrl) //nolint:typecheck
 
-	// create a rawSample slice and fill it with 56 bytes of data. 56 is the size of the rawSample in perf.Record (kprobePacket)
-	rawSample := make([]byte, 56)
+	// create a rawSample slice and fill it with `unsafe.Sizeof(kprobePacket{})`
+	rawSample := make([]byte, unsafe.Sizeof(kprobePacket{}))
 	for i := range rawSample {
 		rawSample[i] = byte(i)
 	}
@@ -350,8 +351,8 @@ func TestDropReasonReadData_WithPerfArrayLostSamples(t *testing.T) {
 	mockedMap := mocks.NewMockIMap(ctrl)
 	mockedPerfReader := mocks.NewMockIPerfReader(ctrl)
 
-	// create a rawSample slice and fill it with 56 bytes of data. 56 is the size of the rawSample in perf.Record (kprobePacket)
-	rawSample := make([]byte, 56)
+	// create a rawSample slice and fill it with `unsafe.Sizeof(kprobePacket{})`
+	rawSample := make([]byte, unsafe.Sizeof(kprobePacket{}))
 	for i := range rawSample {
 		rawSample[i] = byte(i)
 	}
@@ -392,8 +393,8 @@ func TestDropReasonReadData_WithUnknownError(t *testing.T) {
 	mockedMap := mocks.NewMockIMap(ctrl)
 	mockedPerfReader := mocks.NewMockIPerfReader(ctrl)
 
-	// create a rawSample slice and fill it with 56 bytes of data. 56 is the size of the rawSample in perf.Record (kprobePacket)
-	rawSample := make([]byte, 56)
+	// create a rawSample slice and fill it with `unsafe.Sizeof(kprobePacket{})`
+	rawSample := make([]byte, unsafe.Sizeof(kprobePacket{}))
 	for i := range rawSample {
 		rawSample[i] = byte(i)
 	}

pkg/plugin/packetparser/_cprog/packetparser.c

@@ -37,7 +37,7 @@ struct packet
 	struct tcpmetadata tcp_metadata; // TCP metadata
 	direction dir; // 0 -> INGRESS, 1 -> EGRESS
 	__u64 ts; // timestamp in nanoseconds
-	__u64 bytes; // packet size in bytes
+	__u32 bytes; // packet size in bytes
 };
 
 struct

pkg/utils/flow_utils.go

@@ -256,14 +256,14 @@ func DNSRcodeToString(f *flow.Flow) string {
 }
 
 // AddPacketSize adds the packet size to the flow's metadata.
-func AddPacketSize(meta *RetinaMetadata, packetSize uint64) {
+func AddPacketSize(meta *RetinaMetadata, packetSize uint32) {
 	if meta == nil {
 		return
 	}
 	meta.Bytes = packetSize
 }
 
-func PacketSize(f *flow.Flow) uint64 {
+func PacketSize(f *flow.Flow) uint32 {
 	if f.Extensions == nil {
 		return 0
 	}
@@ -273,7 +273,7 @@ func PacketSize(f *flow.Flow) uint64 {
 }
 
 // AddDropReason adds the drop reason to the flow's metadata.
-func AddDropReason(f *flow.Flow, meta *RetinaMetadata, dropReason uint32) {
+func AddDropReason(f *flow.Flow, meta *RetinaMetadata, dropReason uint16) {
 	if f == nil || meta == nil {
 		return
 	}