[record & replay] Tidy up BCCWrapper & perf profiler before adding …

…the `rr` sub-tree. (#1697) Summary: We implement a few cosmetic changes to BCC wrapper that support the record & replay effort: 1. We add method `CommonPerfBufferSetup` which will be shared between the recording & normal BCC wrapper impls. 2. We rename the vector of perf buffer specs to `perf_buffer_specs_` i.e. because its inherent name is confusing. 3. We return a status from `PollPerfBuffer` which will be useful for the replaying BCC wrapper. 4. We return BCC as a pointer from BCC wrapper to enable the eventual replaying BCC wrapper. 5. In the perf profiler, we change the data type for the perf buffer names to `std::string` to avoid creating new strings every time they are used. In response to code review comments, we will rebase #1693 onto this PR, and continue to split that PR into pieces and merge it gradually. Type of change: /kind feature Test Plan: Existing test coverage. --------- Signed-off-by: Pete Stevenson <[email protected]>
pixie-io · Sep 18, 2023 · e48d522 · e48d522
1 parent 56033f3
commit e48d522
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 49 deletions.
diff --git a/src/stirling/bpf_tools/bcc_wrapper.cc b/src/stirling/bpf_tools/bcc_wrapper.cc
@@ -341,20 +341,34 @@ void BCCWrapperImpl::DetachTracepoints() {
   tracepoints_.clear();
 }
 
-Status BCCWrapperImpl::OpenPerfBuffer(const PerfBufferSpec& perf_buffer) {
-  const int kPageSizeBytes = system::Config::GetInstance().PageSizeBytes();
-  int num_pages = IntRoundUpDivide(perf_buffer.size_bytes, kPageSizeBytes);
+int BCCWrapperImpl::CommonPerfBufferSetup(const PerfBufferSpec& perf_buffer_spec) {
+  DCHECK(perf_buffer_spec.cb_cookie != nullptr) << "perf_buffer_spec.cb_cookie must be non-null.";
+  DCHECK(perf_buffer_spec.size_bytes > 0) << "perf_buffer_spec.cb_cookie must greater than zero.";
+  perf_buffer_specs_.push_back(perf_buffer_spec);
+
+  const int page_size_bytes = system::Config::GetInstance().PageSizeBytes();
+  const int required_num_pages = IntRoundUpDivide(perf_buffer_spec.size_bytes, page_size_bytes);
 
   // Perf buffers must be sized to a power of 2.
-  num_pages = IntRoundUpToPow2(num_pages);
+  const int num_pages = IntRoundUpToPow2(required_num_pages);
 
   VLOG(1) << absl::Substitute(
       "Opening perf buffer: [$0] [allocated_num_pages=$1 allocated_size_bytes=$2] (per cpu)",
-      perf_buffer.ToString(), num_pages, num_pages * kPageSizeBytes);
-  PX_RETURN_IF_ERROR(bpf_.open_perf_buffer(std::string(perf_buffer.name),
-                                           perf_buffer.probe_output_fn, perf_buffer.probe_loss_fn,
-                                           perf_buffer.cb_cookie, num_pages));
-  perf_buffers_.push_back(perf_buffer);
+      perf_buffer_spec.ToString(), num_pages, num_pages * page_size_bytes);
+
+  return num_pages;
+}
+
+Status BCCWrapperImpl::OpenPerfBuffer(const PerfBufferSpec& perf_buffer_spec) {
+  const int num_pages = CommonPerfBufferSetup(perf_buffer_spec);
+
+  const std::string& name = perf_buffer_spec.name;
+  void* cb_cookie = perf_buffer_spec.cb_cookie;
+  auto& data_fn = perf_buffer_spec.probe_output_fn;
+  auto& loss_fn = perf_buffer_spec.probe_loss_fn;
+
+  PX_RETURN_IF_ERROR(BPF()->open_perf_buffer(name, data_fn, loss_fn, cb_cookie, num_pages));
+
   ++num_open_perf_buffers_;
   return Status::OK();
 }
@@ -374,11 +388,11 @@ Status BCCWrapperImpl::ClosePerfBuffer(const PerfBufferSpec& perf_buffer) {
 }
 
 void BCCWrapperImpl::ClosePerfBuffers() {
-  for (const PerfBufferSpec& p : perf_buffers_) {
+  for (const auto& p : perf_buffer_specs_) {
     auto res = ClosePerfBuffer(p);
     LOG_IF(ERROR, !res.ok()) << res.msg();
   }
-  perf_buffers_.clear();
+  perf_buffer_specs_.clear();
 }
 
 Status BCCWrapperImpl::AttachPerfEvent(const PerfEventSpec& perf_event) {
@@ -423,16 +437,19 @@ std::string BCCWrapperImpl::GetKProbeTargetName(const KProbeSpec& probe) {
   return target;
 }
 
-void BCCWrapperImpl::PollPerfBuffer(std::string_view perf_buffer_name, int timeout_ms) {
-  auto perf_buffer = bpf_.get_perf_buffer(std::string(perf_buffer_name));
-  if (perf_buffer != nullptr) {
-    perf_buffer->poll(timeout_ms);
+Status BCCWrapperImpl::PollPerfBuffer(const std::string& name, const int timeout_ms) {
+  auto perf_buffer = bpf_.get_perf_buffer(name);
+  if (perf_buffer == nullptr) {
+    return error::NotFound(absl::Substitute("Perf buffer \"$0\" not found.", name));
   }
+  perf_buffer->poll(timeout_ms);
+  return Status::OK();
 }
 
-void BCCWrapperImpl::PollPerfBuffers(int timeout_ms) {
-  for (const auto& spec : perf_buffers_) {
-    PollPerfBuffer(spec.name, timeout_ms);
+void BCCWrapperImpl::PollPerfBuffers(const int timeout_ms) {
+  for (const auto& spec : perf_buffer_specs_) {
+    const auto s = PollPerfBuffer(spec.name, timeout_ms);
+    LOG_IF(ERROR, !s.ok()) << s.msg();
   }
 }
 
@@ -448,7 +465,9 @@ std::unique_ptr<BCCWrapper> CreateBCC() { return std::make_unique<BCCWrapperImpl
 
 std::unique_ptr<WrappedBCCStackTable> WrappedBCCStackTable::Create(bpf_tools::BCCWrapper* bcc,
                                                                    const std::string& name) {
-  return std::make_unique<WrappedBCCStackTableImpl>(bcc, name);
+  using BaseT = WrappedBCCStackTable;
+  using ImplT = WrappedBCCStackTableImpl;
+  return CreateBCCWrappedMapOrArray<BaseT, ImplT>(bcc, name);
 }
 
 }  // namespace bpf_tools

diff --git a/src/stirling/bpf_tools/bcc_wrapper.h b/src/stirling/bpf_tools/bcc_wrapper.h
@@ -89,7 +89,7 @@ class BCCWrapper {
     return task_struct_offsets_opt_;
   }
 
-  virtual ebpf::BPF& BPF() = 0;
+  virtual ebpf::BPF* BPF() = 0;
 
   /**
    * Compiles the BPF code.
@@ -137,7 +137,6 @@ class BCCWrapper {
   /**
    * Open a perf buffer for reading events.
    * @param perf_buff Specifications of the perf buffer (name, callback function, etc.).
-   * PollPerfBuffer().
    * @return Error if perf buffer cannot be opened (e.g. perf buffer does not exist).
    */
   virtual Status OpenPerfBuffer(const PerfBufferSpec& perf_buffer) = 0;
@@ -211,16 +210,25 @@ class BCCWrapper {
                                       const uint64_t config) = 0;
 
   /**
-   * Drains all of the opened perf buffers, calling the handle function that was
-   * specified in the PerfBufferSpec when OpenPerfBuffer was called.
+   * Drains a specific perf buffer.
    *
+   * @param name The name of the perf buffer to drain.
    * @param timeout_ms If there's no event in the perf buffer, then timeout_ms specifies the
    *                   amount of time to wait for an event to arrive before returning.
    *                   Default is 0, because if nothing is ready, then we want to go back to sleep
    *                   and catch new events in the next iteration.
+   * @return Error status. This is useful for callers directly using this method, but not for
+   *                       PollPerfBuffers() which directly uses the list of open perf buffers.
+   */
+  virtual Status PollPerfBuffer(const std::string& name, const int timeout_ms = 0) = 0;
+
+  /**
+   * Drains all of the opened perf buffers, calling the handle function that was
+   * specified in the PerfBufferSpec when OpenPerfBuffer was called.
+   *
+   * @param timeout_ms Pass through to PollPerfBuffer()
    */
-  virtual void PollPerfBuffers(int timeout_ms = 0) = 0;
-  virtual void PollPerfBuffer(std::string_view perf_buffer_name, int timeout_ms) = 0;
+  virtual void PollPerfBuffers(const int timeout_ms = 0) = 0;
 
   /**
    * Detaches all probes, and closes all perf buffers that are open.
@@ -233,9 +241,10 @@ class BCCWrapper {
   static size_t num_open_perf_buffers() { return num_open_perf_buffers_; }
   static size_t num_attached_perf_events() { return num_attached_perf_events_; }
 
+  virtual Status ClosePerfBuffer(const PerfBufferSpec& perf_buffer) = 0;
+
  protected:
   FRIEND_TEST(BCCWrapperTest, DetachUProbe);
-  virtual Status ClosePerfBuffer(const PerfBufferSpec& perf_buffer) = 0;
 
   // These are static counters across all instances, because:
   // 1) We want to ensure we have cleaned all BPF resources up across *all* instances (no leaks).
@@ -254,13 +263,15 @@ class BCCWrapper {
 
 class BCCWrapperImpl : public BCCWrapper {
  public:
+  int CommonPerfBufferSetup(const PerfBufferSpec& perf_buffer_spec);
+
   virtual ~BCCWrapperImpl() {
     // Not really required, because BPF destructor handles these.
     // But we do it anyways out of paranoia.
     Close();
   }
 
-  ebpf::BPF& BPF() override { return bpf_; }
+  ebpf::BPF* BPF() override { return &bpf_; }
 
   Status InitBPFProgram(std::string_view bpf_program, std::vector<std::string> cflags = {},
                         bool requires_linux_headers = true,
@@ -283,13 +294,11 @@ class BCCWrapperImpl : public BCCWrapper {
     PX_RETURN_IF_ERROR(bpf_.open_perf_event(table_name, type, config));
     return Status::OK();
   }
-  void PollPerfBuffers(int timeout_ms = 0) override;
-  void PollPerfBuffer(std::string_view perf_buffer_name, int timeout_ms) override;
+  void PollPerfBuffers(const int timeout_ms = 0) override;
+  Status PollPerfBuffer(const std::string& name, const int timeout_ms = 0) override;
   void Close() override;
 
- protected:
   Status ClosePerfBuffer(const PerfBufferSpec& perf_buffer) override;
-  std::vector<PerfBufferSpec> perf_buffers_;
 
  private:
   FRIEND_TEST(BCCWrapperTest, DetachUProbe);
@@ -315,6 +324,10 @@ class BCCWrapperImpl : public BCCWrapper {
   std::vector<TracepointSpec> tracepoints_;
   std::vector<PerfEventSpec> perf_events_;
 
+ protected:
+  std::vector<PerfBufferSpec> perf_buffer_specs_;
+
+ private:
   std::string system_headers_include_dir_;
 
   // Initialize this with one of the below bitmask flags to turn on different debug output.
@@ -332,7 +345,6 @@ class BCCWrapperImpl : public BCCWrapper {
 
 std::unique_ptr<BCCWrapper> CreateBCC();
 
-// Wrapped maps & arrays.
 template <typename T>
 class WrappedBCCArrayTable {
  public:
@@ -366,11 +378,13 @@ class WrappedBCCArrayTableImpl : public WrappedBCCArrayTable<T> {
   }
 
   WrappedBCCArrayTableImpl(bpf_tools::BCCWrapper* bcc, const std::string& name) : name_(name) {
-    underlying_ = std::make_unique<U>(bcc->BPF().get_array_table<T>(name_));
+    underlying_ = std::make_unique<U>(bcc->BPF()->get_array_table<T>(name_));
   }
 
- private:
+ protected:
   const std::string name_;
+
+ private:
   char const* const err_msg_ = "BPF failed to $0 value for array table: $1, index: $2. $3.";
   std::unique_ptr<U> underlying_;
 };
@@ -458,11 +472,13 @@ class WrappedBCCMapImpl : public WrappedBCCMap<K, V, kUserSpaceManaged> {
   }
 
   WrappedBCCMapImpl(bpf_tools::BCCWrapper* bcc, const std::string& name) : name_(name) {
-    underlying_ = std::make_unique<U>(bcc->BPF().get_hash_table<K, V>(name_));
+    underlying_ = std::make_unique<U>(bcc->BPF()->get_hash_table<K, V>(name_));
   }
 
- private:
+ protected:
   const std::string name_;
+
+ private:
   char const* const err_msg_ = "BPF failed to $0 value for map: $1. $2.";
   std::unique_ptr<U> underlying_;
   absl::flat_hash_set<K> shadow_keys_;
@@ -495,7 +511,7 @@ class WrappedBCCPerCPUArrayTableImpl : public WrappedBCCPerCPUArrayTable<T> {
 
   WrappedBCCPerCPUArrayTableImpl(bpf_tools::BCCWrapper* bcc, const std::string& name)
       : name_(name) {
-    underlying_ = std::make_unique<U>(bcc->BPF().get_percpu_array_table<T>(name_));
+    underlying_ = std::make_unique<U>(bcc->BPF()->get_percpu_array_table<T>(name_));
   }
 
  private:
@@ -529,19 +545,19 @@ class WrappedBCCStackTableImpl : public WrappedBCCStackTable {
   void ClearStackID(const int stack_id) override { underlying_->clear_stack_id(stack_id); }
 
   WrappedBCCStackTableImpl(bpf_tools::BCCWrapper* bcc, const std::string& name) : name_(name) {
-    underlying_ = std::make_unique<U>(bcc->BPF().get_stack_table(name_));
+    underlying_ = std::make_unique<U>(bcc->BPF()->get_stack_table(name_));
   }
 
- private:
+ protected:
   const std::string name_;
+
+ private:
   std::unique_ptr<U> underlying_;
 };
 
 // Creators fns for wrapped maps & arrays:
 template <typename BaseT, typename ImplT>
 std::unique_ptr<BaseT> CreateBCCWrappedMapOrArray(BCCWrapper* bcc, const std::string& name) {
-  // The decision logic for "normal" vs. "recording" vs. "replaying" impl. will be inserted
-  // here in a future PR.
   return std::make_unique<ImplT>(bcc, name);
 }
 

diff --git a/src/stirling/source_connectors/perf_profiler/bcc_bpf_intf/stack_event.h b/src/stirling/source_connectors/perf_profiler/bcc_bpf_intf/stack_event.h
@@ -19,7 +19,7 @@
 #pragma once
 
 #ifdef __cplusplus
-#include <utility>
+#include <string>
 #endif
 
 #include "src/stirling/upid/upid.h"
@@ -81,6 +81,8 @@ static const uint64_t kOverflowError = 1ULL << kOverflowBitPos;
 static const uint64_t kMapReadFailureError = 1ULL << kMapReadFailureBitPos;
 
 #ifdef __cplusplus
-static constexpr std::string_view kHistogramAName = "histogram_a";
-static constexpr std::string_view kHistogramBName = "histogram_b";
+// NOLINTNEXTLINE : runtime/string
+static const std::string kHistogramAName = "histogram_a";
+// NOLINTNEXTLINE : runtime/string
+static const std::string kHistogramBName = "histogram_b";
 #endif
diff --git a/src/stirling/source_connectors/perf_profiler/perf_profile_connector.cc b/src/stirling/source_connectors/perf_profiler/perf_profile_connector.cc
@@ -138,8 +138,8 @@ Status PerfProfileConnector::InitImpl() {
       {"sample_call_stack", static_cast<uint64_t>(stack_trace_sampling_period_.count())});
 
   const auto perf_buffer_specs = MakeArray<bpf_tools::PerfBufferSpec>(
-      {{std::string(kHistogramAName), HandleHistoEvent, HandleHistoLoss, this, perf_buffer_size},
-       {std::string(kHistogramBName), HandleHistoEvent, HandleHistoLoss, this, perf_buffer_size}});
+      {{kHistogramAName, HandleHistoEvent, HandleHistoLoss, this, perf_buffer_size},
+       {kHistogramBName, HandleHistoEvent, HandleHistoLoss, this, perf_buffer_size}});
 
   PX_RETURN_IF_ERROR(bcc_->InitBPFProgram(profiler_bcc_script, defines));
   PX_RETURN_IF_ERROR(bcc_->AttachSamplingProbes(probe_specs));
@@ -345,13 +345,14 @@ void PerfProfileConnector::ProcessBPFStackTraces(ConnectorContext* ctx, DataTabl
   // Read out the perf buffer that contains the histogram for this iteration.
   // TODO(jps): change PollPerfBuffer() to use std::chrono.
   constexpr int kPollTimeoutMS = 0;
-  bcc_->PollPerfBuffer(perfbuf_name, kPollTimeoutMS);
+  const auto poll_status = bcc_->PollPerfBuffer(perfbuf_name, kPollTimeoutMS);
+  LOG_IF(ERROR, !poll_status.ok()) << poll_status.msg();
 
   ++transfer_count_;
 
   // First, tell BPF to switch the maps it writes to.
-  const auto s = profiler_state_->SetValue(kTransferCountIdx, transfer_count_);
-  LOG_IF(ERROR, !s.ok()) << "Error writing transfer_count_";
+  const auto map_status = profiler_state_->SetValue(kTransferCountIdx, transfer_count_);
+  LOG_IF(ERROR, !map_status.ok()) << "Error writing transfer_count_: " << map_status.msg();
 
   // Read BPF stack traces & histogram, build records, incorporate records to data table.
   CreateRecords(stack_traces.get(), ctx, data_table);