remove unneeded bpf map update calls

Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
netobserv · Nov 26, 2024 · ab8e306 · ab8e306
1 parent 294ae3f
commit ab8e306
Show file tree

Hide file tree

Showing 16 changed files with 42 additions and 79 deletions.
diff --git a/bpf/dns_tracker.h b/bpf/dns_tracker.h
@@ -91,8 +91,9 @@ static __always_inline int track_dns_packet(struct __sk_buff *skb, pkt_info *pkt
 
         if ((flags & DNS_QR_FLAG) == 0) { /* dns query */
             fill_dns_id(pkt->id, &dns_req, dns_id, false);
-            if (bpf_map_lookup_elem(&dns_flows, &dns_req) == NULL) {
-                bpf_map_update_elem(&dns_flows, &dns_req, &ts, BPF_ANY);
+            ret = bpf_map_update_elem(&dns_flows, &dns_req, &ts, BPF_NOEXIST);
+            if (trace_messages && ret != 0) {
+                bpf_printk("error creating new dns entry %d\n", ret);
             }
         } else { /* dns response */
             fill_dns_id(pkt->id, &dns_req, dns_id, true);

diff --git a/bpf/flows.c b/bpf/flows.c
@@ -107,19 +107,6 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) {
         aggregate_flow->dns_record.flags = pkt.dns_flags;
         aggregate_flow->dns_record.latency = pkt.dns_latency;
         aggregate_flow->dns_record.errno = dns_errno;
-        long ret = bpf_map_update_elem(&aggregated_flows, &id, aggregate_flow, BPF_ANY);
-        if (ret != 0) {
-            // usually error -16 (-EBUSY) is printed here.
-            // In this case, the flow is dropped, as submitting it to the ringbuffer would cause
-            // a duplicated UNION of flows (two different flows with partial aggregation of the same packets),
-            // which can't be deduplicated.
-            // other possible values https://chromium.googlesource.com/chromiumos/docs/+/master/constants/errnos.md
-            if (trace_messages) {
-                bpf_printk("error updating flow %d\n", ret);
-            }
-            // Update global counter for hashmap update errors
-            increase_counter(HASHMAP_FLOWS_DROPPED);
-        }
     } else {
         // Key does not exist in the map, and will need to create a new entry.
         u64 rtt = 0;
@@ -140,9 +127,7 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) {
             .flow_rtt = rtt,
         };
 
-        // even if we know that the entry is new, another CPU might be concurrently inserting a flow
-        // so we need to specify BPF_ANY
-        long ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_ANY);
+        long ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_NOEXIST);
         if (ret != 0) {
             // usually error -16 (-EBUSY) or -7 (E2BIG) is printed here.
             // In this case, we send the single-packet flow via ringbuffer as in the worst case we can have
@@ -152,7 +137,6 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) {
             if (trace_messages) {
                 bpf_printk("error adding flow %d\n", ret);
             }
-
             new_flow.errno = -ret;
             flow_record *record =
                 (flow_record *)bpf_ringbuf_reserve(&direct_flows, sizeof(flow_record), 0);

diff --git a/bpf/flows_filter.h b/bpf/flows_filter.h
@@ -7,13 +7,9 @@
 
 #include "utils.h"
 
-// remove the comment below to enable debug prints
-//#define ENABLE_BPF_PRINTK
-#ifdef ENABLE_BPF_PRINTK
-#define BPF_PRINTK(fmt, args...) bpf_printk(fmt, ##args)
-#else
-#define BPF_PRINTK(fmt, args...)
-#endif
+#define BPF_PRINTK(fmt, args...)                                                                   \
+    if (trace_messages)                                                                            \
+    bpf_printk(fmt, ##args)
 
 static __always_inline int is_zero_ip(u8 *ip, u8 len) {
     for (int i = 0; i < len; i++) {

diff --git a/bpf/network_events_monitoring.h b/bpf/network_events_monitoring.h
@@ -98,10 +98,6 @@ static inline int trace_network_events(struct sk_buff *skb, struct rh_psample_me
                     __builtin_memcpy(aggregate_flow->network_events[idx], cookie, MAX_EVENT_MD);
                     aggregate_flow->network_events_idx = (idx + 1) % MAX_NETWORK_EVENTS;
                 }
-                ret = bpf_map_update_elem(&aggregated_flows, &id, aggregate_flow, BPF_ANY);
-                if (ret == 0) {
-                    return 0;
-                }
             } else {
                 return -1;
             }
@@ -128,7 +124,7 @@ static inline int trace_network_events(struct sk_buff *skb, struct rh_psample_me
     };
     bpf_probe_read(new_flow.network_events[0], md_len, user_cookie);
     new_flow.network_events_idx++;
-    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_ANY);
+    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_NOEXIST);
     if (trace_messages && ret != 0) {
         bpf_printk("error network events creating new flow %d\n", ret);
     }

diff --git a/bpf/pkt_drops.h b/bpf/pkt_drops.h
@@ -17,10 +17,6 @@ static inline long pkt_drop_lookup_and_update_flow(flow_id *id, u8 state, u16 fl
         aggregate_flow->pkt_drops.latest_state = state;
         aggregate_flow->pkt_drops.latest_flags = flags;
         aggregate_flow->pkt_drops.latest_drop_cause = reason;
-        long ret = bpf_map_update_elem(&aggregated_flows, id, aggregate_flow, BPF_EXIST);
-        if (trace_messages && ret != 0) {
-            bpf_printk("error packet drop updating flow %d\n", ret);
-        }
         return 0;
     }
     return -1;
@@ -93,7 +89,7 @@ static inline int trace_pkt_drop(void *ctx, u8 state, struct sk_buff *skb,
         .pkt_drops.latest_flags = flags,
         .pkt_drops.latest_drop_cause = reason,
     };
-    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_ANY);
+    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_NOEXIST);
     if (trace_messages && ret != 0) {
         bpf_printk("error packet drop creating new flow %d\n", ret);
     }

