From 8b3dfa608dd2d704611799f779b75c0be51f5c0b Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 5 Aug 2024 17:41:56 -0400 Subject: [PATCH 1/8] Add options to modify eBPF map sizes --- src/collector.rs | 2 +- src/main.rs | 42 ++++++++++++++++++++-- src/profiler.rs | 90 +++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 118 insertions(+), 16 deletions(-) diff --git a/src/collector.rs b/src/collector.rs index 910df50..b93e4ab 100644 --- a/src/collector.rs +++ b/src/collector.rs @@ -127,7 +127,7 @@ impl AggregatorCollector { } } -/// Aggregagates the samples in memory, which might be acceptable when profiling for short amounts of time. +/// Aggregates the samples in memory, which might be acceptable when profiling for short amounts of time. impl Collector for AggregatorCollector { fn collect( &mut self, diff --git a/src/main.rs b/src/main.rs index 49cd36b..a4ff407 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,7 +21,7 @@ use tracing_subscriber::FmtSubscriber; use lightswitch::object::ObjectFile; use lightswitch::profile::symbolize_profile; use lightswitch::profile::{fold_profile, to_proto}; -use lightswitch::profiler::Profiler; +use lightswitch::profiler::{Profiler, ProfilerConfig}; use lightswitch::unwind_info::in_memory_unwind_info; use lightswitch::unwind_info::remove_redundant; use lightswitch::unwind_info::remove_unnecesary_markers; @@ -152,6 +152,36 @@ struct Cli { /// Where to write the profile. #[arg(long, default_value_t, value_enum)] sender: ProfileSender, + // Print out info on eBPF map sizes + #[arg(long, help = "Print eBPF map sizes that can be configured")] + mapsize_info: bool, + // eBPF map stacks + #[arg(long, default_value_t = 100000)] + mapsize_stacks: u32, + // eBPF map aggregated_stacks + #[arg( + long, + default_value_t = 10000, + help = "Derived from constant MAX_AGGREGATED_STACKS_ENTRIES" + )] + mapsize_aggregated_stacks: u32, + // eBPF map unwind_info_chunks + #[arg(long, default_value_t = 5000)] + mapsize_unwind_info_chunks: u32, + // eBPF map unwind_tables + #[arg( + long, + default_value_t = 65, + help = "Derived from constant MAX_UNWIND_INFO_SHARDS" + )] + mapsize_unwind_tables: u32, + // eBPF map rate_limits + #[arg( + long, + default_value_t = 5000, + help = "Derived from constant MAX_PROCESSES" + )] + mapsize_rate_limits: u32, } /// Exit the main thread if any thread panics. We prefer this behaviour because pretty much every @@ -227,12 +257,18 @@ fn main() -> Result<(), Box> { } })); - let mut p: Profiler<'_> = Profiler::new( - args.libbpf_logs, + let profiler_config = ProfilerConfig( args.bpf_logging, args.duration, args.sample_freq, + args.mapsize_info, + args.mapsize_stacks, + args.mapsize_aggregated_stacks, + args.mapsize_unwind_info_chunks, + args.mapsize_unwind_tables, + args.mapsize_rate_limits, ); + let mut p: Profiler<'_> = Profiler::new(profiler_config); p.profile_pids(args.pids); p.run(collector.clone()); diff --git a/src/profiler.rs b/src/profiler.rs index f18c274..30236a5 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -285,34 +285,100 @@ impl fmt::Display for SymbolizedAggregatedSample { pub type RawAggregatedProfile = Vec; pub type SymbolizedAggregatedProfile = Vec; -impl Default for Profiler<'_> { - fn default() -> Self { - Self::new(false, false, Duration::MAX, 19) - } +pub struct ProfilerConfig { + pub libbpf_debug: bool, + pub bpf_logging: bool, + pub duration: Duration, + pub sample_freq: u16, + pub mapsize_info: bool, + pub mapsize_stacks: u32, + pub mapsize_aggregated_stacks: u32, + pub mapsize_unwind_info_chunks: u32, + pub mapsize_unwind_tables: u32, + pub mapsize_rate_limits: u32, } +// impl Default for Profiler<'_> { +// fn default() -> Self { +// Self::new(false, false, Duration::MAX, 19) +// } +// } + impl Profiler<'_> { pub fn new( - libbpf_debug: bool, - bpf_logging: bool, - duration: Duration, - sample_freq: u16, + profiler_config: ProfilerConfig, + // libbpf_debug: bool, + // bpf_logging: bool, + // duration: Duration, + // sample_freq: u16, + // mapsize_info: bool, + // mapsize_stacks: u32, + // mapsize_aggregated_stacks: u32, + // mapsize_unwind_info_chunks: u32, + // mapsize_unwind_tables: u32, + // mapsize_rate_limits: u32, ) -> Self { + let duration = profiler_config.duration; + let sample_freq = profiler_config.sample_freq; let mut skel_builder: ProfilerSkelBuilder = ProfilerSkelBuilder::default(); - skel_builder.obj_builder.debug(libbpf_debug); + skel_builder.obj_builder.debug(profiler_config.libbpf_debug); let mut open_skel = skel_builder.open().expect("open skel"); + if profiler_config.mapsize_info { + info!("eBPF map size Configuration:"); + info!("stacks: {}", profiler_config.mapsize_stacks); + info!( + "aggregated_stacks: {}", + profiler_config.mapsize_aggregated_stacks + ); + info!( + "unwind_info_chunks: {}", + profiler_config.mapsize_unwind_info_chunks + ); + info!( + "unwind_tables: {}", + profiler_config.mapsize_unwind_tables + ); + info!( + "rate_limits: {}", + profiler_config.mapsize_rate_limits + ); + } + // mapsize modifications can only be made before the maps are actually loaded + // Initialize map sizes with defaults or modifications + open_skel + .maps() + .stacks() + .set_max_entries(profiler_config.mapsize_stacks); + open_skel + .maps() + .aggregated_stacks() + .set_max_entries(profiler_config.mapsize_aggregated_stacks); + open_skel + .maps() + .unwind_info_chunks() + .set_max_entries(profiler_config.mapsize_unwind_info_chunks); + open_skel + .maps() + .unwind_tables() + .set_max_entries(profiler_config.mapsize_unwind_tables); + open_skel + .maps() + .rate_limits() + .set_max_entries(profiler_config.mapsize_rate_limits); open_skel .rodata_mut() .lightswitch_config .verbose_logging - .write(bpf_logging); + .write(profiler_config.bpf_logging); let bpf = open_skel.load().expect("load skel"); info!("native unwinder BPF program loaded"); let native_unwinder_maps = bpf.maps(); let exec_mappings_fd = native_unwinder_maps.exec_mappings().as_fd(); let mut tracers_builder = TracersSkelBuilder::default(); - tracers_builder.obj_builder.debug(libbpf_debug); + tracers_builder + .obj_builder + .debug(profiler_config.libbpf_debug); let open_tracers = tracers_builder.open().expect("open skel"); open_tracers .maps() @@ -407,7 +473,7 @@ impl Profiler<'_> { perf_buffer.poll(Duration::from_millis(100)).expect("poll"); }); - // Trace events are received here, such memory unmaps. + // Trace events are received here, such as memory unmaps. let tracers_send = self.tracers_chan_send.clone(); let tracers_events_perf_buffer = PerfBufferBuilder::new(self.tracers.maps().tracer_events()) From 7e55b512734ae5682d30905a9ffafc4f59a29dfa Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 5 Aug 2024 18:24:19 -0400 Subject: [PATCH 2/8] eBPF map size info output now works --- src/main.rs | 23 ++++++++++++----------- src/profiler.rs | 25 +++++++++++++++---------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/main.rs b/src/main.rs index a4ff407..74e7048 100644 --- a/src/main.rs +++ b/src/main.rs @@ -257,17 +257,18 @@ fn main() -> Result<(), Box> { } })); - let profiler_config = ProfilerConfig( - args.bpf_logging, - args.duration, - args.sample_freq, - args.mapsize_info, - args.mapsize_stacks, - args.mapsize_aggregated_stacks, - args.mapsize_unwind_info_chunks, - args.mapsize_unwind_tables, - args.mapsize_rate_limits, - ); + let profiler_config = ProfilerConfig { + libbpf_debug: args.libbpf_logs, + bpf_logging: args.bpf_logging, + duration: args.duration, + sample_freq: args.sample_freq, + mapsize_info: args.mapsize_info, + mapsize_stacks: args.mapsize_stacks, + mapsize_aggregated_stacks: args.mapsize_aggregated_stacks, + mapsize_unwind_info_chunks: args.mapsize_unwind_info_chunks, + mapsize_unwind_tables: args.mapsize_unwind_tables, + mapsize_rate_limits: args.mapsize_rate_limits, + }; let mut p: Profiler<'_> = Profiler::new(profiler_config); p.profile_pids(args.pids); p.run(collector.clone()); diff --git a/src/profiler.rs b/src/profiler.rs index 30236a5..553df1e 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -346,25 +346,30 @@ impl Profiler<'_> { // mapsize modifications can only be made before the maps are actually loaded // Initialize map sizes with defaults or modifications open_skel - .maps() + .maps_mut() .stacks() - .set_max_entries(profiler_config.mapsize_stacks); + .set_max_entries(profiler_config.mapsize_stacks) + .expect("Unable to set stacks map max_entries"); open_skel - .maps() + .maps_mut() .aggregated_stacks() - .set_max_entries(profiler_config.mapsize_aggregated_stacks); + .set_max_entries(profiler_config.mapsize_aggregated_stacks) + .expect("Unable to set aggregated_stacks map max_entries"); open_skel - .maps() + .maps_mut() .unwind_info_chunks() - .set_max_entries(profiler_config.mapsize_unwind_info_chunks); + .set_max_entries(profiler_config.mapsize_unwind_info_chunks) + .expect("Unable to set unwind_info_chunks map max_entries"); open_skel - .maps() + .maps_mut() .unwind_tables() - .set_max_entries(profiler_config.mapsize_unwind_tables); + .set_max_entries(profiler_config.mapsize_unwind_tables) + .expect("Unable to set unwind_tables map max_entries"); open_skel - .maps() + .maps_mut() .rate_limits() - .set_max_entries(profiler_config.mapsize_rate_limits); + .set_max_entries(profiler_config.mapsize_rate_limits) + .expect("Unable to set rate_limits map max_entries"); open_skel .rodata_mut() .lightswitch_config From 53a750988e8eac887bd09e0f0bdab4318939e41c Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Thu, 8 Aug 2024 13:16:02 -0400 Subject: [PATCH 3/8] Convert PERF_BUFFER_BYTES into a command line config with default --- src/main.rs | 39 +++++++++++++++++++--- src/profiler.rs | 87 +++++++++++++++++++++++++++---------------------- 2 files changed, 83 insertions(+), 43 deletions(-) diff --git a/src/main.rs b/src/main.rs index 74e7048..2e91fac 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +use core::str; use std::error::Error; use std::fs::File; use std::io::IsTerminal; @@ -61,6 +62,19 @@ fn sample_freq_in_range(s: &str) -> Result { Ok(sample_freq as u16) } +// Return a value if it's a power of 2, otherwise Error +fn value_is_power_of_2(s: &str) -> Result { + let value: usize = s + .parse() + .map_err(|_| format!("`{s}' isn't a valid usize"))?; + // Now we have a value, test whether it's a power of 2 + if (value != 0) && ((value & (value - 1)) == 0) { + Ok(value) + } else { + return Err(format!("{} is not a power of 2", value)); + } +} + /// Given a non-prime unsigned int, return the prime number that precedes it /// as well as the prime that succeeds it fn primes_before_after(non_prime: usize) -> Result<(usize, usize), String> { @@ -152,21 +166,36 @@ struct Cli { /// Where to write the profile. #[arg(long, default_value_t, value_enum)] sender: ProfileSender, + // Buffer Sizes with defaults + #[arg(long, default_value_t = 512 * 1024, value_name = "PERF_BUFFER_BYTES", + help="Size of each profiler perf buffer, in bytes (must be a power of 2)", + value_parser = value_is_power_of_2)] + perf_buffer_bytes: usize, // Print out info on eBPF map sizes - #[arg(long, help = "Print eBPF map sizes that can be configured")] + #[arg(long, help = "Print eBPF map sizes after creation")] mapsize_info: bool, // eBPF map stacks - #[arg(long, default_value_t = 100000)] + #[arg( + long, + default_value_t = 100000, + help = "max number of individual \ + stacks to capture before aggregation" + )] mapsize_stacks: u32, // eBPF map aggregated_stacks #[arg( long, default_value_t = 10000, - help = "Derived from constant MAX_AGGREGATED_STACKS_ENTRIES" + help = "Derived from constant MAX_AGGREGATED_STACKS_ENTRIES - max \ + number of unique stacks after aggregation" )] mapsize_aggregated_stacks: u32, // eBPF map unwind_info_chunks - #[arg(long, default_value_t = 5000)] + #[arg( + long, + default_value_t = 5000, + help = "max number of chunks allowed inside a shard" + )] mapsize_unwind_info_chunks: u32, // eBPF map unwind_tables #[arg( @@ -258,10 +287,12 @@ fn main() -> Result<(), Box> { })); let profiler_config = ProfilerConfig { + // NOTE the difference in this arg name from the actual config name libbpf_debug: args.libbpf_logs, bpf_logging: args.bpf_logging, duration: args.duration, sample_freq: args.sample_freq, + perf_buffer_bytes: args.perf_buffer_bytes, mapsize_info: args.mapsize_info, mapsize_stacks: args.mapsize_stacks, mapsize_aggregated_stacks: args.mapsize_aggregated_stacks, diff --git a/src/profiler.rs b/src/profiler.rs index 553df1e..9acc8a3 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -163,6 +163,8 @@ pub struct Profiler<'bpf> { duration: Duration, // Per-CPU Sampling Frequency of this profile in Hz sample_freq: u16, + // Size of each perf buffer, in include_bytes! + perf_buffer_bytes: usize, session_duration: Duration, } @@ -171,10 +173,6 @@ const MAX_SHARDS: u64 = MAX_UNWIND_INFO_SHARDS as u64; const SHARD_CAPACITY: usize = MAX_UNWIND_TABLE_SIZE as usize; const MAX_CHUNKS: usize = MAX_UNWIND_TABLE_CHUNKS as usize; -// Make each perf buffer 512 KB -// TODO: should make this configurable via a command line argument in future -const PERF_BUFFER_BYTES: usize = 512 * 1024; - #[derive(Debug, Hash, Eq, PartialEq)] pub struct RawAggregatedSample { pub pid: i32, @@ -290,6 +288,7 @@ pub struct ProfilerConfig { pub bpf_logging: bool, pub duration: Duration, pub sample_freq: u16, + pub perf_buffer_bytes: usize, pub mapsize_info: bool, pub mapsize_stacks: u32, pub mapsize_aggregated_stacks: u32, @@ -305,44 +304,13 @@ pub struct ProfilerConfig { // } impl Profiler<'_> { - pub fn new( - profiler_config: ProfilerConfig, - // libbpf_debug: bool, - // bpf_logging: bool, - // duration: Duration, - // sample_freq: u16, - // mapsize_info: bool, - // mapsize_stacks: u32, - // mapsize_aggregated_stacks: u32, - // mapsize_unwind_info_chunks: u32, - // mapsize_unwind_tables: u32, - // mapsize_rate_limits: u32, - ) -> Self { + pub fn new(profiler_config: ProfilerConfig) -> Self { let duration = profiler_config.duration; let sample_freq = profiler_config.sample_freq; + let perf_buffer_bytes = profiler_config.perf_buffer_bytes; let mut skel_builder: ProfilerSkelBuilder = ProfilerSkelBuilder::default(); skel_builder.obj_builder.debug(profiler_config.libbpf_debug); let mut open_skel = skel_builder.open().expect("open skel"); - if profiler_config.mapsize_info { - info!("eBPF map size Configuration:"); - info!("stacks: {}", profiler_config.mapsize_stacks); - info!( - "aggregated_stacks: {}", - profiler_config.mapsize_aggregated_stacks - ); - info!( - "unwind_info_chunks: {}", - profiler_config.mapsize_unwind_info_chunks - ); - info!( - "unwind_tables: {}", - profiler_config.mapsize_unwind_tables - ); - info!( - "rate_limits: {}", - profiler_config.mapsize_rate_limits - ); - } // mapsize modifications can only be made before the maps are actually loaded // Initialize map sizes with defaults or modifications open_skel @@ -380,6 +348,42 @@ impl Profiler<'_> { let native_unwinder_maps = bpf.maps(); let exec_mappings_fd = native_unwinder_maps.exec_mappings().as_fd(); + // If mapsize_info requested, pull the max_entries from each map of + // interest and print out + if profiler_config.mapsize_info { + info!("eBPF ACTUAL map size Configuration:"); + info!( + "stacks: {}", + bpf.maps().stacks().info().unwrap().info.max_entries + ); + info!( + "aggregated_stacks: {}", + bpf.maps() + .aggregated_stacks() + .info() + .unwrap() + .info + .max_entries + ); + info!( + "unwind_info_chunks: {}", + bpf.maps() + .unwind_info_chunks() + .info() + .unwrap() + .info + .max_entries + ); + info!( + "unwind_tables: {}", + bpf.maps().unwind_tables().info().unwrap().info.max_entries + ); + info!( + "rate_limits: {}", + bpf.maps().rate_limits().info().unwrap().info.max_entries + ); + } + let mut tracers_builder = TracersSkelBuilder::default(); tracers_builder .obj_builder @@ -429,6 +433,7 @@ impl Profiler<'_> { profile_receive, duration, sample_freq, + perf_buffer_bytes, session_duration: Duration::from_secs(5), } } @@ -466,12 +471,14 @@ impl Profiler<'_> { // New process events. let chan_send = self.chan_send.clone(); let perf_buffer = PerfBufferBuilder::new(self.bpf.maps().events()) - .pages(PERF_BUFFER_BYTES / page_size::get()) + .pages(self.perf_buffer_bytes / page_size::get()) .sample_cb(move |_cpu: i32, data: &[u8]| { Self::handle_event(&chan_send, data); }) .lost_cb(Self::handle_lost_events) .build() + // TODO: Instead of unwrap, consume and emit any error - usually + // about buffer bytes not being a power of 2 .unwrap(); let _poll_thread = thread::spawn(move || loop { @@ -482,7 +489,7 @@ impl Profiler<'_> { let tracers_send = self.tracers_chan_send.clone(); let tracers_events_perf_buffer = PerfBufferBuilder::new(self.tracers.maps().tracer_events()) - .pages(PERF_BUFFER_BYTES / page_size::get()) + .pages(self.perf_buffer_bytes / page_size::get()) .sample_cb(move |_cpu: i32, data: &[u8]| { let mut event = tracer_event_t::default(); plain::copy_from_bytes(&mut event, data).expect("serde tracers event"); @@ -496,6 +503,8 @@ impl Profiler<'_> { warn!("lost {} events from the tracers", lost_count); }) .build() + // TODO: Instead of unwrap, consume and emit any error - usually + // about buffer bytes not being a power of 2 .unwrap(); let _tracers_poll_thread = thread::spawn(move || loop { From c173910c6b9dcd0a30a2baaadf41949cdeba6e1b Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Thu, 8 Aug 2024 19:35:05 -0400 Subject: [PATCH 4/8] Tests for value_is_power_of_2, and add ProfilerConfig to integration_test --- src/main.rs | 51 +++++++++++++++++++++++++++++++++++---- tests/integration_test.rs | 18 ++++++++++++-- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2e91fac..fd41ccb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -63,15 +63,16 @@ fn sample_freq_in_range(s: &str) -> Result { } // Return a value if it's a power of 2, otherwise Error -fn value_is_power_of_2(s: &str) -> Result { +fn value_is_power_of_two(s: &str) -> Result { let value: usize = s .parse() .map_err(|_| format!("`{s}' isn't a valid usize"))?; // Now we have a value, test whether it's a power of 2 - if (value != 0) && ((value & (value - 1)) == 0) { + // NOTE: Neither 0 nor 1 are a power of 2, so rule them out + if (value != 0) && (value != 1) && ((value & (value - 1)) == 0) { Ok(value) } else { - return Err(format!("{} is not a power of 2", value)); + Err(format!("{} is not a power of 2", value)) } } @@ -169,7 +170,7 @@ struct Cli { // Buffer Sizes with defaults #[arg(long, default_value_t = 512 * 1024, value_name = "PERF_BUFFER_BYTES", help="Size of each profiler perf buffer, in bytes (must be a power of 2)", - value_parser = value_is_power_of_2)] + value_parser = value_is_power_of_two)] perf_buffer_bytes: usize, // Print out info on eBPF map sizes #[arg(long, help = "Print eBPF map sizes after creation")] @@ -363,7 +364,9 @@ mod tests { use super::*; use assert_cmd::Command; use clap::Parser; + use rand::distributions::{Distribution, Uniform}; use rstest::rstest; + use std::collections::HashSet; #[test] fn verify_cli() { @@ -380,7 +383,7 @@ mod tests { let actual = String::from_utf8(cmd.unwrap().stdout).unwrap(); insta::assert_yaml_snapshot!(actual, @r###" --- - "Usage: lightswitch [OPTIONS]\n\nOptions:\n --pids \n Specific PIDs to profile\n\n --tids \n Specific TIDs to profile (these can be outside the PIDs selected above)\n\n --show-unwind-info \n Show unwind info for given binary\n\n --show-info \n Show build ID for given binary\n\n -D, --duration \n How long this agent will run in seconds\n \n [default: 18446744073709551615]\n\n --libbpf-logs\n Enable libbpf logs. This includes the BPF verifier output\n\n --bpf-logging\n Enable BPF programs logging\n\n --logging \n Set lightswitch's logging level\n \n [default: info]\n [possible values: trace, debug, info, warn, error]\n\n --sample-freq \n Per-CPU Sampling Frequency in Hz\n \n [default: 19]\n\n --profile-format \n Output file for Flame Graph in SVG format\n \n [default: flame-graph]\n [possible values: none, flame-graph, pprof]\n\n --profile-name \n Name for the generated profile\n\n --sender \n Where to write the profile\n \n [default: local-disk]\n\n Possible values:\n - none: Discard the profile. Used for kernel tests\n - local-disk\n - remote\n\n -h, --help\n Print help (see a summary with '-h')\n" + "Usage: lightswitch [OPTIONS]\n\nOptions:\n --pids \n Specific PIDs to profile\n\n --tids \n Specific TIDs to profile (these can be outside the PIDs selected above)\n\n --show-unwind-info \n Show unwind info for given binary\n\n --show-info \n Show build ID for given binary\n\n -D, --duration \n How long this agent will run in seconds\n \n [default: 18446744073709551615]\n\n --libbpf-logs\n Enable libbpf logs. This includes the BPF verifier output\n\n --bpf-logging\n Enable BPF programs logging\n\n --logging \n Set lightswitch's logging level\n \n [default: info]\n [possible values: trace, debug, info, warn, error]\n\n --sample-freq \n Per-CPU Sampling Frequency in Hz\n \n [default: 19]\n\n --profile-format \n Output file for Flame Graph in SVG format\n \n [default: flame-graph]\n [possible values: none, flame-graph, pprof]\n\n --profile-name \n Name for the generated profile\n\n --sender \n Where to write the profile\n \n [default: local-disk]\n\n Possible values:\n - none: Discard the profile. Used for kernel tests\n - local-disk\n - remote\n\n --perf-buffer-bytes \n Size of each profiler perf buffer, in bytes (must be a power of 2)\n \n [default: 524288]\n\n --mapsize-info\n Print eBPF map sizes after creation\n\n --mapsize-stacks \n max number of individual stacks to capture before aggregation\n \n [default: 100000]\n\n --mapsize-aggregated-stacks \n Derived from constant MAX_AGGREGATED_STACKS_ENTRIES - max number of unique stacks after aggregation\n \n [default: 10000]\n\n --mapsize-unwind-info-chunks \n max number of chunks allowed inside a shard\n \n [default: 5000]\n\n --mapsize-unwind-tables \n Derived from constant MAX_UNWIND_INFO_SHARDS\n \n [default: 65]\n\n --mapsize-rate-limits \n Derived from constant MAX_PROCESSES\n \n [default: 5000]\n\n -h, --help\n Print help (see a summary with '-h')\n" "###); } @@ -452,4 +455,42 @@ mod tests { } } } + + #[rstest] + fn should_be_powers_of_two() { + let mut test_uint_strings: Vec = vec![]; + for shift in 1..63 { + let val: usize = 2 << shift; + let val_str = val.to_string(); + test_uint_strings.push(val_str); + } + for val_string in test_uint_strings { + assert!(value_is_power_of_two(val_string.as_str()).is_ok()) + } + } + + #[rstest] + fn should_not_be_powers_of_two() { + let mut test_uint_stringset: HashSet = HashSet::new(); + // usizes that ARE powers of two, for later exclusion + for shift in 0..63 { + let val: usize = 2 << shift; + let val_string = val.to_string(); + test_uint_stringset.insert(val_string); + } + // Now, for a random sampling of 500000 integers in the range of usize, + // excluding any that are known to be powers of 2 + let between = Uniform::from(0..=usize::MAX); + let mut rng = rand::thread_rng(); + for _ in 0..500000 { + let usize_int: usize = between.sample(&mut rng); + let usize_int_string = usize_int.to_string(); + if test_uint_stringset.contains(&usize_int_string) { + println!("{}", usize_int_string); + continue; + } + let result = value_is_power_of_two(usize_int_string.as_str()); + assert!(result.is_err()); + } + } } diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 7e8c4cb..7338993 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -6,8 +6,8 @@ use std::time::Duration; use lightswitch::collector::{AggregatorCollector, Collector}; use lightswitch::profile::symbolize_profile; -use lightswitch::profiler::Profiler; use lightswitch::profiler::SymbolizedAggregatedProfile; +use lightswitch::profiler::{Profiler, ProfilerConfig}; /// Find the `nix` binary either in the $PATH or in the below hardcoded location. fn nix_bin() -> String { @@ -97,7 +97,21 @@ fn test_integration() { let collector = Arc::new(Mutex::new( Box::new(AggregatorCollector::new()) as Box )); - let mut p = Profiler::new(bpf_test_debug, bpf_test_debug, Duration::from_secs(5), 999); + + let profiler_config = ProfilerConfig { + libbpf_debug: bpf_test_debug, // changed from default + bpf_logging: bpf_test_debug, // changed from default + duration: Duration::from_secs(5), // changed from default + sample_freq: 999, // changed from default + perf_buffer_bytes: 512 * 1024, + mapsize_info: false, + mapsize_stacks: 100000, + mapsize_aggregated_stacks: 10000, + mapsize_unwind_info_chunks: 5000, + mapsize_unwind_tables: 65, + mapsize_rate_limits: 5000, + }; + let mut p = Profiler::new(profiler_config); p.profile_pids(vec![cpp_proc.pid()]); p.run(collector.clone()); let collector = collector.lock().unwrap(); From 594125f80ceb484915c5838e04dcdcd3519c2638 Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 12 Aug 2024 11:32:45 -0400 Subject: [PATCH 5/8] Corrections after review - add ProfilerConfig default impl --- src/main.rs | 4 ++-- src/profiler.rs | 28 ++++++++++++++++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/main.rs b/src/main.rs index 9dee301..935ffab 100644 --- a/src/main.rs +++ b/src/main.rs @@ -458,7 +458,7 @@ mod tests { #[rstest] fn should_be_powers_of_two() { - let mut test_uint_strings: Vec = vec![]; + let mut test_uint_strings = vec![]; for shift in 1..63 { let val: usize = 2 << shift; let val_str = val.to_string(); @@ -486,7 +486,7 @@ mod tests { let usize_int: usize = between.sample(&mut rng); let usize_int_string = usize_int.to_string(); if test_uint_stringset.contains(&usize_int_string) { - println!("{}", usize_int_string); + // We know this is a power of 2, already tested separately, skip continue; } let result = value_is_power_of_two(usize_int_string.as_str()); diff --git a/src/profiler.rs b/src/profiler.rs index 9acc8a3..7abe41e 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -163,7 +163,7 @@ pub struct Profiler<'bpf> { duration: Duration, // Per-CPU Sampling Frequency of this profile in Hz sample_freq: u16, - // Size of each perf buffer, in include_bytes! + // Size of each perf buffer, in bytes perf_buffer_bytes: usize, session_duration: Duration, } @@ -297,11 +297,27 @@ pub struct ProfilerConfig { pub mapsize_rate_limits: u32, } -// impl Default for Profiler<'_> { -// fn default() -> Self { -// Self::new(false, false, Duration::MAX, 19) -// } -// } +// Note that we zero out most of the fields in the default ProfilerConfig here, +// as we're normally going to pass in the defaults from Clap, and we don't want +// to be in the business of keeping the default values defined in Clap in sync +// with the defaults defined here. +impl Default for ProfilerConfig { + fn default() -> Self { + Self { + libbpf_debug: false, + bpf_logging: false, + duration: Duration::MAX, + sample_freq: 0, + perf_buffer_bytes: 0, + mapsize_info: false, + mapsize_stacks: 0, + mapsize_aggregated_stacks: 0, + mapsize_unwind_info_chunks: 0, + mapsize_unwind_tables: 0, + mapsize_rate_limits: 0, + } + } +} impl Profiler<'_> { pub fn new(profiler_config: ProfilerConfig) -> Self { From 8570d8a02a24b9d45f35dde7577aa5bc859cea01 Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 12 Aug 2024 22:37:07 -0400 Subject: [PATCH 6/8] Sane defaults for ProfilerConfig; tests for perf buffer arg --- src/main.rs | 105 ++++++++++++++++++++++++++++++-------- src/profiler.rs | 28 +++++----- tests/integration_test.rs | 16 ++---- 3 files changed, 103 insertions(+), 46 deletions(-) diff --git a/src/main.rs b/src/main.rs index b3f11dd..794873b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -63,20 +63,31 @@ fn sample_freq_in_range(s: &str) -> Result { Ok(sample_freq as u16) } -// Return a value if it's a power of 2, otherwise Error +// Clap value_parser() in the form of: Fn(&str) -> Result +// Convert a &str into a usize, if possible, and return the result if it's a +// power of 2, otherwise Error fn value_is_power_of_two(s: &str) -> Result { let value: usize = s .parse() .map_err(|_| format!("`{s}' isn't a valid usize"))?; // Now we have a value, test whether it's a power of 2 - // NOTE: Neither 0 nor 1 are a power of 2, so rule them out - if (value != 0) && (value != 1) && ((value & (value - 1)) == 0) { + if is_power_of_two(value) { Ok(value) } else { Err(format!("{} is not a power of 2", value)) } } +fn is_power_of_two(v: usize) -> bool { + // NOTE: Neither 0 nor 1 are a power of 2 (ignoring 2^0 for this use case), + // so rule them out + if (v != 0) && (v != 1) && ((v & (v - 1)) == 0) { + true + } else { + false + } +} + /// Given a non-prime unsigned int, return the prime number that precedes it /// as well as the prime that succeeds it fn primes_before_after(non_prime: usize) -> Result<(usize, usize), String> { @@ -375,7 +386,7 @@ mod tests { use assert_cmd::Command; use clap::Parser; use rand::distributions::{Distribution, Uniform}; - use rstest::rstest; + use rstest::{fixture, rstest}; use std::collections::HashSet; #[test] @@ -466,27 +477,37 @@ mod tests { } } - #[rstest] - fn should_be_powers_of_two() { - let mut test_uint_strings = vec![]; - for shift in 1..63 { + // Powers of 2 in usize range + #[fixture] + fn power_of_two_usize() -> Vec { + let mut test_usizes = vec![]; + for shift in 0..63 { let val: usize = 2 << shift; + test_usizes.push(val); + } + test_usizes + } + + // Powers of 2 represented as Strings + #[fixture] + fn power_of_two_strings(power_of_two_usize: Vec) -> Vec { + let mut test_uint_strings = vec![]; + for val in power_of_two_usize { let val_str = val.to_string(); test_uint_strings.push(val_str); } - for val_string in test_uint_strings { - assert!(value_is_power_of_two(val_string.as_str()).is_ok()) - } + test_uint_strings } - #[rstest] - fn should_not_be_powers_of_two() { - let mut test_uint_stringset: HashSet = HashSet::new(); + // This fixture produces 5 million random results from the range of usize + // integers that are NOT powers of 2 + #[fixture] + fn all_but_power_of_two_usize(power_of_two_usize: Vec) -> Vec { + let mut test_usize_set: HashSet = HashSet::new(); + let mut test_usize_not_p2: Vec = vec![]; // usizes that ARE powers of two, for later exclusion - for shift in 0..63 { - let val: usize = 2 << shift; - let val_string = val.to_string(); - test_uint_stringset.insert(val_string); + for val in power_of_two_usize { + test_usize_set.insert(val); } // Now, for a random sampling of 500000 integers in the range of usize, // excluding any that are known to be powers of 2 @@ -494,12 +515,54 @@ mod tests { let mut rng = rand::thread_rng(); for _ in 0..500000 { let usize_int: usize = between.sample(&mut rng); - let usize_int_string = usize_int.to_string(); - if test_uint_stringset.contains(&usize_int_string) { + if test_usize_set.contains(&usize_int) { // We know this is a power of 2, already tested separately, skip continue; } - let result = value_is_power_of_two(usize_int_string.as_str()); + test_usize_not_p2.push(usize_int); + } + test_usize_not_p2 + } + + // all_but_power_of_two_usize, but as Strings + #[fixture] + fn all_but_power_of_two_strings(all_but_power_of_two_usize: Vec) -> Vec { + let mut test_uint_strings: Vec = vec![]; + for val in all_but_power_of_two_usize { + let val_str = val.to_string(); + test_uint_strings.push(val_str); + } + test_uint_strings + } + + // Testing is_power_of_two predicate used by perf_buffer_bytes + // value_parser() + #[rstest] + fn test_should_be_powers_of_two(power_of_two_usize: Vec) { + for val in power_of_two_usize { + assert!(is_power_of_two(val)) + } + } + + #[rstest] + fn test_should_not_be_powers_of_two(all_but_power_of_two_usize: Vec) { + for val in all_but_power_of_two_usize { + assert!(!is_power_of_two(val)) + } + } + + // Testing the value_parser() implementation for perf_buffer_bytes + #[rstest] + fn args_should_be_powers_of_two(power_of_two_strings: Vec) { + for val_string in power_of_two_strings { + assert!(value_is_power_of_two(val_string.as_str()).is_ok()) + } + } + + #[rstest] + fn args_should_not_be_powers_of_two(all_but_power_of_two_strings: Vec) { + for non_p2_string in all_but_power_of_two_strings { + let result = value_is_power_of_two(&non_p2_string.as_str()); assert!(result.is_err()); } } diff --git a/src/profiler.rs b/src/profiler.rs index ca8c9a7..cd7e618 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -300,24 +300,24 @@ pub struct ProfilerConfig { pub mapsize_rate_limits: u32, } -// Note that we zero out most of the fields in the default ProfilerConfig here, -// as we're normally going to pass in the defaults from Clap, and we don't want +// Note that we normally pass in the defaults from Clap, and we don't want // to be in the business of keeping the default values defined in Clap in sync -// with the defaults defined here. +// with the defaults defined here. So these are some defaults that will +// almost always be overridden. impl Default for ProfilerConfig { fn default() -> Self { Self { libbpf_debug: false, bpf_logging: false, duration: Duration::MAX, - sample_freq: 0, - perf_buffer_bytes: 0, + sample_freq: 19, + perf_buffer_bytes: 512 * 1024, mapsize_info: false, - mapsize_stacks: 0, - mapsize_aggregated_stacks: 0, - mapsize_unwind_info_chunks: 0, - mapsize_unwind_tables: 0, - mapsize_rate_limits: 0, + mapsize_stacks: 100000, + mapsize_aggregated_stacks: 10000, + mapsize_unwind_info_chunks: 5000, + mapsize_unwind_tables: 65, + mapsize_rate_limits: 5000, } } } @@ -501,8 +501,8 @@ impl Profiler<'_> { }) .lost_cb(Self::handle_lost_events) .build() - // TODO: Instead of unwrap, consume and emit any error - usually - // about buffer bytes not being a power of 2 + // TODO: Instead of unwrap, consume and emit any error, with + // .expect() perhaps? .unwrap(); let _poll_thread = thread::spawn(move || loop { @@ -525,8 +525,8 @@ impl Profiler<'_> { warn!("lost {} events from the tracers", lost_count); }) .build() - // TODO: Instead of unwrap, consume and emit any error - usually - // about buffer bytes not being a power of 2 + // TODO: Instead of unwrap, consume and emit any error, with + // .expect() perhaps? .unwrap(); let _tracers_poll_thread = thread::spawn(move || loop { diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 93ee1f0..24bac4b 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -101,17 +101,11 @@ fn test_integration() { )); let profiler_config = ProfilerConfig { - libbpf_debug: bpf_test_debug, // changed from default - bpf_logging: bpf_test_debug, // changed from default - duration: Duration::from_secs(5), // changed from default - sample_freq: 999, // changed from default - perf_buffer_bytes: 512 * 1024, - mapsize_info: false, - mapsize_stacks: 100000, - mapsize_aggregated_stacks: 10000, - mapsize_unwind_info_chunks: 5000, - mapsize_unwind_tables: 65, - mapsize_rate_limits: 5000, + libbpf_debug: bpf_test_debug, + bpf_logging: bpf_test_debug, + duration: Duration::from_secs(5), + sample_freq: 999, + ..Default::default() }; let (_stop_signal_send, stop_signal_receive) = bounded(1); let mut p = Profiler::new(profiler_config, stop_signal_receive); From 04a386fe5e66d86532ed2a6727d14ef5580fdc4c Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 12 Aug 2024 23:16:19 -0400 Subject: [PATCH 7/8] Make clippy happy --- src/main.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main.rs b/src/main.rs index 794873b..035685d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -81,11 +81,7 @@ fn value_is_power_of_two(s: &str) -> Result { fn is_power_of_two(v: usize) -> bool { // NOTE: Neither 0 nor 1 are a power of 2 (ignoring 2^0 for this use case), // so rule them out - if (v != 0) && (v != 1) && ((v & (v - 1)) == 0) { - true - } else { - false - } + (v != 0) && (v != 1) && ((v & (v - 1)) == 0) } /// Given a non-prime unsigned int, return the prime number that precedes it From e15ebbff6bfc41d127ac2dabfd9302553d556315 Mon Sep 17 00:00:00 2001 From: Gordon Marler Date: Mon, 12 Aug 2024 23:28:33 -0400 Subject: [PATCH 8/8] Another clippy fix --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 035685d..e44e06e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -558,7 +558,7 @@ mod tests { #[rstest] fn args_should_not_be_powers_of_two(all_but_power_of_two_strings: Vec) { for non_p2_string in all_but_power_of_two_strings { - let result = value_is_power_of_two(&non_p2_string.as_str()); + let result = value_is_power_of_two(non_p2_string.as_str()); assert!(result.is_err()); } }