cleanup(libscap): always enable global counters when per-cpu ones are…

… enabled

Signed-off-by: Andrea Terzolo <[email protected]>

test/libscap/test_suites/engines/bpf/bpf.cpp

@@ -138,8 +138,8 @@ TEST(bpf, metrics_v2_check_per_CPU_stats)
 
  ssize_t num_possible_CPUs = num_possible_cpus();
 
- // We want to check our CPUs counters
- uint32_t flags = METRICS_V2_KERNEL_COUNTERS;
+ // Enabling `METRICS_V2_KERNEL_COUNTERS_PER_CPU` we also enable `METRICS_V2_KERNEL_COUNTERS`
+ uint32_t flags = METRICS_V2_KERNEL_COUNTERS_PER_CPU;
  uint32_t nstats = 0;
  int32_t rc = 0;
  const metrics_v2* stats_v2 = scap_get_stats_v2(h, flags, &nstats, &rc);
@@ -151,9 +151,17 @@ TEST(bpf, metrics_v2_check_per_CPU_stats)
  ssize_t found = 0;
  char expected_name[METRIC_NAME_MAX] = "";
  snprintf(expected_name, METRIC_NAME_MAX, N_EVENTS_PER_CPU_PREFIX"%ld", found);
+ bool check_general_kernel_counters_presence = false;
 
  while(i < nstats)
  {
+ // We check if `METRICS_V2_KERNEL_COUNTERS` are enabled as well
+ if(strncmp(stats_v2[i].name, N_EVENTS_PREFIX, sizeof(N_EVENTS_PREFIX)-1) == 0)
+ {
+ check_general_kernel_counters_presence = true;
+ continue;
+ }
+
  // `sizeof(N_EVENTS_PER_CPU_PREFIX)-1` because we need to exclude the `\0`
  if(strncmp(stats_v2[i].name, N_EVENTS_PER_CPU_PREFIX, sizeof(N_EVENTS_PER_CPU_PREFIX)-1) == 0)
  {
@@ -176,6 +184,8 @@ TEST(bpf, metrics_v2_check_per_CPU_stats)
  }
  }
 
+ ASSERT_TRUE(check_general_kernel_counters_presence) << "per-CPU counter are enabled but general kernel counters are not";
+
  // This test could fail in case of rare race conditions in which the number of available CPUs changes
  // between the scap_open and the `num_possible_cpus` function. In CI we shouldn't have hot plugs so probably we
  // can live with this. 
@@ -220,6 +230,16 @@ TEST(bpf, metrics_v2_check_results)
  FAIL() << "unable to find stat '" << stat_name << "' into the array";
  }
  }
+
+ // Check per-CPU stats are not enabled since we didn't provide the flag.
+ for(i = 0; i < nstats; i++)
+ {
+ if(strncmp(stats_v2[i].name, N_EVENTS_PER_CPU_PREFIX, sizeof(N_EVENTS_PER_CPU_PREFIX)-1) == 0)
+ {
+ FAIL() << "per-CPU counters are enabled but we didn't provide the flag!"; 
+ } 
+ }
+
  scap_close(h);
 }
 

test/libscap/test_suites/engines/kmod/kmod.cpp

@@ -193,8 +193,8 @@ TEST(kmod, metrics_v2_check_per_CPU_stats)
 
  ssize_t num_online_CPUs = sysconf(_SC_NPROCESSORS_ONLN);
 
- // We want to check our CPUs counters
- uint32_t flags = METRICS_V2_KERNEL_COUNTERS;
+ // Enabling `METRICS_V2_KERNEL_COUNTERS_PER_CPU` we also enable `METRICS_V2_KERNEL_COUNTERS`
+ uint32_t flags = METRICS_V2_KERNEL_COUNTERS_PER_CPU;
  uint32_t nstats = 0;
  int32_t rc = 0;
  const metrics_v2* stats_v2 = scap_get_stats_v2(h, flags, &nstats, &rc);