diff --git a/bpf/rtt_tracker.h b/bpf/rtt_tracker.h
@@ -17,10 +17,6 @@ static inline int rtt_lookup_and_update_flow(flow_id *id, u16 flags, u64 rtt) {
         if (aggregate_flow->flow_rtt < rtt) {
             aggregate_flow->flow_rtt = rtt;
         }
-        long ret = bpf_map_update_elem(&aggregated_flows, id, aggregate_flow, BPF_ANY);
-        if (trace_messages && ret != 0) {
-            bpf_printk("error rtt updating flow %d\n", ret);
-        }
         return 0;
     }
     return -1;
@@ -87,7 +83,7 @@ static inline int calculate_flow_rtt_tcp(struct sock *sk, struct sk_buff *skb) {
         .flow_rtt = rtt,
         .dscp = dscp,
     };
-    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_ANY);
+    ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_NOEXIST);
     if (trace_messages && ret != 0) {
         bpf_printk("error rtt track creating flow %d\n", ret);
     }

diff --git a/bpf/types.h b/bpf/types.h
@@ -184,7 +184,6 @@ typedef struct dns_flow_id_t {
 
 // Enum to define global counters keys and share it with userspace
 typedef enum global_counters_key_t {
-    HASHMAP_FLOWS_DROPPED,
     FILTER_REJECT,
     FILTER_ACCEPT,
     FILTER_NOMATCH,

diff --git a/pkg/ebpf/bpf_arm64_bpfel.go b/pkg/ebpf/bpf_arm64_bpfel.go
diff --git a/pkg/ebpf/bpf_arm64_bpfel.o b/pkg/ebpf/bpf_arm64_bpfel.o
diff --git a/pkg/ebpf/bpf_powerpc_bpfel.go b/pkg/ebpf/bpf_powerpc_bpfel.go
diff --git a/pkg/ebpf/bpf_powerpc_bpfel.o b/pkg/ebpf/bpf_powerpc_bpfel.o
diff --git a/pkg/ebpf/bpf_s390_bpfeb.go b/pkg/ebpf/bpf_s390_bpfeb.go
diff --git a/pkg/ebpf/bpf_s390_bpfeb.o b/pkg/ebpf/bpf_s390_bpfeb.o
diff --git a/pkg/ebpf/bpf_x86_bpfel.go b/pkg/ebpf/bpf_x86_bpfel.go
diff --git a/pkg/ebpf/bpf_x86_bpfel.o b/pkg/ebpf/bpf_x86_bpfel.o
diff --git a/pkg/tracer/tracer.go b/pkg/tracer/tracer.go
@@ -760,7 +760,6 @@ func (m *FlowFetcher) LookupAndDeleteMap(met *metrics.Metrics) map[ebpf.BpfFlowI
 func (m *FlowFetcher) ReadGlobalCounter(met *metrics.Metrics) {
 	var allCPUValue []uint32
 	globalCounters := map[ebpf.BpfGlobalCountersKeyT]prometheus.Counter{
-		ebpf.BpfGlobalCountersKeyTHASHMAP_FLOWS_DROPPED:               met.DroppedFlowsCounter.WithSourceAndReason("flow-fetcher", "CannotUpdateFlowsHashMap"),
 		ebpf.BpfGlobalCountersKeyTFILTER_REJECT:                       met.FilteredFlowsCounter.WithSourceAndReason("flow-filtering", "FilterReject"),
 		ebpf.BpfGlobalCountersKeyTFILTER_ACCEPT:                       met.FilteredFlowsCounter.WithSourceAndReason("flow-filtering", "FilterAccept"),
 		ebpf.BpfGlobalCountersKeyTFILTER_NOMATCH:                      met.FilteredFlowsCounter.WithSourceAndReason("flow-filtering", "FilterNoMatch"),