address review feedback

iopsystems · Apr 30, 2024 · e5ad20f · e5ad20f
1 parent 203e21a
commit e5ad20f
Show file tree

Hide file tree

Showing 16 changed files with 98 additions and 92 deletions.
diff --git a/src/common/bpf/histogram.h b/src/common/bpf/histogram.h
@@ -208,74 +208,18 @@ static u32 clz(u64 value) {
 }
 
 // base-2 histogram indexing function that is compatible with Rust `histogram`
-// crate with grouping power = 4. This uses 2 pages (8KB) in kernel space and
-// 7.6KB in user space and has a relative error of 6.25%
+// crate.
 //
 // See the indexing logic here:
 // https://github.com/pelikan-io/rustcommon/blob/main/histogram/src/config.rs
-static u32 value_to_index4(u64 value) {
-    if (value < 32) {
+static u32 value_to_index(u8 grouping_power, u64 value) {
+    if (value < (2 << grouping_power)) {
         return value;
     } else {
         u64 power = 63 - clz(value);
-        u64 bin = power - 3;
-        u64 offset = (value - (1 << power)) >> (power - 4);
+        u64 bin = power - grouping_power + 1;
+        u64 offset = (value - (1 << power)) >> (power - grouping_power);
 
-        return (bin * 16 + offset);
-    }
-}
-
-// base-2 histogram indexing function that is compatible with Rust `histogram`
-// crate with grouping power = 5. This uses 4 pages (16KB) in kernel space and
-// 15KB in user space and has a relative error of 3.13%
-//
-// See the indexing logic here:
-// https://github.com/pelikan-io/rustcommon/blob/main/histogram/src/config.rs
-static u32 value_to_index5(u64 value) {
-    if (value < 64) {
-        return value;
-    } else {
-        u64 power = 63 - clz(value);
-        u64 bin = power - 4;
-        u64 offset = (value - (1 << power)) >> (power - 5);
-
-        return (bin * 32 + offset);
-    }
-}
-
-// base-2 histogram indexing function that is compatible with Rust `histogram`
-// crate with grouping power = 5. This uses 4 pages (16KB) in kernel space and
-// 15KB in user space and has a relative error of 3.13%
-//
-// See the indexing logic here:
-// https://github.com/pelikan-io/rustcommon/blob/main/histogram/src/config.rs
-static u32 value_to_index6(u64 value) {
-    if (value < 128) {
-        return value;
-    } else {
-        u64 power = 63 - clz(value);
-        u64 bin = power - 5;
-        u64 offset = (value - (1 << power)) >> (power - 6);
-
-        return (bin * 64 + offset);
-    }
-}
-
-
-// base-2 histogram indexing function that is compatible with Rust `histogram`
-// crate with grouping power = 7. This uses 15 pages (60KB) in kernel space and
-// 58KB in user space per histogram with a relative error of 0.781%
-//
-// See the indexing logic here:
-// https://github.com/pelikan-io/rustcommon/blob/main/histogram/src/config.rs
-static u32 value_to_index7(u64 value) {
-    if (value < 256) {
-        return value;
-    } else {
-        u64 power = 63 - clz(value);
-        u64 bin = power - 6;
-        u64 offset = (value - (1 << power)) >> (power - 7);
-
-        return (bin * 128 + offset);
+        return (bin * (1 << grouping_power) + offset);
     }
 }
diff --git a/src/samplers/block_io/linux/latency/mod.bpf.c b/src/samplers/block_io/linux/latency/mod.bpf.c
@@ -11,6 +11,7 @@
 extern int LINUX_KERNEL_VERSION __kconfig;
 
 #define COUNTER_GROUP_WIDTH 8
+#define HISTOGRAM_POWER 7
 #define MAX_CPUS 1024
 
 #define REQ_OP_BITS	8
@@ -106,7 +107,7 @@ static int handle_block_rq_complete(struct request *rq, int error, unsigned int
 		}
     }
 
-	idx = value_to_index7(nr_bytes);
+	idx = value_to_index(HISTOGRAM_POWER, nr_bytes);
 	cnt = bpf_map_lookup_elem(&size, &idx);
 
 	if (cnt) {
@@ -121,7 +122,7 @@ static int handle_block_rq_complete(struct request *rq, int error, unsigned int
 	if (*tsp <= ts) {
 		delta = ts - *tsp;
 
-		idx = value_to_index7(delta);
+		idx = value_to_index(HISTOGRAM_POWER, delta);
 		cnt = bpf_map_lookup_elem(&latency, &idx);
 
 		if (cnt) {

diff --git a/src/samplers/block_io/linux/latency/mod.rs b/src/samplers/block_io/linux/latency/mod.rs
@@ -58,9 +58,18 @@ impl Biolat {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} block_rq_insert() BPF instruction count: {}", skel.progs().block_rq_insert().insn_cnt());
-        debug!("{NAME} block_rq_issue() BPF instruction count: {}", skel.progs().block_rq_issue().insn_cnt());
-        debug!("{NAME} block_rq_complete() BPF instruction count: {}", skel.progs().block_rq_complete().insn_cnt());
+        debug!(
+            "{NAME} block_rq_insert() BPF instruction count: {}",
+            skel.progs().block_rq_insert().insn_cnt()
+        );
+        debug!(
+            "{NAME} block_rq_issue() BPF instruction count: {}",
+            skel.progs().block_rq_issue().insn_cnt()
+        );
+        debug!(
+            "{NAME} block_rq_complete() BPF instruction count: {}",
+            skel.progs().block_rq_complete().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/cpu/linux/usage/bpf.rs b/src/samplers/cpu/linux/usage/bpf.rs
@@ -56,7 +56,10 @@ impl CpuUsage {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} cpuacct_account_field() BPF instruction count: {}", skel.progs().cpuacct_account_field_kprobe().insn_cnt());
+        debug!(
+            "{NAME} cpuacct_account_field() BPF instruction count: {}",
+            skel.progs().cpuacct_account_field_kprobe().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/network/linux/traffic/bpf.rs b/src/samplers/network/linux/traffic/bpf.rs
@@ -50,8 +50,14 @@ impl NetworkTraffic {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} netif_receive_skb() BPF instruction count: {}", skel.progs().netif_receive_skb().insn_cnt());
-        debug!("{NAME} tcp_cleanup_rbuf() BPF instruction count: {}", skel.progs().tcp_cleanup_rbuf().insn_cnt());
+        debug!(
+            "{NAME} netif_receive_skb() BPF instruction count: {}",
+            skel.progs().netif_receive_skb().insn_cnt()
+        );
+        debug!(
+            "{NAME} tcp_cleanup_rbuf() BPF instruction count: {}",
+            skel.progs().tcp_cleanup_rbuf().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/scheduler/linux/runqueue/mod.bpf.c b/src/samplers/scheduler/linux/runqueue/mod.bpf.c
@@ -15,6 +15,7 @@
 #include <bpf/bpf_helpers.h>
 
 #define COUNTER_GROUP_WIDTH 8
+#define HISTOGRAM_POWER 7
 #define MAX_CPUS 1024
 #define MAX_PID 4194304
 
@@ -184,7 +185,7 @@ int handle__sched_switch(u64 *ctx)
 			delta_ns = ts - *tsp;
 
 			// update histogram
-			idx = value_to_index7(delta_ns);
+			idx = value_to_index(HISTOGRAM_POWER, delta_ns);
 			cnt = bpf_map_lookup_elem(&running, &idx);
 			if (cnt) {
 				__sync_fetch_and_add(cnt, 1);
@@ -214,7 +215,7 @@ int handle__sched_switch(u64 *ctx)
 		delta_ns = ts - *tsp;
 
 		// update the histogram
-		idx = value_to_index7(delta_ns);
+		idx = value_to_index(HISTOGRAM_POWER, delta_ns);
 		cnt = bpf_map_lookup_elem(&runqlat, &idx);
 		if (cnt) {
 			__sync_fetch_and_add(cnt, 1);
@@ -232,7 +233,7 @@ int handle__sched_switch(u64 *ctx)
 				offcpu_ns = offcpu_ns - delta_ns;
 
 				// update the histogram
-				idx = value_to_index7(offcpu_ns);
+				idx = value_to_index(HISTOGRAM_POWER, offcpu_ns);
 				cnt = bpf_map_lookup_elem(&offcpu, &idx);
 				if (cnt) {
 					__sync_fetch_and_add(cnt, 1);

diff --git a/src/samplers/scheduler/linux/runqueue/mod.rs b/src/samplers/scheduler/linux/runqueue/mod.rs
@@ -60,9 +60,18 @@ impl Runqlat {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} handle__sched_wakeup() BPF instruction count: {}", skel.progs().handle__sched_wakeup().insn_cnt());
-        debug!("{NAME} handle__sched_wakeup_new() BPF instruction count: {}", skel.progs().handle__sched_wakeup_new().insn_cnt());
-        debug!("{NAME} handle__sched_switch() BPF instruction count: {}", skel.progs().handle__sched_switch().insn_cnt());
+        debug!(
+            "{NAME} handle__sched_wakeup() BPF instruction count: {}",
+            skel.progs().handle__sched_wakeup().insn_cnt()
+        );
+        debug!(
+            "{NAME} handle__sched_wakeup_new() BPF instruction count: {}",
+            skel.progs().handle__sched_wakeup_new().insn_cnt()
+        );
+        debug!(
+            "{NAME} handle__sched_switch() BPF instruction count: {}",
+            skel.progs().handle__sched_switch().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/syscall/linux/latency/mod.bpf.c b/src/samplers/syscall/linux/latency/mod.bpf.c
@@ -18,7 +18,7 @@
 #include <bpf/bpf_core_read.h>
 
 #define COUNTER_GROUP_WIDTH 8
-#define HISTOGRAM_BUCKETS 7424
+#define HISTOGRAM_POWER 7
 #define MAX_CPUS 1024
 #define MAX_SYSCALL_ID 1024
 #define MAX_PID 4194304
@@ -188,7 +188,7 @@ int sys_exit(struct trace_event_raw_sys_exit *args)
 	*start_ts = 0;
 
 	// calculate the histogram index for this latency value
-	idx = value_to_index7(lat);
+	idx = value_to_index(HISTOGRAM_POWER, lat);
 
 	// update the total latency histogram
 	cnt = bpf_map_lookup_elem(&total_latency, &idx);

diff --git a/src/samplers/syscall/linux/latency/mod.rs b/src/samplers/syscall/linux/latency/mod.rs
@@ -59,8 +59,14 @@ impl Syscall {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} sys_enter() BPF instruction count: {}", skel.progs().sys_enter().insn_cnt());
-        debug!("{NAME} sys_exit() BPF instruction count: {}", skel.progs().sys_exit().insn_cnt());
+        debug!(
+            "{NAME} sys_enter() BPF instruction count: {}",
+            skel.progs().sys_enter().insn_cnt()
+        );
+        debug!(
+            "{NAME} sys_exit() BPF instruction count: {}",
+            skel.progs().sys_exit().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/tcp/linux/packet_latency/mod.bpf.c b/src/samplers/tcp/linux/packet_latency/mod.bpf.c
@@ -16,6 +16,8 @@
 #include <bpf/bpf_core_read.h>
 #include <bpf/bpf_tracing.h>
 
+#define HISTOGRAM_POWER 7
+
 #define MAX_ENTRIES	10240
 #define AF_INET		2
 #define NO_EXIST    1
@@ -83,7 +85,7 @@ static int handle_tcp_rcv_space_adjust(void *ctx, struct sock *sk)
 
 	delta_ns = (now - *tsp);
 
-	idx = value_to_index7(delta_ns);
+	idx = value_to_index(HISTOGRAM_POWER, delta_ns);
 	cnt = bpf_map_lookup_elem(&latency, &idx);
 
 	if (cnt) {

diff --git a/src/samplers/tcp/linux/packet_latency/mod.rs b/src/samplers/tcp/linux/packet_latency/mod.rs
@@ -57,9 +57,18 @@ impl PacketLatency {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} tcp_probe() BPF instruction count: {}", skel.progs().tcp_probe().insn_cnt());
-        debug!("{NAME} tcp_rcv_space_adjust() BPF instruction count: {}", skel.progs().tcp_rcv_space_adjust().insn_cnt());
-        debug!("{NAME} tcp_destroy_sock() BPF instruction count: {}", skel.progs().tcp_destroy_sock().insn_cnt());
+        debug!(
+            "{NAME} tcp_probe() BPF instruction count: {}",
+            skel.progs().tcp_probe().insn_cnt()
+        );
+        debug!(
+            "{NAME} tcp_rcv_space_adjust() BPF instruction count: {}",
+            skel.progs().tcp_rcv_space_adjust().insn_cnt()
+        );
+        debug!(
+            "{NAME} tcp_destroy_sock() BPF instruction count: {}",
+            skel.progs().tcp_destroy_sock().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/tcp/linux/receive/mod.bpf.c b/src/samplers/tcp/linux/receive/mod.bpf.c
@@ -16,6 +16,8 @@
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_endian.h>
 
+#define HISTOGRAM_POWER 7
+
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
 	__uint(map_flags, BPF_F_MMAPABLE);
@@ -49,7 +51,7 @@ int BPF_KPROBE(tcp_rcv_kprobe, struct sock *sk)
 	// record nanoseconds.
 	srtt_ns = 1000 * (u64) srtt_us >> 3;
 
-	idx = value_to_index7(srtt_ns);
+	idx = value_to_index(HISTOGRAM_POWER, srtt_ns);
 	cnt = bpf_map_lookup_elem(&srtt, &idx);
 
 	if (cnt) {
@@ -60,7 +62,7 @@ int BPF_KPROBE(tcp_rcv_kprobe, struct sock *sk)
 	// record nanoseconds.
 	mdev_ns = 1000 * (u64) mdev_us >> 2;
 
-	idx = value_to_index7(mdev_ns);
+	idx = value_to_index(HISTOGRAM_POWER, mdev_ns);
 	cnt = bpf_map_lookup_elem(&jitter, &idx);
 
 	if (cnt) {

diff --git a/src/samplers/tcp/linux/receive/mod.rs b/src/samplers/tcp/linux/receive/mod.rs
@@ -56,7 +56,10 @@ impl Receive {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} tcp_rcv() BPF instruction count: {}", skel.progs().tcp_rcv_kprobe().insn_cnt());
+        debug!(
+            "{NAME} tcp_rcv() BPF instruction count: {}",
+            skel.progs().tcp_rcv_kprobe().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/tcp/linux/retransmit/mod.rs b/src/samplers/tcp/linux/retransmit/mod.rs
@@ -55,7 +55,10 @@ impl Retransmit {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} tcp_retransmit_skb() BPF instruction count: {}", skel.progs().tcp_retransmit_skb().insn_cnt());
+        debug!(
+            "{NAME} tcp_retransmit_skb() BPF instruction count: {}",
+            skel.progs().tcp_retransmit_skb().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/tcp/linux/traffic/bpf.rs b/src/samplers/tcp/linux/traffic/bpf.rs
@@ -52,8 +52,14 @@ impl TcpTraffic {
             .load()
             .map_err(|e| error!("failed to load bpf program: {e}"))?;
 
-        debug!("{NAME} tcp_sendmsg() BPF instruction count: {}", skel.progs().tcp_sendmsg().insn_cnt());
-        debug!("{NAME} tcp_cleanup_rbuf() BPF instruction count: {}", skel.progs().tcp_cleanup_rbuf().insn_cnt());
+        debug!(
+            "{NAME} tcp_sendmsg() BPF instruction count: {}",
+            skel.progs().tcp_sendmsg().insn_cnt()
+        );
+        debug!(
+            "{NAME} tcp_cleanup_rbuf() BPF instruction count: {}",
+            skel.progs().tcp_cleanup_rbuf().insn_cnt()
+        );
 
         skel.attach()
             .map_err(|e| error!("failed to attach bpf program: {e}"))?;

diff --git a/src/samplers/tcp/linux/traffic/mod.bpf.c b/src/samplers/tcp/linux/traffic/mod.bpf.c
@@ -16,6 +16,8 @@
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_endian.h>
 
+#define HISTOGRAM_POWER 7
+
 /* Taken from kernel include/linux/socket.h. */
 #define AF_INET		2	/* Internet IP Protocol 	*/
 #define AF_INET6	10	/* IP version 6			*/
@@ -72,7 +74,7 @@ static int probe_ip(bool receiving, struct sock *sk, size_t size)
 			__sync_fetch_and_add(cnt, (u64) size);
 		}
 
-		idx = value_to_index7((u64) size);
+		idx = value_to_index(HISTOGRAM_POWER, (u64) size);
 		cnt = bpf_map_lookup_elem(&rx_size, &idx);
 
 		if (cnt) {
@@ -93,7 +95,7 @@ static int probe_ip(bool receiving, struct sock *sk, size_t size)
 			__sync_fetch_and_add(cnt, (u64) size);
 		}
 
-		idx = value_to_index7((u64) size);
+		idx = value_to_index(HISTOGRAM_POWER, (u64) size);
 		cnt = bpf_map_lookup_elem(&tx_size, &idx);
 
 		if (cnt) {