NETOBSERV-1995: Use global map, spinlock, split maps #469
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,22 +51,25 @@ | |
| */ | ||
| #include "network_events_monitoring.h" | ||
|
|
||
| static inline void update_existing_flow(flow_metrics *aggregate_flow, pkt_info *pkt, u64 len) { | ||
| bpf_spin_lock(&aggregate_flow->lock); | ||
| aggregate_flow->packets += 1; | ||
| aggregate_flow->bytes += len; | ||
| aggregate_flow->end_mono_time_ts = pkt->current_ts; | ||
| aggregate_flow->flags |= pkt->flags; | ||
| aggregate_flow->dscp = pkt->dscp; | ||
| bpf_spin_unlock(&aggregate_flow->lock); | ||
|
There was a problem hiding this comment. we no longer needs netobserv-ebpf-agent/bpf/flows.c Line 59 in 119061c as that was specific to perCPU map |
||
| } | ||
|
|
||
| static inline void update_dns(additional_metrics *extra_metrics, pkt_info *pkt, int dns_errno) { | ||
| if (pkt->dns_id != 0) { | ||
|
There was a problem hiding this comment. u can check dns_err here and if its There was a problem hiding this comment. there can be an error and still have DNS data: when the dns request wasn't found, we can't generate latency, but we still provide the other data, and return ENOENT. |
||
| extra_metrics->dns_record.id = pkt->dns_id; | ||
| extra_metrics->dns_record.flags = pkt->dns_flags; | ||
| extra_metrics->dns_record.latency = pkt->dns_latency; | ||
| } | ||
| if (dns_errno != 0) { | ||
| extra_metrics->dns_record.errno = dns_errno; | ||
| } | ||
| } | ||
|
|
||
| static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | ||
|
|
@@ -76,6 +79,9 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| return TC_ACT_OK; | ||
| } | ||
| do_sampling = 1; | ||
|
|
||
| u16 eth_protocol = 0; | ||
|
|
||
| pkt_info pkt; | ||
| __builtin_memset(&pkt, 0, sizeof(pkt)); | ||
|
|
||
|
|
@@ -90,7 +96,7 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| struct ethhdr *eth = (struct ethhdr *)data; | ||
| u64 len = skb->len; | ||
|
|
||
| if (fill_ethhdr(eth, data_end, &pkt, ð_protocol) == DISCARD) { | ||
| return TC_ACT_OK; | ||
| } | ||
|
|
||
|
|
@@ -99,7 +105,7 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| id.direction = direction; | ||
|
|
||
| // check if this packet need to be filtered if filtering feature is enabled | ||
| bool skip = check_and_do_flow_filtering(&id, pkt.flags, 0, eth_protocol); | ||
| if (skip) { | ||
| return TC_ACT_OK; | ||
| } | ||
|
|
@@ -108,30 +114,22 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| if (enable_dns_tracking) { | ||
| dns_errno = track_dns_packet(skb, &pkt); | ||
| } | ||
| flow_metrics *aggregate_flow = (flow_metrics *)bpf_map_lookup_elem(&aggregated_flows, &id); | ||
| if (aggregate_flow != NULL) { | ||
| update_existing_flow(aggregate_flow, &pkt, len); | ||
| } else { | ||
| // Key does not exist in the map, and will need to create a new entry. | ||
|
There was a problem hiding this comment. @msherif1234 I don't understand the rationale for this code. We're setting default RTT for TCP flows, how could that be accurate? I don't see how this wouldn't produce fake data. There was a problem hiding this comment. this the min value of ythe hook isn't fire IIRC it was a define in the kernel and I believe we doc that as the default min RTT, when I recall more I will update here |
||
| flow_metrics new_flow = { | ||
| .packets = 1, | ||
| .bytes = len, | ||
| .eth_protocol = eth_protocol, | ||
| .start_mono_time_ts = pkt.current_ts, | ||
| .end_mono_time_ts = pkt.current_ts, | ||
| .flags = pkt.flags, | ||
| .dscp = pkt.dscp, | ||
| }; | ||
| __builtin_memcpy(new_flow.dst_mac, eth->h_dest, ETH_ALEN); | ||
| __builtin_memcpy(new_flow.src_mac, eth->h_source, ETH_ALEN); | ||
|
|
||
| long ret = bpf_map_update_elem(&aggregated_flows, &id, &new_flow, BPF_NOEXIST); | ||
|
There was a problem hiding this comment. we will probably need to spin lock/unlock here as well There was a problem hiding this comment. the lock is on the value object, not the key, and it's a new object in that case, so I don't think it makes sense to lock There was a problem hiding this comment. if u have concurrent new flows on different cpus one should create and the other will update ? There was a problem hiding this comment. That's what the There was a problem hiding this comment. with eexist lookup and update we need to take the lock as well what about There was a problem hiding this comment.
|
||
| if (ret != 0) { | ||
|
|
@@ -142,7 +140,7 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| flow_metrics *aggregate_flow = | ||
| (flow_metrics *)bpf_map_lookup_elem(&aggregated_flows, &id); | ||
| if (aggregate_flow != NULL) { | ||
| update_existing_flow(aggregate_flow, &pkt, len); | ||
| } else { | ||
| if (trace_messages) { | ||
| bpf_printk("failed to update an exising flow\n"); | ||
|
|
@@ -171,6 +169,48 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) { | |
| } | ||
| } | ||
| } | ||
|
|
||
| // Update additional metrics (per-CPU map) | ||
| if (pkt.dns_id != 0 || dns_errno != 0) { | ||
| // hack on id will be removed with dedup-in-kernel work | ||
| id.direction = 0; | ||
| id.if_index = 0; | ||
|
There was a problem hiding this comment. will the feature work with this hack ? There was a problem hiding this comment. sure, it's precisely here to make it work :-) |
||
| additional_metrics *extra_metrics = | ||
| (additional_metrics *)bpf_map_lookup_elem(&additional_flow_metrics, &id); | ||
| if (extra_metrics != NULL) { | ||
| update_dns(extra_metrics, &pkt, dns_errno); | ||
| } else { | ||
| additional_metrics new_metrics = { | ||
| .dns_record.id = pkt.dns_id, | ||
| .dns_record.flags = pkt.dns_flags, | ||
| .dns_record.latency = pkt.dns_latency, | ||
| .dns_record.errno = dns_errno, | ||
| }; | ||
| long ret = | ||
| bpf_map_update_elem(&additional_flow_metrics, &id, &new_metrics, BPF_NOEXIST); | ||
| if (ret != 0) { | ||
| if (trace_messages && ret != -EEXIST) { | ||
| bpf_printk("error adding DNS %d\n", ret); | ||
| } | ||
| if (ret == -EEXIST) { | ||
| // Concurrent write from another CPU; retry | ||
| additional_metrics *extra_metrics = | ||
| (additional_metrics *)bpf_map_lookup_elem(&additional_flow_metrics, &id); | ||
| if (extra_metrics != NULL) { | ||
| update_dns(extra_metrics, &pkt, dns_errno); | ||
| } else { | ||
| if (trace_messages) { | ||
| bpf_printk("failed to update DNS\n"); | ||
| } | ||
| increase_counter(HASHMAP_FAIL_UPDATE_DNS); | ||
| } | ||
| } else { | ||
| increase_counter(HASHMAP_FAIL_UPDATE_DNS); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return TC_ACT_OK; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this rule as part of the lint target we can make it part of the fmt but in that case we need to remove it from the lint so we won't run it twice when build ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's for two different things: the format rule allows to auto-format as part of the local build process on your machine, whereas the lint rule is still useful on the CI to make sure we didn't commit & push unformatted code.