From cf036686d788986c8c5f7737e804ffba9a781deb Mon Sep 17 00:00:00 2001 From: r1viollet Date: Tue, 23 Jan 2024 10:19:04 +0100 Subject: [PATCH 1/3] Consider timestamps in comm and fork events To prevent clearing mmap data when it is not useful, we consider timestamps of comm and fork events --- include/dso_hdr.hpp | 1 + include/unwind.hpp | 4 +++- src/ddprof_worker.cc | 35 ++++++++++++++++++++++------------- src/dso_hdr.cc | 9 +++++++++ src/unwind.cc | 8 ++++++-- test/dso-ut.cc | 13 +++++++++++++ 6 files changed, 54 insertions(+), 16 deletions(-) diff --git a/include/dso_hdr.hpp b/include/dso_hdr.hpp index 1ea40f153..e05b4c5c7 100644 --- a/include/dso_hdr.hpp +++ b/include/dso_hdr.hpp @@ -118,6 +118,7 @@ class DsoHdr { bool maybe_insert_erase_overlap(Dso &&dso, PerfClock::time_point timestamp); // Clear all dsos and regions associated with this pid + bool pid_free(int pid, PerfClock::time_point timestamp); void pid_free(int pid); // Duplicate mapping info from parent_pid into pid diff --git a/include/unwind.hpp b/include/unwind.hpp index dc0cad93f..8b182ecbf 100644 --- a/include/unwind.hpp +++ b/include/unwind.hpp @@ -6,6 +6,7 @@ #pragma once #include "ddres_def.hpp" +#include "perf_clock.hpp" #include @@ -27,6 +28,7 @@ DDRes unwindstate_unwind(UnwindState *us); void unwind_cycle(UnwindState *us); // Clear unwinding structures of this pid -void unwind_pid_free(UnwindState *us, pid_t pid); +void unwind_pid_free(UnwindState *us, pid_t pid, + PerfClock::time_point timestamp); } // namespace ddprof diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index 0f29ba345..df0c66300 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -47,7 +47,8 @@ const DDPROF_STATS s_cycled_stats[] = { const long k_clock_ticks_per_sec = sysconf(_SC_CLK_TCK); /// Remove all structures related to -DDRes worker_pid_free(DDProfContext &ctx, pid_t el); +DDRes worker_pid_free(DDProfContext &ctx, pid_t el, + PerfClock::time_point timestamp); DDRes clear_unvisited_pids(DDProfContext &ctx); @@ -231,7 +232,7 @@ DDRes ddprof_unwind_sample(DDProfContext &ctx, perf_event_sample *sample, if (us->_dwfl_wrapper && us->_dwfl_wrapper->_inconsistent) { // Loaded modules were inconsistent, assume we should flush everything. LG_WRN("(Inconsistent DWFL/DSOs)%d - Free associated objects", us->pid); - DDRES_CHECK_FWD(worker_pid_free(ctx, us->pid)); + DDRES_CHECK_FWD(worker_pid_free(ctx, us->pid, PerfClock::now())); } return res; } @@ -303,10 +304,11 @@ DDRes aggregate_live_allocations(DDProfContext &ctx) { return {}; } -DDRes worker_pid_free(DDProfContext &ctx, pid_t el) { +DDRes worker_pid_free(DDProfContext &ctx, pid_t el, + PerfClock::time_point timestamp) { DDRES_CHECK_FWD(aggregate_live_allocations_for_pid(ctx, el)); UnwindState *us = ctx.worker_ctx.us; - unwind_pid_free(us, el); + unwind_pid_free(us, el, timestamp); ctx.worker_ctx.live_allocation.clear_pid(el); return {}; } @@ -314,8 +316,9 @@ DDRes worker_pid_free(DDProfContext &ctx, pid_t el) { DDRes clear_unvisited_pids(DDProfContext &ctx) { UnwindState *us = ctx.worker_ctx.us; const std::vector pids_remove = us->dwfl_hdr.get_unvisited(); + const PerfClock::time_point now = PerfClock::now(); for (pid_t const el : pids_remove) { - DDRES_CHECK_FWD(worker_pid_free(ctx, el)); + DDRES_CHECK_FWD(worker_pid_free(ctx, el, now)); } us->dwfl_hdr.reset_unvisited(); return {}; @@ -584,21 +587,21 @@ void ddprof_pr_lost(DDProfContext &ctx, const perf_event_lost *lost, } DDRes ddprof_pr_comm(DDProfContext &ctx, const perf_event_comm *comm, - int watcher_pos) { + int watcher_pos, PerfClock::time_point timestamp) { // Change in process name (assuming exec) : clear all associated dso if (comm->header.misc & PERF_RECORD_MISC_COMM_EXEC) { LG_DBG("<%d>(COMM)%d -> %s", watcher_pos, comm->pid, comm->comm); - DDRES_CHECK_FWD(worker_pid_free(ctx, comm->pid)); + DDRES_CHECK_FWD(worker_pid_free(ctx, comm->pid, timestamp)); } return {}; } DDRes ddprof_pr_fork(DDProfContext &ctx, const perf_event_fork *frk, - int watcher_pos) { + int watcher_pos, PerfClock::time_point timestamp) { LG_DBG("<%d>(FORK)%d -> %d/%d", watcher_pos, frk->ppid, frk->pid, frk->tid); if (frk->ppid != frk->pid) { // Clear everything and populate at next error or with coming samples - DDRES_CHECK_FWD(worker_pid_free(ctx, frk->pid)); + DDRES_CHECK_FWD(worker_pid_free(ctx, frk->pid, timestamp)); ctx.worker_ctx.us->dso_hdr.pid_fork(frk->pid, frk->ppid); } return {}; @@ -745,8 +748,11 @@ DDRes ddprof_worker_process_event(const perf_event_header *hdr, int watcher_pos, break; case PERF_RECORD_COMM: if (wpid->pid) { - DDRES_CHECK_FWD(ddprof_pr_comm( - ctx, reinterpret_cast(hdr), watcher_pos)); + auto timestamp = perf_clock_time_point_from_timestamp( + hdr_time(hdr, watcher->sample_type)); + DDRES_CHECK_FWD( + ddprof_pr_comm(ctx, reinterpret_cast(hdr), + watcher_pos, timestamp)); } break; case PERF_RECORD_EXIT: @@ -757,8 +763,11 @@ DDRes ddprof_worker_process_event(const perf_event_header *hdr, int watcher_pos, break; case PERF_RECORD_FORK: if (wpid->pid) { - DDRES_CHECK_FWD(ddprof_pr_fork( - ctx, reinterpret_cast(hdr), watcher_pos)); + auto timestamp = perf_clock_time_point_from_timestamp( + hdr_time(hdr, watcher->sample_type)); + DDRES_CHECK_FWD( + ddprof_pr_fork(ctx, reinterpret_cast(hdr), + watcher_pos, timestamp)); } break; diff --git a/src/dso_hdr.cc b/src/dso_hdr.cc index 21efb5953..8526e78f7 100644 --- a/src/dso_hdr.cc +++ b/src/dso_hdr.cc @@ -404,6 +404,15 @@ DsoHdr::DsoFindRes DsoHdr::dso_find_or_backpopulate(pid_t pid, return dso_find_or_backpopulate(pid_mapping, pid, addr); } +bool DsoHdr::pid_free(int pid, PerfClock::time_point timestamp) { + const auto &pid_mapping = _pid_map[pid]; + if (timestamp < pid_mapping._backpopulate_state.last_backpopulate_time) { + return false; + } + pid_free(pid); + return true; +} + void DsoHdr::pid_free(int pid) { _pid_map.erase(pid); } bool DsoHdr::pid_backpopulate(pid_t pid, int &nb_elts_added) { diff --git a/src/unwind.cc b/src/unwind.cc index 9899665c2..bade40b8d 100644 --- a/src/unwind.cc +++ b/src/unwind.cc @@ -123,8 +123,12 @@ DDRes unwindstate_unwind(UnwindState *us) { return res; } -void unwind_pid_free(UnwindState *us, pid_t pid) { - us->dso_hdr.pid_free(pid); +void unwind_pid_free(UnwindState *us, pid_t pid, + PerfClock::time_point timestamp) { + if (!(us->dso_hdr.pid_free(pid, timestamp))) { + LG_DBG("(PID Free)%d -> avoid free of mappings (%ld)", pid, + timestamp.time_since_epoch().count()); + } us->dwfl_hdr.clear_pid(pid); us->symbol_hdr.clear(pid); us->process_hdr.clear(pid); diff --git a/test/dso-ut.cc b/test/dso-ut.cc index d21fed4d1..740e35c75 100644 --- a/test/dso-ut.cc +++ b/test/dso-ut.cc @@ -160,6 +160,19 @@ TEST(DSOTest, erase) { } } +TEST(DSOTest, avoid_erase) { + PerfClock::init(); // init otherwise it returns 0 + DsoHdr dso_hdr; + PerfClock::time_point old_now = PerfClock::now(); + pid_t my_pid = getpid(); + usleep(100); + int nb_elts = 0; + dso_hdr.pid_backpopulate(my_pid, nb_elts); + EXPECT_GT(nb_elts, 0); + EXPECT_FALSE(dso_hdr.pid_free(my_pid, old_now)); + EXPECT_GE(dso_hdr.get_nb_dso(), nb_elts); +} + TEST(DSOTest, find_same) { DsoHdr dso_hdr; fill_mock_hdr(dso_hdr); From b3ee9ad243dd96014f4f2df7a8ceb2d71214ade4 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Wed, 24 Jan 2024 14:16:25 +0100 Subject: [PATCH 2/3] Consider timestamps in fork and comm events Ensure caches are not cleared when PID maps are not cleared --- src/unwind.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/unwind.cc b/src/unwind.cc index bade40b8d..2a93db44d 100644 --- a/src/unwind.cc +++ b/src/unwind.cc @@ -128,10 +128,11 @@ void unwind_pid_free(UnwindState *us, pid_t pid, if (!(us->dso_hdr.pid_free(pid, timestamp))) { LG_DBG("(PID Free)%d -> avoid free of mappings (%ld)", pid, timestamp.time_since_epoch().count()); + } else { + us->dwfl_hdr.clear_pid(pid); + us->symbol_hdr.clear(pid); + us->process_hdr.clear(pid); } - us->dwfl_hdr.clear_pid(pid); - us->symbol_hdr.clear(pid); - us->process_hdr.clear(pid); } void unwind_cycle(UnwindState *us) { From a521115d7e7fa82a3ad53f7a8ace78e4b3401d5c Mon Sep 17 00:00:00 2001 From: r1viollet Date: Fri, 26 Jan 2024 14:28:32 +0100 Subject: [PATCH 3/3] Unwind free - Minor API change Communicate the result of the unwind free to caller --- include/unwind.hpp | 2 +- src/unwind.cc | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/unwind.hpp b/include/unwind.hpp index 8b182ecbf..d7724e3a2 100644 --- a/include/unwind.hpp +++ b/include/unwind.hpp @@ -28,7 +28,7 @@ DDRes unwindstate_unwind(UnwindState *us); void unwind_cycle(UnwindState *us); // Clear unwinding structures of this pid -void unwind_pid_free(UnwindState *us, pid_t pid, +bool unwind_pid_free(UnwindState *us, pid_t pid, PerfClock::time_point timestamp); } // namespace ddprof diff --git a/src/unwind.cc b/src/unwind.cc index 2a93db44d..92b743661 100644 --- a/src/unwind.cc +++ b/src/unwind.cc @@ -123,16 +123,17 @@ DDRes unwindstate_unwind(UnwindState *us) { return res; } -void unwind_pid_free(UnwindState *us, pid_t pid, +bool unwind_pid_free(UnwindState *us, pid_t pid, PerfClock::time_point timestamp) { if (!(us->dso_hdr.pid_free(pid, timestamp))) { LG_DBG("(PID Free)%d -> avoid free of mappings (%ld)", pid, timestamp.time_since_epoch().count()); - } else { - us->dwfl_hdr.clear_pid(pid); - us->symbol_hdr.clear(pid); - us->process_hdr.clear(pid); + return false; } + us->dwfl_hdr.clear_pid(pid); + us->symbol_hdr.clear(pid); + us->process_hdr.clear(pid); + return true; } void unwind_cycle(UnwindState *us) {