@@ -206,9 +206,17 @@ TEST(kmod, metrics_v2_check_per_CPU_stats)
  ssize_t found = 0;
  char expected_name[METRIC_NAME_MAX] = "";
  snprintf(expected_name, METRIC_NAME_MAX, N_EVENTS_PER_DEVICE_PREFIX"%ld", found);
+ bool check_general_kernel_counters_presence = false;
 
  while(i < nstats)
  {
+ // We check if `METRICS_V2_KERNEL_COUNTERS` are enabled as well
+ if(strncmp(stats_v2[i].name, N_EVENTS_PREFIX, sizeof(N_EVENTS_PREFIX)-1) == 0)
+ {
+ check_general_kernel_counters_presence = true;
+ continue;
+ }
+
  // `sizeof(N_EVENTS_PER_DEVICE_PREFIX)-1` because we need to exclude the `\0`
  if(strncmp(stats_v2[i].name, N_EVENTS_PER_DEVICE_PREFIX, sizeof(N_EVENTS_PER_DEVICE_PREFIX)-1) == 0)
  {
@@ -231,6 +239,8 @@ TEST(kmod, metrics_v2_check_per_CPU_stats)
  }
  }
 
+ ASSERT_TRUE(check_general_kernel_counters_presence) << "per-CPU counter are enabled but general kernel counters are not";
+
  // This test could fail in case of rare race conditions in which the number of online CPUs changes
  // between the scap_open and the `sysconf(_SC_NPROCESSORS_ONLN)` function. In CI we shouldn't have hot plugs so probably we
  // can live with this. 
@@ -271,6 +281,16 @@ TEST(kmod, metrics_v2_check_results)
  FAIL() << "unable to find stat '" << stat_name << "' into the array";
  }
  }
+
+ // Check per-CPU stats are not enabled since we didn't provide the flag.
+ for(i = 0; i < nstats; i++)
+ {
+ if(strncmp(stats_v2[i].name, N_EVENTS_PER_DEVICE_PREFIX, sizeof(N_EVENTS_PER_DEVICE_PREFIX)-1) == 0)
+ {
+ FAIL() << "per-CPU counters are enabled but we didn't provide the flag!"; 
+ } 
+ }
+
  scap_close(h);
 }
 

test/libscap/test_suites/engines/modern_bpf/modern_bpf.cpp

@@ -257,8 +257,8 @@ TEST(modern_bpf, metrics_v2_check_per_CPU_stats)
 
  ssize_t num_possible_CPUs = num_possible_cpus();
 
- // We want to check our CPUs counters
- uint32_t flags = METRICS_V2_KERNEL_COUNTERS;
+ // Enabling `METRICS_V2_KERNEL_COUNTERS_PER_CPU` we also enable `METRICS_V2_KERNEL_COUNTERS`
+ uint32_t flags = METRICS_V2_KERNEL_COUNTERS_PER_CPU;
  uint32_t nstats = 0;
  int32_t rc = 0;
  const metrics_v2* stats_v2 = scap_get_stats_v2(h, flags, &nstats, &rc);
@@ -270,9 +270,17 @@ TEST(modern_bpf, metrics_v2_check_per_CPU_stats)
  ssize_t found = 0;
  char expected_name[METRIC_NAME_MAX] = "";
  snprintf(expected_name, METRIC_NAME_MAX, N_EVENTS_PER_CPU_PREFIX"%ld", found);
+ bool check_general_kernel_counters_presence = false;
 
  while(i < nstats)
  {
+ // We check if `METRICS_V2_KERNEL_COUNTERS` are enabled as well
+ if(strncmp(stats_v2[i].name, N_EVENTS_PREFIX, sizeof(N_EVENTS_PREFIX)-1) == 0)
+ {
+ check_general_kernel_counters_presence = true;
+ continue;
+ }
+
  // `sizeof(N_EVENTS_PER_CPU_PREFIX)-1` because we need to exclude the `\0`
  if(strncmp(stats_v2[i].name, N_EVENTS_PER_CPU_PREFIX, sizeof(N_EVENTS_PER_CPU_PREFIX)-1) == 0)
  {
@@ -295,6 +303,8 @@ TEST(modern_bpf, metrics_v2_check_per_CPU_stats)
  }
  }
 
+ ASSERT_TRUE(check_general_kernel_counters_presence) << "per-CPU counter are enabled but general kernel counters are not";
+
  // This test could fail in case of rare race conditions in which the number of available CPUs changes
  // between the scap_open and the `num_possible_cpus` function. In CI we shouldn't have hot plugs so probably we
  // can live with this. 
@@ -340,6 +350,16 @@ TEST(modern_bpf, metrics_v2_check_results)
  FAIL() << "unable to find stat '" << stat_name << "' into the array";
  }
  }
+
+ // Check per-CPU stats are not enabled since we didn't provide the flag.
+ for(i = 0; i < nstats; i++)
+ {
+ if(strncmp(stats_v2[i].name, N_EVENTS_PER_CPU_PREFIX, sizeof(N_EVENTS_PER_CPU_PREFIX)-1) == 0)
+ {
+ FAIL() << "per-CPU counters are enabled but we didn't provide the flag!"; 
+ } 
+ }
+
  scap_close(h);
 }
 

userspace/libpman/src/stats.c

@@ -53,7 +53,7 @@ typedef enum modern_bpf_libbpf_stats
 } modern_bpf_libbpf_stats;
 
 const char *const modern_bpf_kernel_counters_stats_names[] = {
- [MODERN_BPF_N_EVTS] = "n_evts",
+ [MODERN_BPF_N_EVTS] = N_EVENTS_PREFIX,
  [MODERN_BPF_N_DROPS_BUFFER_TOTAL] = "n_drops_buffer_total",
  [MODERN_BPF_N_DROPS_BUFFER_CLONE_FORK_ENTER] = "n_drops_buffer_clone_fork_enter",
  [MODERN_BPF_N_DROPS_BUFFER_CLONE_FORK_EXIT] = "n_drops_buffer_clone_fork_exit",
@@ -254,43 +254,6 @@ struct metrics_v2 *pman_get_metrics_v2(uint32_t flags, uint32_t *nstats, int32_t
  offset = pos;
  }
 
- /* KERNEL COUNTERS PER CPU STATS 
- * The following `if` handle the case in which we want to get the metrics per CPU but not the global ones.
- * It is an unsual case but at the moment we support it.
- */
- if ((flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU) && !(flags & METRICS_V2_KERNEL_COUNTERS))
- {
- char error_message[MAX_ERROR_MESSAGE_LEN];
- int counter_maps_fd = bpf_map__fd(g_state.skel->maps.counter_maps);
- if(counter_maps_fd <= 0)
- {
- pman_print_error("unable to get 'counter_maps' fd during kernel stats processing");
- return NULL;
- }
-
- struct counter_map cnt_map = {};
- for(uint32_t index = 0; index < g_state.n_possible_cpus; index++)
- {
- if(bpf_map_lookup_elem(counter_maps_fd, &index, &cnt_map) < 0)
- {
- snprintf(error_message, MAX_ERROR_MESSAGE_LEN, "unable to get the counter map for CPU %d", index);
- pman_print_error((const char *)error_message);
- close(counter_maps_fd);
- return NULL;
- }
-
- // We set the num events for that CPU.
- set_u64_monotonic_kernel_counter(offset, cnt_map.n_evts, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(g_state.stats[offset].name, METRIC_NAME_MAX, N_EVENTS_PER_CPU_PREFIX"%d", index);
- offset++;
-
- // We set the drops for that CPU.
- set_u64_monotonic_kernel_counter(offset, cnt_map.n_drops_buffer + cnt_map.n_drops_max_event_size, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(g_state.stats[offset].name, METRIC_NAME_MAX, N_DROPS_PER_CPU_PREFIX"%d", index);
- offset++;
- }
- }
-
  /* LIBBPF STATS */
 
  /* At the time of writing (Apr 2, 2023) libbpf stats are only available on a per program granularity.

userspace/libscap/engine/bpf/scap_bpf.c

@@ -50,7 +50,7 @@ limitations under the License.
 #include <libscap/strerror.h>
 
 static const char * const bpf_kernel_counters_stats_names[] = {
- [BPF_N_EVTS] = "n_evts",
+ [BPF_N_EVTS] = N_EVENTS_PREFIX,
  [BPF_N_DROPS_BUFFER_TOTAL] = "n_drops_buffer_total",
  [BPF_N_DROPS_BUFFER_CLONE_FORK_ENTER] = "n_drops_buffer_clone_fork_enter",
  [BPF_N_DROPS_BUFFER_CLONE_FORK_EXIT] = "n_drops_buffer_clone_fork_exit",
@@ -1803,33 +1803,6 @@ const struct metrics_v2* scap_bpf_get_stats_v2(struct scap_engine_handle engine,
  offset = pos;
  }
 
- /* KERNEL COUNTERS PER CPU STATS 
- * The following `if` handle the case in which we want to get the metrics per CPU but not the global ones.
- * It is an unsual case but at the moment we support it.
- */
- if ((flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU) && !(flags & METRICS_V2_KERNEL_COUNTERS))
- {
- struct scap_bpf_per_cpu_state v = {};
- for(int cpu = 0; cpu < handle->m_ncpus; cpu++)
- {
- if(bpf_map_lookup_elem(handle->m_bpf_map_fds[SCAP_LOCAL_STATE_MAP], &cpu, &v) < 0)
- {
- *rc = scap_errprintf(handle->m_lasterr, errno, "Error looking up local state %d", cpu);
- return NULL;
- }
-
- // We set the num events for that CPU.
- set_u64_monotonic_kernel_counter(&(stats[offset]), v.n_evts, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(stats[offset].name, METRIC_NAME_MAX, N_EVENTS_PER_CPU_PREFIX"%d", cpu);
- offset++;
-
- // We set the drops for that CPU.
- set_u64_monotonic_kernel_counter(&(stats[offset]), v.n_drops_buffer + v.n_drops_scratch_map + v.n_drops_pf + v.n_drops_bug, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(stats[offset].name, METRIC_NAME_MAX, N_DROPS_PER_CPU_PREFIX"%d", cpu);
- offset++;
- }
- }
-
  /* LIBBPF STATS */
 
  /* At the time of writing (Apr 2, 2023) libbpf stats are only available on a per program granularity.

userspace/libscap/engine/kmod/scap_kmod.c

@@ -40,7 +40,7 @@ limitations under the License.
 #include <assert.h>
 
 static const char * const kmod_kernel_counters_stats_names[] = {
- [KMOD_N_EVTS] = "n_evts",
+ [KMOD_N_EVTS] = N_EVENTS_PREFIX,
  [KMOD_N_DROPS_BUFFER_TOTAL] = "n_drops_buffer_total",
  [KMOD_N_DROPS_BUFFER_CLONE_FORK_ENTER] = "n_drops_buffer_clone_fork_enter",
  [KMOD_N_DROPS_BUFFER_CLONE_FORK_EXIT] = "n_drops_buffer_clone_fork_exit",
@@ -666,7 +666,6 @@ const struct metrics_v2* scap_kmod_get_stats_v2(struct scap_engine_handle engine
  dev->m_bufinfo->n_drops_pf;
  stats[KMOD_N_PREEMPTIONS].value.u64 += dev->m_bufinfo->n_preemptions;
 
- // This is just a way to avoid the double loop on the number of devices.
  if((flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU))
  {
  // We set the num events for that CPU.
@@ -683,28 +682,6 @@ const struct metrics_v2* scap_kmod_get_stats_v2(struct scap_engine_handle engine
  offset = pos;
  }
 
- /* KERNEL COUNTERS PER CPU STATS 
- * The following `if` handle the case in which we want to get the metrics per CPU but not the global ones.
- * It is an unsual case but at the moment we support it.
- */
- if ((flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU) && !(flags & METRICS_V2_KERNEL_COUNTERS))
- {
- for(uint32_t j = 0; j < devset->m_ndevs; j++)
- {
- struct scap_device *dev = &devset->m_devs[j];
-
- // We set the num events for that CPU.
- set_u64_monotonic_kernel_counter(&(stats[offset]), dev->m_bufinfo->n_evts, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(stats[offset].name, METRIC_NAME_MAX, N_EVENTS_PER_DEVICE_PREFIX"%d", j);
- offset++;
-
- // We set the drops for that CPU.
- set_u64_monotonic_kernel_counter(&(stats[offset]), dev->m_bufinfo->n_drops_buffer + dev->m_bufinfo->n_drops_pf, METRICS_V2_KERNEL_COUNTERS_PER_CPU);
- snprintf(stats[offset].name, METRIC_NAME_MAX, N_DROPS_PER_DEVICE_PREFIX"%d", j);
- offset++;
- }
- }
-
  *nstats = offset;
  *rc = SCAP_SUCCESS;
  return stats;

userspace/libscap/metrics_v2.h

@@ -30,6 +30,11 @@ extern "C" {
 //
 #define METRIC_NAME_MAX 512
 
+//
+// Prefix name for n_evts metric (Used by all drivers)
+//
+#define N_EVENTS_PREFIX "n_evts"
+
 //
 // Prefix names for per-CPU metrics (Used by legacy ebpf and modern ebpf)
 //

userspace/libscap/scap.c

@@ -302,6 +302,12 @@ int32_t scap_get_stats(scap_t* handle, scap_stats* stats)
 //
 const struct metrics_v2* scap_get_stats_v2(scap_t* handle, uint32_t flags, uint32_t* nstats, int32_t* rc)
 {
+ // If we enable per-cpu counters, we also enable kernel global counters by default.
+ if(flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU)
+ {
+ flags |= METRICS_V2_KERNEL_COUNTERS;
+ }
+
  if(handle && handle->m_vtable)
  {
  return handle->m_vtable->get_stats_v2(handle->m_engine, flags, nstats, rc);

userspace/libsinsp/metrics_collector.cpp

@@ -696,7 +696,7 @@ void libs_metrics_collector::snapshot()
  * libscap metrics
  */
 
- if((m_metrics_flags & METRICS_V2_KERNEL_COUNTERS) || (m_metrics_flags & METRICS_V2_LIBBPF_STATS))
+ if((m_metrics_flags & METRICS_V2_KERNEL_COUNTERS) || (m_metrics_flags & METRICS_V2_LIBBPF_STATS) || (m_metrics_flags & METRICS_V2_KERNEL_COUNTERS_PER_CPU))
  {
  uint32_t nstats = 0;
  int32_t rc = 0;

userspace/libsinsp/metrics_collector.h

@@ -339,7 +339,7 @@ class libs_metrics_collector
 private:
  sinsp* m_inspector;
  std::shared_ptr<sinsp_stats_v2> m_sinsp_stats_v2;
- uint32_t m_metrics_flags = METRICS_V2_KERNEL_COUNTERS | METRICS_V2_LIBBPF_STATS | METRICS_V2_RESOURCE_UTILIZATION | METRICS_V2_STATE_COUNTERS | METRICS_V2_PLUGINS;
+ uint32_t m_metrics_flags = METRICS_V2_KERNEL_COUNTERS | METRICS_V2_LIBBPF_STATS | METRICS_V2_RESOURCE_UTILIZATION | METRICS_V2_STATE_COUNTERS | METRICS_V2_PLUGINS | METRICS_V2_KERNEL_COUNTERS_PER_CPU;
  std::vector<metrics_v2> m_metrics;
 };