From 4ab9979de1353d9db32f9f1031a2026d0a76805f Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 18:06:15 +0000 Subject: [PATCH 01/11] Add libdatadog5 and timestamps --- cmake/Findlibdatadog.cmake | 2 +- include/pprof/ddprof_pprof.hpp | 13 ++++++++++--- src/ddprof_worker.cc | 18 ++++++++++++------ src/exporter/ddprof_exporter.cc | 15 ++++++--------- src/pprof/ddprof_pprof.cc | 19 +++++++++++++------ tools/libdatadog_checksums.txt | 8 ++++---- 6 files changed, 46 insertions(+), 29 deletions(-) diff --git a/cmake/Findlibdatadog.cmake b/cmake/Findlibdatadog.cmake index a9b752c4a..464bb9026 100644 --- a/cmake/Findlibdatadog.cmake +++ b/cmake/Findlibdatadog.cmake @@ -5,7 +5,7 @@ # libdatadog : common profiler imported libraries # https://github.com/DataDog/libdatadog/releases/tag/v0.8.0 set(TAG_LIBDATADOG - "v4.0.0" + "v5.0.0" CACHE STRING "libdatadog github tag") set(Datadog_ROOT ${VENDOR_PATH}/libdatadog-${TAG_LIBDATADOG}) diff --git a/include/pprof/ddprof_pprof.hpp b/include/pprof/ddprof_pprof.hpp index e7e1362d1..95e856256 100644 --- a/include/pprof/ddprof_pprof.hpp +++ b/include/pprof/ddprof_pprof.hpp @@ -24,18 +24,25 @@ struct DDProfPProf { Tags _tags; }; +struct DDProfValuePack { + int64_t value; + uint64_t count; + uint64_t timestamp; +}; + DDRes pprof_create_profile(DDProfPProf *pprof, DDProfContext &ctx); /** * Aggregate to the existing profile the provided unwinding output. * @param uw_output - * @param value matching the watcher type (ex : cpu period) + * @param pack combines the value, count, and timestamp of an event * @param watcher_idx matches the registered order at profile creation * @param pprof */ DDRes pprof_aggregate(const UnwindOutput *uw_output, - const SymbolHdr &symbol_hdr, uint64_t value, - uint64_t count, const PerfWatcher *watcher, + const SymbolHdr &symbol_hdr, + const DDProfValuePack &pack, + const PerfWatcher *watcher, EventAggregationModePos value_pos, DDProfPProf *pprof); DDRes pprof_reset(DDProfPProf *pprof); diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index 5b9e52672..f981aa10a 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -23,6 +23,7 @@ #include "unwind_helpers.hpp" #include "unwind_state.hpp" +#include #include #include #include @@ -73,12 +74,12 @@ DDRes report_lost_events(DDProfContext &ctx) { watcher->sample_frequency : watcher->sample_period; - auto value = period * nb_lost; + int64_t value = period * nb_lost; LG_WRN("Reporting %lu lost samples (cumulated lost value: %lu) for " "watcher #%d", nb_lost, value, watcher_idx); DDRES_CHECK_FWD(pprof_aggregate( - &us->output, us->symbol_hdr, value, nb_lost, watcher, kSumPos, + &us->output, us->symbol_hdr, {value, nb_lost, 0}, watcher, kSumPos, ctx.worker_ctx.pprof[ctx.worker_ctx.i_current_pprof])); ctx.worker_ctx.lost_events_per_watcher[watcher_idx] = 0; } @@ -236,9 +237,14 @@ DDRes aggregate_livealloc_stack( const LiveAllocation::PprofStacks::value_type &alloc_info, DDProfContext &ctx, const PerfWatcher *watcher, DDProfPProf *pprof, const SymbolHdr &symbol_hdr) { + DDProfValuePack pack{ + alloc_info.second._value, + static_cast(std::max(0, alloc_info.second._count)), + 0 + }; + DDRES_CHECK_FWD( - pprof_aggregate(&alloc_info.first, symbol_hdr, alloc_info.second._value, - alloc_info.second._count, watcher, kLiveSumPos, pprof)); + pprof_aggregate(&alloc_info.first, symbol_hdr, pack, watcher, kLiveSumPos, pprof)); if (ctx.params.show_samples) { ddprof_print_sample(alloc_info.first, symbol_hdr, alloc_info.second._value, kLiveSumPos, *watcher); @@ -410,8 +416,8 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, // in lib mode we don't aggregate (protect to avoid link failures) int const i_export = ctx.worker_ctx.i_current_pprof; DDProfPProf *pprof = ctx.worker_ctx.pprof[i_export]; - DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, sample_val, - 1, watcher, kSumPos, pprof)); + DDProfValuePack pack{static_cast(sample_val), 1, sample->time}; + DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); if (ctx.params.show_samples) { ddprof_print_sample(us->output, us->symbol_hdr, sample->period, kSumPos, *watcher); diff --git a/src/exporter/ddprof_exporter.cc b/src/exporter/ddprof_exporter.cc index e1a83ca3f..d20d198b4 100644 --- a/src/exporter/ddprof_exporter.cc +++ b/src/exporter/ddprof_exporter.cc @@ -292,7 +292,7 @@ DDRes ddprof_exporter_export(ddog_prof_Profile *profile, DDProfExporter *exporter) { DDRes res = ddres_init(); ddog_prof_Profile_SerializeResult serialized_result = - ddog_prof_Profile_serialize(profile, nullptr, nullptr); + ddog_prof_Profile_serialize(profile, nullptr, nullptr, nullptr); if (serialized_result.tag != DDOG_PROF_PROFILE_SERIALIZE_RESULT_OK) { defer { ddog_Error_drop(&serialized_result.err); }; DDRES_RETURN_ERROR_LOG(DD_WHAT_EXPORTER, "Failed to serialize: %s", @@ -309,29 +309,26 @@ DDRes ddprof_exporter_export(ddog_prof_Profile *profile, ddog_Timespec const start = encoded_profile->start; ddog_Timespec const end = encoded_profile->end; - ddog_ByteSlice const profile_data = { - .ptr = encoded_profile->buffer.ptr, - .len = encoded_profile->buffer.len, - }; - if (exporter->_export) { ddog_Vec_Tag ffi_additional_tags = ddog_Vec_Tag_new(); defer { ddog_Vec_Tag_drop(ffi_additional_tags); }; DDRES_CHECK_FWD( fill_cycle_tags(additional_tags, profile_seq, ffi_additional_tags);); - LG_NTC("[EXPORTER] Export buffer of size %lu", profile_data.len); + LG_NTC("[EXPORTER] Export buffer of size %lu", encoded_profile->buffer.len); // Backend has some logic based on the following naming ddog_prof_Exporter_File files_[] = {{ .name = to_CharSlice("auto.pprof"), - .file = profile_data, + .file = ddog_Vec_U8_as_slice(&encoded_profile->buffer), }}; ddog_prof_Exporter_Slice_File const files = {.ptr = files_, .len = std::size(files_)}; ddog_prof_Exporter_Request_BuildResult res_request = - ddog_prof_Exporter_Request_build(exporter->_exporter, start, end, files, + ddog_prof_Exporter_Request_build(exporter->_exporter, start, end, + ddog_prof_Exporter_Slice_File_empty(), + files, &ffi_additional_tags, nullptr, nullptr, k_timeout_ms); diff --git a/src/pprof/ddprof_pprof.cc b/src/pprof/ddprof_pprof.cc index 44cd67f06..1fd771107 100644 --- a/src/pprof/ddprof_pprof.cc +++ b/src/pprof/ddprof_pprof.cc @@ -229,10 +229,16 @@ DDRes pprof_create_profile(DDProfPProf *pprof, DDProfContext &ctx) { .value = default_period, }; } - pprof->_profile = ddog_prof_Profile_new( + auto prof_res = ddog_prof_Profile_new( sample_types, pprof_values.get_num_sample_type_ids() > 0 ? &period : nullptr, nullptr); + if (prof_res.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) { + ddog_Error_drop(&prof_res.err); + DDRES_RETURN_ERROR_LOG(DD_WHAT_PPROF, "Unable to create new profile"); + } + pprof->_profile = prof_res.ok; + // Add relevant tags { bool const include_kernel = @@ -260,8 +266,9 @@ DDRes pprof_free_profile(DDProfPProf *pprof) { // Assumption of API is that sample is valid in a single type DDRes pprof_aggregate(const UnwindOutput *uw_output, - const SymbolHdr &symbol_hdr, uint64_t value, - uint64_t count, const PerfWatcher *watcher, + const SymbolHdr &symbol_hdr, + const DDProfValuePack &pack, + const PerfWatcher *watcher, EventAggregationModePos value_pos, DDProfPProf *pprof) { const ddprof::SymbolTable &symbol_table = symbol_hdr._symbol_table; @@ -271,10 +278,10 @@ DDRes pprof_aggregate(const UnwindOutput *uw_output, const PProfIndices &pprof_indices = watcher->pprof_indices[value_pos]; int64_t values[k_max_value_types] = {}; assert(pprof_indices.pprof_index != -1); - values[pprof_indices.pprof_index] = value; + values[pprof_indices.pprof_index] = pack.value; if (watcher_has_countable_sample_type(watcher)) { assert(pprof_indices.pprof_count_index != -1); - values[pprof_indices.pprof_count_index] = count; + values[pprof_indices.pprof_count_index] = pack.count; } ddog_prof_Location locations_buff[kMaxStackDepth]; @@ -347,7 +354,7 @@ DDRes pprof_aggregate(const UnwindOutput *uw_output, .labels = {.ptr = labels, .len = labels_num}, }; - auto res = ddog_prof_Profile_add(profile, sample); + auto res = ddog_prof_Profile_add(profile, sample, pack.timestamp); if (res.tag != DDOG_PROF_PROFILE_RESULT_OK) { defer { ddog_Error_drop(&res.err); }; auto msg = ddog_Error_message(&res.err); diff --git a/tools/libdatadog_checksums.txt b/tools/libdatadog_checksums.txt index 7056e250c..e2160e291 100644 --- a/tools/libdatadog_checksums.txt +++ b/tools/libdatadog_checksums.txt @@ -1,4 +1,4 @@ -8819ede8a8dbbc133e014426ac2b151a31b977142a228afec3759e7a66fc2a4f libdatadog-aarch64-alpine-linux-musl.tar.gz -dff1c1b87c9cc304aa8f009421117b3822190055ba34cd267c8f4500c46637c0 libdatadog-aarch64-unknown-linux-gnu.tar.gz -e65540dc0c21fbc06ae884622f17c73c52e475613c0628f9a9630624d8bb01b2 libdatadog-x86_64-alpine-linux-musl.tar.gz -a987f8dd6c1aae92fc33613ae3b11473f701ef0f1a858c3a9b0673b83d2c2a90 libdatadog-x86_64-unknown-linux-gnu.tar.gz +f5fb14372b8d6018f4759eb81447dfec0d3393e8e4e44fe890c42045563b5de4 libdatadog-aarch64-alpine-linux-musl.tar.gz +2b3d1c5c3965ab4a9436aff4e101814eddaa59b59cb984ce6ebda45613aadbc3 libdatadog-aarch64-unknown-linux-gnu.tar.gz +060482ff1c34940cf7fad1dc841693602e04e4fa54ac9e9f08cb688efcbab137 libdatadog-x86_64-alpine-linux-musl.tar.gz +11c09440271dd4374b8fca8f0faa66c43a5e057aae05902543beb1e6cb382e52 libdatadog-x86_64-unknown-linux-gnu.tar.gz From 9f1b43a1439387a07aab430ba707cbf71e25657e Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 21:36:04 +0000 Subject: [PATCH 02/11] Fix up tests, add CLI --- include/ddprof_cli.hpp | 1 + include/ddprof_context.hpp | 1 + src/ddprof_cli.cc | 6 ++++++ src/ddprof_context_lib.cc | 1 + src/ddprof_worker.cc | 5 ++++- test/ddprof_exporter-ut.cc | 2 +- test/ddprof_pprof-ut.cc | 6 +++--- 7 files changed, 17 insertions(+), 5 deletions(-) diff --git a/include/ddprof_cli.hpp b/include/ddprof_cli.hpp index 95ab51915..bed4d4b9c 100644 --- a/include/ddprof_cli.hpp +++ b/include/ddprof_cli.hpp @@ -69,6 +69,7 @@ struct DDProfCLI { bool help_extended{false}; int socket{-1}; bool continue_exec{false}; + bool use_timestamps{false}; // args std::vector command_line; diff --git a/include/ddprof_context.hpp b/include/ddprof_context.hpp index 507c26849..dfb02b3f3 100644 --- a/include/ddprof_context.hpp +++ b/include/ddprof_context.hpp @@ -30,6 +30,7 @@ struct DDProfContext { int dd_profiling_fd{-1}; // opened file descriptor to our internal lib ddprof::UniqueFd sockfd; bool show_samples{false}; + bool use_timestamps{false}; cpu_set_t cpu_affinity{}; std::string switch_user; std::string internal_stats; diff --git a/src/ddprof_cli.cc b/src/ddprof_cli.cc index 7ec64646d..1e8b0e4a5 100644 --- a/src/ddprof_cli.cc +++ b/src/ddprof_cli.cc @@ -233,6 +233,9 @@ int DDProfCLI::parse(int argc, const char *argv[]) { app.add_flag("--show_samples", show_samples, "Display captured samples as logs.\n") ->group("Debug options"); + app.add_flag("--use_timestamps", use_timestamps, + "Put timestamps on sampled events.\n") + ->group("Debug options"); app.add_flag("--version,-v", version, "Display the profiler's version.\n") ->group("Debug options"); app.add_option("--enable", enable, @@ -452,6 +455,9 @@ void DDProfCLI::print() const { if (show_samples) { PRINT_NFO(" - show_samples: %s", show_samples ? "true" : "false"); } + if (use_timestamps) { + PRINT_NFO(" - use_timestamps: %s", use_timestamps ? "true" : "false"); + } PRINT_NFO(" - fault_info: %s", fault_info ? "true" : "false"); if (default_stack_sample_size != k_default_perf_stack_sample_size) { diff --git a/src/ddprof_context_lib.cc b/src/ddprof_context_lib.cc index abcb567c9..eb77c18f5 100644 --- a/src/ddprof_context_lib.cc +++ b/src/ddprof_context_lib.cc @@ -74,6 +74,7 @@ void copy_cli_values(const DDProfCLI &ddprof_cli, DDProfContext &ctx) { } ctx.params.show_samples = ddprof_cli.show_samples; + ctx.params.use_timestamps = ddprof_cli.use_timestamps; ctx.params.fault_info = ddprof_cli.fault_info; ctx.params.initial_loaded_libs_check_delay = ddprof_cli.initial_loaded_libs_check_delay; diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index f981aa10a..cf646a853 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -416,7 +416,10 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, // in lib mode we don't aggregate (protect to avoid link failures) int const i_export = ctx.worker_ctx.i_current_pprof; DDProfPProf *pprof = ctx.worker_ctx.pprof[i_export]; - DDProfValuePack pack{static_cast(sample_val), 1, sample->time}; + DDProfValuePack pack{static_cast(sample_val), 1, 0}; + if (ctx.params.use_timestamps) { + pack.timestamp = sample->time; + } DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); if (ctx.params.show_samples) { ddprof_print_sample(us->output, us->symbol_hdr, sample->period, kSumPos, diff --git a/test/ddprof_exporter-ut.cc b/test/ddprof_exporter-ut.cc index 440c5be13..d40903343 100644 --- a/test/ddprof_exporter-ut.cc +++ b/test/ddprof_exporter-ut.cc @@ -158,7 +158,7 @@ TEST(DDProfExporter, simple) { ctx.watchers.push_back(*ewatcher_from_str("sCPU")); res = pprof_create_profile(&pprofs, ctx); EXPECT_TRUE(IsDDResOK(res)); - res = pprof_aggregate(&mock_output, symbol_hdr, 1000, 1, &ctx.watchers[0], + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[0], kSumPos, &pprofs); EXPECT_TRUE(IsDDResOK(res)); } diff --git a/test/ddprof_pprof-ut.cc b/test/ddprof_pprof-ut.cc index 99a5a7787..799a08439 100644 --- a/test/ddprof_pprof-ut.cc +++ b/test/ddprof_pprof-ut.cc @@ -37,7 +37,7 @@ void test_pprof(DDProfPProf *pprofs) { ddog_prof_Profile *profile = &pprofs->_profile; struct ddog_prof_Profile_SerializeResult serialized_result = - ddog_prof_Profile_serialize(profile, nullptr, nullptr); + ddog_prof_Profile_serialize(profile, nullptr, nullptr, nullptr); ASSERT_EQ(serialized_result.tag, DDOG_PROF_PROFILE_SERIALIZE_RESULT_OK); @@ -74,7 +74,7 @@ TEST(DDProfPProf, aggregate) { DDRes res = pprof_create_profile(&pprof, ctx); EXPECT_TRUE(ctx.watchers[0].pprof_indices[kSumPos].pprof_index != -1); EXPECT_TRUE(ctx.watchers[0].pprof_indices[kSumPos].pprof_count_index != -1); - res = pprof_aggregate(&mock_output, symbol_hdr, 1000, 1, &ctx.watchers[0], + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[0], kSumPos, &pprof); EXPECT_TRUE(IsDDResOK(res)); @@ -113,7 +113,7 @@ TEST(DDProfPProf, just_live) { EXPECT_TRUE(ctx.watchers[1].pprof_indices[kLiveSumPos].pprof_index != -1); EXPECT_TRUE(ctx.watchers[1].pprof_indices[kLiveSumPos].pprof_count_index != -1); - res = pprof_aggregate(&mock_output, symbol_hdr, 1000, 1, &ctx.watchers[1], + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[1], kLiveSumPos, &pprof); EXPECT_TRUE(IsDDResOK(res)); test_pprof(&pprof); From 6408f299f860d3f301a067dc28c988de125bb18b Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 21:38:11 +0000 Subject: [PATCH 03/11] Style nags --- include/chrono_utils.hpp | 4 +- include/pprof/ddprof_pprof.hpp | 3 +- include/scope.hpp | 66 ++++++++++++++++----------------- src/ddprof_worker.cc | 13 +++---- src/exporter/ddprof_exporter.cc | 5 +-- src/pprof/ddprof_pprof.cc | 7 ++-- test/ddprof_exporter-ut.cc | 4 +- test/ddprof_pprof-ut.cc | 8 ++-- 8 files changed, 52 insertions(+), 58 deletions(-) diff --git a/include/chrono_utils.hpp b/include/chrono_utils.hpp index b93648a93..f408d13d9 100644 --- a/include/chrono_utils.hpp +++ b/include/chrono_utils.hpp @@ -13,7 +13,7 @@ struct is_duration> : std::true_type {}; template inline constexpr bool is_duration_v = is_duration::value; template - requires is_duration_v +requires is_duration_v constexpr timespec duration_to_timespec(Duration d) { auto nsecs = std::chrono::duration_cast(d); d -= nsecs; @@ -22,7 +22,7 @@ constexpr timespec duration_to_timespec(Duration d) { } template - requires is_duration_v +requires is_duration_v constexpr timeval duration_to_timeval(Duration d) { auto nsecs = std::chrono::duration_cast(d); d -= nsecs; diff --git a/include/pprof/ddprof_pprof.hpp b/include/pprof/ddprof_pprof.hpp index 95e856256..4fa7083c5 100644 --- a/include/pprof/ddprof_pprof.hpp +++ b/include/pprof/ddprof_pprof.hpp @@ -40,8 +40,7 @@ DDRes pprof_create_profile(DDProfPProf *pprof, DDProfContext &ctx); * @param pprof */ DDRes pprof_aggregate(const UnwindOutput *uw_output, - const SymbolHdr &symbol_hdr, - const DDProfValuePack &pack, + const SymbolHdr &symbol_hdr, const DDProfValuePack &pack, const PerfWatcher *watcher, EventAggregationModePos value_pos, DDProfPProf *pprof); diff --git a/include/scope.hpp b/include/scope.hpp index abdaad907..6b7c1575f 100644 --- a/include/scope.hpp +++ b/include/scope.hpp @@ -79,8 +79,8 @@ template class box { public: template - requires std::is_constructible_v - explicit box(TT &&t, GG &&guard) noexcept(noexcept(box((T &&)t))) + requires std::is_constructible_v + explicit box(TT &&t, GG &&guard) noexcept(noexcept(box((T &&) t))) : box(std::forward(t)) { guard.release(); } @@ -101,9 +101,8 @@ template class box { public: template - requires std::is_convertible_v - box(TT &&t, GG &&guard) noexcept(noexcept(static_cast((TT &&)t))) - : value(static_cast(t)) { + requires std::is_convertible_v box(TT &&t, GG &&guard) + noexcept(noexcept(static_cast((TT &&) t))) : value(static_cast(t)) { guard.release(); } T &get() const noexcept { return value.get(); } @@ -213,8 +212,8 @@ class [[nodiscard]] basic_scope_exit : Policy { public: template - requires _ctor_from::value - explicit basic_scope_exit(EFP &&ef) noexcept(_noexcept_ctor_from::value) + requires _ctor_from::value explicit basic_scope_exit(EFP &&ef) noexcept( + _noexcept_ctor_from::value) : exit_function(std::forward(ef), _make_failsafe(_noexcept_ctor_from{}, &ef)) {} basic_scope_exit(basic_scope_exit &&that) noexcept( @@ -259,14 +258,14 @@ template class unique_resource { public: // should be private template - requires std::is_constructible_v, RR, - detail::_empty_scope_exit> && - std::is_constructible_v, DD, - detail::_empty_scope_exit> - unique_resource(RR &&r, DD &&d, bool should_run) noexcept( - noexcept(detail::box(std::forward(r), detail::_empty_scope_exit{})) - && noexcept(detail::box(std::forward
(d), - detail::_empty_scope_exit{}))) + requires std::is_constructible_v, RR, + detail::_empty_scope_exit> && + std::is_constructible_v, DD, detail::_empty_scope_exit> + unique_resource(RR &&r, DD &&d, bool should_run) + noexcept(noexcept(detail::box(std::forward(r), + detail::_empty_scope_exit{})) + &&noexcept(detail::box(std::forward
(d), + detail::_empty_scope_exit{}))) : resource(std::forward(r), scope_exit([&] { if (should_run) d(r); @@ -296,22 +295,22 @@ template class unique_resource { unique_resource() = default; template - requires std::is_constructible_v, RR, - detail::_empty_scope_exit> && - std::is_constructible_v, DD, - detail::_empty_scope_exit> - unique_resource(RR &&r, DD &&d) noexcept( - noexcept(detail::box(std::forward(r), detail::_empty_scope_exit{})) - && noexcept(detail::box(std::forward
(d), - detail::_empty_scope_exit{}))) + requires std::is_constructible_v, RR, + detail::_empty_scope_exit> && + std::is_constructible_v, DD, detail::_empty_scope_exit> + unique_resource(RR &&r, DD &&d) + noexcept(noexcept(detail::box(std::forward(r), + detail::_empty_scope_exit{})) + &&noexcept(detail::box(std::forward
(d), + detail::_empty_scope_exit{}))) : resource(std::forward(r), scope_exit([&] { d(r); })), deleter(std::forward
(d), scope_exit([&, this] { d(get()); })), execute_on_destruction{true} {} unique_resource(unique_resource &&that) noexcept( noexcept(detail::box(that.resource.move(), detail::_empty_scope_exit{})) - && noexcept(detail::box(that.deleter.move(), - detail::_empty_scope_exit{}))) + &&noexcept(detail::box(that.deleter.move(), + detail::_empty_scope_exit{}))) : resource(that.resource.move(), detail::_empty_scope_exit{}), deleter(that.deleter.move(), scope_exit([&, this] { if (that.execute_on_destruction) @@ -322,8 +321,8 @@ template class unique_resource { std::exchange(that.execute_on_destruction, false)) {} unique_resource &operator=(unique_resource &&that) noexcept( - is_nothrow_delete_v && std::is_nothrow_move_assignable_v && - std::is_nothrow_move_assignable_v) { + is_nothrow_delete_v &&std::is_nothrow_move_assignable_v + &&std::is_nothrow_move_assignable_v) { static_assert( std::is_nothrow_move_assignable::value || std::is_copy_assignable::value, @@ -379,17 +378,16 @@ template class unique_resource { decltype(auto) get() const noexcept { return resource.get(); } decltype(auto) get_deleter() const noexcept { return deleter.get(); } template - requires std::is_pointer_v + requires std::is_pointer_v auto operator->() const noexcept //(noexcept(detail::for_noexcept_on_copy_construction(this_.get()))) -> decltype(get()) { return get(); } template - requires std::is_pointer_v && - (!std::is_void_v>) - std::add_lvalue_reference_t> - operator*() const noexcept { + requires std::is_pointer_v &&(!std::is_void_v>) + std::add_lvalue_reference_t> + operator*() const noexcept { return *get(); } @@ -405,8 +403,8 @@ unique_resource(R, D, bool) -> unique_resource; template [[nodiscard]] auto make_unique_resource_checked( R &&r, const S &invalid, - D &&d) noexcept(std::is_nothrow_constructible_v, R> && - std::is_nothrow_constructible_v, D>) + D &&d) noexcept(std::is_nothrow_constructible_v, R> + &&std::is_nothrow_constructible_v, D>) -> unique_resource, std::decay_t> { bool const mustrelease(r == invalid); unique_resource resource{std::forward(r), std::forward(d), diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index cf646a853..e96d5a118 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -237,14 +237,12 @@ DDRes aggregate_livealloc_stack( const LiveAllocation::PprofStacks::value_type &alloc_info, DDProfContext &ctx, const PerfWatcher *watcher, DDProfPProf *pprof, const SymbolHdr &symbol_hdr) { - DDProfValuePack pack{ + DDProfValuePack pack{ alloc_info.second._value, - static_cast(std::max(0, alloc_info.second._count)), - 0 - }; + static_cast(std::max(0, alloc_info.second._count)), 0}; - DDRES_CHECK_FWD( - pprof_aggregate(&alloc_info.first, symbol_hdr, pack, watcher, kLiveSumPos, pprof)); + DDRES_CHECK_FWD(pprof_aggregate(&alloc_info.first, symbol_hdr, pack, watcher, + kLiveSumPos, pprof)); if (ctx.params.show_samples) { ddprof_print_sample(alloc_info.first, symbol_hdr, alloc_info.second._value, kLiveSumPos, *watcher); @@ -420,7 +418,8 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, if (ctx.params.use_timestamps) { pack.timestamp = sample->time; } - DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); + DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, + watcher, kSumPos, pprof)); if (ctx.params.show_samples) { ddprof_print_sample(us->output, us->symbol_hdr, sample->period, kSumPos, *watcher); diff --git a/src/exporter/ddprof_exporter.cc b/src/exporter/ddprof_exporter.cc index d20d198b4..202408846 100644 --- a/src/exporter/ddprof_exporter.cc +++ b/src/exporter/ddprof_exporter.cc @@ -328,9 +328,8 @@ DDRes ddprof_exporter_export(ddog_prof_Profile *profile, ddog_prof_Exporter_Request_BuildResult res_request = ddog_prof_Exporter_Request_build(exporter->_exporter, start, end, ddog_prof_Exporter_Slice_File_empty(), - files, - &ffi_additional_tags, nullptr, nullptr, - k_timeout_ms); + files, &ffi_additional_tags, nullptr, + nullptr, k_timeout_ms); if (res_request.tag == DDOG_PROF_EXPORTER_REQUEST_BUILD_RESULT_OK) { ddog_prof_Exporter_Request *request = res_request.ok; diff --git a/src/pprof/ddprof_pprof.cc b/src/pprof/ddprof_pprof.cc index 1fd771107..2f18eea68 100644 --- a/src/pprof/ddprof_pprof.cc +++ b/src/pprof/ddprof_pprof.cc @@ -234,8 +234,8 @@ DDRes pprof_create_profile(DDProfPProf *pprof, DDProfContext &ctx) { pprof_values.get_num_sample_type_ids() > 0 ? &period : nullptr, nullptr); if (prof_res.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) { - ddog_Error_drop(&prof_res.err); - DDRES_RETURN_ERROR_LOG(DD_WHAT_PPROF, "Unable to create new profile"); + ddog_Error_drop(&prof_res.err); + DDRES_RETURN_ERROR_LOG(DD_WHAT_PPROF, "Unable to create new profile"); } pprof->_profile = prof_res.ok; @@ -266,8 +266,7 @@ DDRes pprof_free_profile(DDProfPProf *pprof) { // Assumption of API is that sample is valid in a single type DDRes pprof_aggregate(const UnwindOutput *uw_output, - const SymbolHdr &symbol_hdr, - const DDProfValuePack &pack, + const SymbolHdr &symbol_hdr, const DDProfValuePack &pack, const PerfWatcher *watcher, EventAggregationModePos value_pos, DDProfPProf *pprof) { diff --git a/test/ddprof_exporter-ut.cc b/test/ddprof_exporter-ut.cc index d40903343..411c1cb15 100644 --- a/test/ddprof_exporter-ut.cc +++ b/test/ddprof_exporter-ut.cc @@ -158,8 +158,8 @@ TEST(DDProfExporter, simple) { ctx.watchers.push_back(*ewatcher_from_str("sCPU")); res = pprof_create_profile(&pprofs, ctx); EXPECT_TRUE(IsDDResOK(res)); - res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[0], - kSumPos, &pprofs); + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, + &ctx.watchers[0], kSumPos, &pprofs); EXPECT_TRUE(IsDDResOK(res)); } { diff --git a/test/ddprof_pprof-ut.cc b/test/ddprof_pprof-ut.cc index 799a08439..60ec3b429 100644 --- a/test/ddprof_pprof-ut.cc +++ b/test/ddprof_pprof-ut.cc @@ -74,8 +74,8 @@ TEST(DDProfPProf, aggregate) { DDRes res = pprof_create_profile(&pprof, ctx); EXPECT_TRUE(ctx.watchers[0].pprof_indices[kSumPos].pprof_index != -1); EXPECT_TRUE(ctx.watchers[0].pprof_indices[kSumPos].pprof_count_index != -1); - res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[0], - kSumPos, &pprof); + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, + &ctx.watchers[0], kSumPos, &pprof); EXPECT_TRUE(IsDDResOK(res)); @@ -113,8 +113,8 @@ TEST(DDProfPProf, just_live) { EXPECT_TRUE(ctx.watchers[1].pprof_indices[kLiveSumPos].pprof_index != -1); EXPECT_TRUE(ctx.watchers[1].pprof_indices[kLiveSumPos].pprof_count_index != -1); - res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, &ctx.watchers[1], - kLiveSumPos, &pprof); + res = pprof_aggregate(&mock_output, symbol_hdr, {1000, 1, 0}, + &ctx.watchers[1], kLiveSumPos, &pprof); EXPECT_TRUE(IsDDResOK(res)); test_pprof(&pprof); res = pprof_free_profile(&pprof); From 4b5e6a19df8d565f07144a0bc4e3e7758c7a7b94 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Sat, 21 Oct 2023 00:30:20 +0200 Subject: [PATCH 04/11] Timeline - refactor option name Proposal to name the option timeline to be more consistent with the UI feature --- include/chrono_utils.hpp | 4 +-- include/ddprof_cli.hpp | 2 +- include/ddprof_context.hpp | 2 +- include/scope.hpp | 66 ++++++++++++++++++++------------------ src/ddprof_cli.cc | 7 ++-- src/ddprof_context_lib.cc | 2 +- src/ddprof_worker.cc | 6 ++-- 7 files changed, 44 insertions(+), 45 deletions(-) diff --git a/include/chrono_utils.hpp b/include/chrono_utils.hpp index f408d13d9..b93648a93 100644 --- a/include/chrono_utils.hpp +++ b/include/chrono_utils.hpp @@ -13,7 +13,7 @@ struct is_duration> : std::true_type {}; template inline constexpr bool is_duration_v = is_duration::value; template -requires is_duration_v + requires is_duration_v constexpr timespec duration_to_timespec(Duration d) { auto nsecs = std::chrono::duration_cast(d); d -= nsecs; @@ -22,7 +22,7 @@ constexpr timespec duration_to_timespec(Duration d) { } template -requires is_duration_v + requires is_duration_v constexpr timeval duration_to_timeval(Duration d) { auto nsecs = std::chrono::duration_cast(d); d -= nsecs; diff --git a/include/ddprof_cli.hpp b/include/ddprof_cli.hpp index bed4d4b9c..ac290526d 100644 --- a/include/ddprof_cli.hpp +++ b/include/ddprof_cli.hpp @@ -69,7 +69,7 @@ struct DDProfCLI { bool help_extended{false}; int socket{-1}; bool continue_exec{false}; - bool use_timestamps{false}; + bool timeline{false}; // args std::vector command_line; diff --git a/include/ddprof_context.hpp b/include/ddprof_context.hpp index dfb02b3f3..851e54f91 100644 --- a/include/ddprof_context.hpp +++ b/include/ddprof_context.hpp @@ -30,7 +30,7 @@ struct DDProfContext { int dd_profiling_fd{-1}; // opened file descriptor to our internal lib ddprof::UniqueFd sockfd; bool show_samples{false}; - bool use_timestamps{false}; + bool timeline{false}; cpu_set_t cpu_affinity{}; std::string switch_user; std::string internal_stats; diff --git a/include/scope.hpp b/include/scope.hpp index 6b7c1575f..abdaad907 100644 --- a/include/scope.hpp +++ b/include/scope.hpp @@ -79,8 +79,8 @@ template class box { public: template - requires std::is_constructible_v - explicit box(TT &&t, GG &&guard) noexcept(noexcept(box((T &&) t))) + requires std::is_constructible_v + explicit box(TT &&t, GG &&guard) noexcept(noexcept(box((T &&)t))) : box(std::forward(t)) { guard.release(); } @@ -101,8 +101,9 @@ template class box { public: template - requires std::is_convertible_v box(TT &&t, GG &&guard) - noexcept(noexcept(static_cast((TT &&) t))) : value(static_cast(t)) { + requires std::is_convertible_v + box(TT &&t, GG &&guard) noexcept(noexcept(static_cast((TT &&)t))) + : value(static_cast(t)) { guard.release(); } T &get() const noexcept { return value.get(); } @@ -212,8 +213,8 @@ class [[nodiscard]] basic_scope_exit : Policy { public: template - requires _ctor_from::value explicit basic_scope_exit(EFP &&ef) noexcept( - _noexcept_ctor_from::value) + requires _ctor_from::value + explicit basic_scope_exit(EFP &&ef) noexcept(_noexcept_ctor_from::value) : exit_function(std::forward(ef), _make_failsafe(_noexcept_ctor_from{}, &ef)) {} basic_scope_exit(basic_scope_exit &&that) noexcept( @@ -258,14 +259,14 @@ template class unique_resource { public: // should be private template - requires std::is_constructible_v, RR, - detail::_empty_scope_exit> && - std::is_constructible_v, DD, detail::_empty_scope_exit> - unique_resource(RR &&r, DD &&d, bool should_run) - noexcept(noexcept(detail::box(std::forward(r), - detail::_empty_scope_exit{})) - &&noexcept(detail::box(std::forward
(d), - detail::_empty_scope_exit{}))) + requires std::is_constructible_v, RR, + detail::_empty_scope_exit> && + std::is_constructible_v, DD, + detail::_empty_scope_exit> + unique_resource(RR &&r, DD &&d, bool should_run) noexcept( + noexcept(detail::box(std::forward(r), detail::_empty_scope_exit{})) + && noexcept(detail::box(std::forward
(d), + detail::_empty_scope_exit{}))) : resource(std::forward(r), scope_exit([&] { if (should_run) d(r); @@ -295,22 +296,22 @@ template class unique_resource { unique_resource() = default; template - requires std::is_constructible_v, RR, - detail::_empty_scope_exit> && - std::is_constructible_v, DD, detail::_empty_scope_exit> - unique_resource(RR &&r, DD &&d) - noexcept(noexcept(detail::box(std::forward(r), - detail::_empty_scope_exit{})) - &&noexcept(detail::box(std::forward
(d), - detail::_empty_scope_exit{}))) + requires std::is_constructible_v, RR, + detail::_empty_scope_exit> && + std::is_constructible_v, DD, + detail::_empty_scope_exit> + unique_resource(RR &&r, DD &&d) noexcept( + noexcept(detail::box(std::forward(r), detail::_empty_scope_exit{})) + && noexcept(detail::box(std::forward
(d), + detail::_empty_scope_exit{}))) : resource(std::forward(r), scope_exit([&] { d(r); })), deleter(std::forward
(d), scope_exit([&, this] { d(get()); })), execute_on_destruction{true} {} unique_resource(unique_resource &&that) noexcept( noexcept(detail::box(that.resource.move(), detail::_empty_scope_exit{})) - &&noexcept(detail::box(that.deleter.move(), - detail::_empty_scope_exit{}))) + && noexcept(detail::box(that.deleter.move(), + detail::_empty_scope_exit{}))) : resource(that.resource.move(), detail::_empty_scope_exit{}), deleter(that.deleter.move(), scope_exit([&, this] { if (that.execute_on_destruction) @@ -321,8 +322,8 @@ template class unique_resource { std::exchange(that.execute_on_destruction, false)) {} unique_resource &operator=(unique_resource &&that) noexcept( - is_nothrow_delete_v &&std::is_nothrow_move_assignable_v - &&std::is_nothrow_move_assignable_v) { + is_nothrow_delete_v && std::is_nothrow_move_assignable_v && + std::is_nothrow_move_assignable_v) { static_assert( std::is_nothrow_move_assignable::value || std::is_copy_assignable::value, @@ -378,16 +379,17 @@ template class unique_resource { decltype(auto) get() const noexcept { return resource.get(); } decltype(auto) get_deleter() const noexcept { return deleter.get(); } template - requires std::is_pointer_v + requires std::is_pointer_v auto operator->() const noexcept //(noexcept(detail::for_noexcept_on_copy_construction(this_.get()))) -> decltype(get()) { return get(); } template - requires std::is_pointer_v &&(!std::is_void_v>) - std::add_lvalue_reference_t> - operator*() const noexcept { + requires std::is_pointer_v && + (!std::is_void_v>) + std::add_lvalue_reference_t> + operator*() const noexcept { return *get(); } @@ -403,8 +405,8 @@ unique_resource(R, D, bool) -> unique_resource; template [[nodiscard]] auto make_unique_resource_checked( R &&r, const S &invalid, - D &&d) noexcept(std::is_nothrow_constructible_v, R> - &&std::is_nothrow_constructible_v, D>) + D &&d) noexcept(std::is_nothrow_constructible_v, R> && + std::is_nothrow_constructible_v, D>) -> unique_resource, std::decay_t> { bool const mustrelease(r == invalid); unique_resource resource{std::forward(r), std::forward(d), diff --git a/src/ddprof_cli.cc b/src/ddprof_cli.cc index 1e8b0e4a5..7b4ebcc86 100644 --- a/src/ddprof_cli.cc +++ b/src/ddprof_cli.cc @@ -233,8 +233,7 @@ int DDProfCLI::parse(int argc, const char *argv[]) { app.add_flag("--show_samples", show_samples, "Display captured samples as logs.\n") ->group("Debug options"); - app.add_flag("--use_timestamps", use_timestamps, - "Put timestamps on sampled events.\n") + app.add_flag("--timeline", timeline, "Put timestamps on sampled events.\n") ->group("Debug options"); app.add_flag("--version,-v", version, "Display the profiler's version.\n") ->group("Debug options"); @@ -455,8 +454,8 @@ void DDProfCLI::print() const { if (show_samples) { PRINT_NFO(" - show_samples: %s", show_samples ? "true" : "false"); } - if (use_timestamps) { - PRINT_NFO(" - use_timestamps: %s", use_timestamps ? "true" : "false"); + if (timeline) { + PRINT_NFO(" - timeline: %s", timeline ? "true" : "false"); } PRINT_NFO(" - fault_info: %s", fault_info ? "true" : "false"); diff --git a/src/ddprof_context_lib.cc b/src/ddprof_context_lib.cc index eb77c18f5..ba63ed246 100644 --- a/src/ddprof_context_lib.cc +++ b/src/ddprof_context_lib.cc @@ -74,7 +74,7 @@ void copy_cli_values(const DDProfCLI &ddprof_cli, DDProfContext &ctx) { } ctx.params.show_samples = ddprof_cli.show_samples; - ctx.params.use_timestamps = ddprof_cli.use_timestamps; + ctx.params.timeline = ddprof_cli.timeline; ctx.params.fault_info = ddprof_cli.fault_info; ctx.params.initial_loaded_libs_check_delay = ddprof_cli.initial_loaded_libs_check_delay; diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index e96d5a118..ab2909f0d 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -414,10 +414,8 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, // in lib mode we don't aggregate (protect to avoid link failures) int const i_export = ctx.worker_ctx.i_current_pprof; DDProfPProf *pprof = ctx.worker_ctx.pprof[i_export]; - DDProfValuePack pack{static_cast(sample_val), 1, 0}; - if (ctx.params.use_timestamps) { - pack.timestamp = sample->time; - } + DDProfValuePack pack{static_cast(sample_val), 1, + ctx.params.timeline ? sample->time : 0}; DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); if (ctx.params.show_samples) { From 39d7a5d5620e08dc2b12cf6422f19ed2d39c329b Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 23:22:13 +0000 Subject: [PATCH 05/11] Fix const correctness --- src/ddprof_worker.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index ab2909f0d..5380abe1a 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -74,7 +74,7 @@ DDRes report_lost_events(DDProfContext &ctx) { watcher->sample_frequency : watcher->sample_period; - int64_t value = period * nb_lost; + const int64_t value = period * nb_lost; LG_WRN("Reporting %lu lost samples (cumulated lost value: %lu) for " "watcher #%d", nb_lost, value, watcher_idx); @@ -237,7 +237,7 @@ DDRes aggregate_livealloc_stack( const LiveAllocation::PprofStacks::value_type &alloc_info, DDProfContext &ctx, const PerfWatcher *watcher, DDProfPProf *pprof, const SymbolHdr &symbol_hdr) { - DDProfValuePack pack{ + const DDProfValuePack pack{ alloc_info.second._value, static_cast(std::max(0, alloc_info.second._count)), 0}; @@ -414,7 +414,7 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, // in lib mode we don't aggregate (protect to avoid link failures) int const i_export = ctx.worker_ctx.i_current_pprof; DDProfPProf *pprof = ctx.worker_ctx.pprof[i_export]; - DDProfValuePack pack{static_cast(sample_val), 1, + const DDProfValuePack pack{static_cast(sample_val), 1, ctx.params.timeline ? sample->time : 0}; DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); From a4349423a2832c7c3695710bcafabd09cc9678a8 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 23:38:25 +0000 Subject: [PATCH 06/11] Slight change to CLI options --- src/ddprof_cli.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ddprof_cli.cc b/src/ddprof_cli.cc index 7b4ebcc86..dd2129615 100644 --- a/src/ddprof_cli.cc +++ b/src/ddprof_cli.cc @@ -163,6 +163,11 @@ int DDProfCLI::parse(int argc, const char *argv[]) { ->excludes(pid_opt) ->excludes(exec_option); + app.add_flag("--timeline", timeline, + "Enables Timeline view in the Datadog UI.\n" + "Works by adding timestmaps to certain events.") + ->group("Profiling settings"); + app.add_option( "--upload_period,-u", upload_period, "Upload period for profiles (in seconds).\n") @@ -233,8 +238,6 @@ int DDProfCLI::parse(int argc, const char *argv[]) { app.add_flag("--show_samples", show_samples, "Display captured samples as logs.\n") ->group("Debug options"); - app.add_flag("--timeline", timeline, "Put timestamps on sampled events.\n") - ->group("Debug options"); app.add_flag("--version,-v", version, "Display the profiler's version.\n") ->group("Debug options"); app.add_option("--enable", enable, From a1d9dafe909c8901b1d2126a2dbfba7b18064c8f Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 23:47:42 +0000 Subject: [PATCH 07/11] Add envvar and some other bikeshedding --- src/ddprof_cli.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ddprof_cli.cc b/src/ddprof_cli.cc index dd2129615..3f88557e2 100644 --- a/src/ddprof_cli.cc +++ b/src/ddprof_cli.cc @@ -163,10 +163,11 @@ int DDProfCLI::parse(int argc, const char *argv[]) { ->excludes(pid_opt) ->excludes(exec_option); - app.add_flag("--timeline", timeline, + app.add_flag("--timeline,-t", timeline, "Enables Timeline view in the Datadog UI.\n" "Works by adding timestmaps to certain events.") - ->group("Profiling settings"); + ->group("Profiling settings") + ->envname("DD_PROFILING_TIMELINE_ENABLE"); app.add_option( "--upload_period,-u", upload_period, From dd4095ef294abf20f8ef3623ad47b671a4514b2b Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 20 Oct 2023 23:52:03 +0000 Subject: [PATCH 08/11] Fix style nags --- src/ddprof_worker.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index 5380abe1a..d28e97321 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -415,7 +415,7 @@ DDRes ddprof_pr_sample(DDProfContext &ctx, perf_event_sample *sample, int const i_export = ctx.worker_ctx.i_current_pprof; DDProfPProf *pprof = ctx.worker_ctx.pprof[i_export]; const DDProfValuePack pack{static_cast(sample_val), 1, - ctx.params.timeline ? sample->time : 0}; + ctx.params.timeline ? sample->time : 0}; DDRES_CHECK_FWD(pprof_aggregate(&us->output, us->symbol_hdr, pack, watcher, kSumPos, pprof)); if (ctx.params.show_samples) { From 516f1b9bfabe1a703c1f2da6131458c92f3bfeca Mon Sep 17 00:00:00 2001 From: sanchda Date: Sat, 21 Oct 2023 00:27:17 +0000 Subject: [PATCH 09/11] Regen docs --- docs/Commands.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/Commands.md b/docs/Commands.md index bb1692134..9a0a38431 100644 --- a/docs/Commands.md +++ b/docs/Commands.md @@ -44,6 +44,9 @@ Profiling settings: -g,--global Excludes: command_line --pid Instrument all processes. Requires specific capabilities or a perf_event_paranoid value of less than 1. + -t,--timeline (Env:DD_PROFILING_TIMELINE_ENABLE) + Enables Timeline view in the Datadog UI. + Works by adding timestmaps to certain events. -u,--upload_period UINT [59] (Env:DD_PROFILING_UPLOAD_PERIOD) Upload period for profiles (in seconds). From bfd3684baf08b00724739447cafcbc94374351a1 Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Wed, 25 Oct 2023 14:57:31 -0500 Subject: [PATCH 10/11] Stash airplane progress lol --- include/perf_watcher_lookup.hpp | 201 +++++++++++++++++++++++ src/ddprof.cc | 23 ++- src/ddprof_worker.cc | 2 +- src/perf_mainloop.cc | 12 -- src/perf_watcher.cc | 6 +- src/pevent_lib.cc | 271 +++----------------------------- src/ringbuffer_utils.cc | 1 - 7 files changed, 239 insertions(+), 277 deletions(-) create mode 100644 include/perf_watcher_lookup.hpp diff --git a/include/perf_watcher_lookup.hpp b/include/perf_watcher_lookup.hpp new file mode 100644 index 000000000..31467d036 --- /dev/null +++ b/include/perf_watcher_lookup.hpp @@ -0,0 +1,201 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. This product includes software +// developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present +// Datadog, Inc. + +#pragma once + +#include "perf_ringbuffer.hpp" +#include "perf_watcher.hpp" +#include "pevent.hpp" + +#include +#include + +namespace ddprof { + +struct PEvent { + PerfWatcher *watcher; + int fd; // Underlying perf event FD for perf_events, otherwise an eventfd that + // signals data is available in ring buffer + int mapfd; // FD for ring buffer, same as `fd` for perf events + int cpu; // CPU id + int attr_idx; // matching perf_event_attr + size_t ring_buffer_size; // size of the ring buffer + RingBufferType ring_buffer_type; + RingBuffer rb; // metadata and buffers for processing perf ringbuffer +}; + +class PEventTable { +private: + PEventTable() {} + + // Lookups + std::unordered_map id_to_pevent; + std::unordered_map cpu_to_fd; + + // Stashed attrs + std::vector attrs; + +public: + PEventTable(const PEventTable&) = delete; + PEventTable& operator=(const PEventTable&) = delete; + + static PEventTable& get_instance() { + static PEventTable instance; + return instance; + } + + PEvent *pevent_from_id(uint64_t id) { + auto it = id_to_pevent.find(id); + return (it != id_to_pevent.end()) ? it->second : nullptr; + } + + bool open_custom_watcher(PerfWatcher &watcher, pid_t pid, PerfClockSource perf_clock_source) { + PEvent event = { + fd, + fd, + cpu, + attr_id, + buffer_size_order, + RingBufferType::kPerfRingBuffer, + false, + {} + }; + int const order = pevent_compute_min_mmap_order( + k_mpsc_buffer_size_shift, watcher->options.stack_sample_size, + k_min_number_samples_per_ring_buffer); + DDRES_CHECK_FWD(ring_buffer_create(order, RingBufferType::kMPSCRingBuffer, + true, &event)); + } + + bool open_perf_watcher(PerfWatcher &watcher, pid_t pid, PerfClockSource perf_clock_source) { + std::vector possible_attrs = all_perf_configs_from_watcher(&watcher, true, perf_clock_source); + + // We have a number of configurations and we need to try them on all CPUs. We prefer the earlier configurations, + // but can failover to the later ones. If a configuration fails, it should not be used again. Generally, either + // all or none of a configuration will work. If we fail midway through, we take what we can get. We return + // false if no configs succeed + for (int cpu = 0; cpu < num_cpu; ++cpu) { + auto it = possible_attrs.begin(); + while (it != possible_attrs.end()) { + int fd = perf_event_open(it, pid, cpu, -1, PERF_FLAG_FD_CLOEXEC); + if (fd == -1) { +# warning TODO add error here + it = possible_attrs.erase(it); // Don't retry this config + } + + // Get the ID + uint64_t sample_id = 0; + if (-1 == ioctl(fd, PERF_EVENT_IOC_ID, &sample_id)) { + // If I can't get the sample, then I can't use this event. + LG_WARN("Error getting perf sample ID\n"); + close(fd); + continue; + } + + // Store the attr + int attr_id = attrs.size(); + attrs.push_back(it); + + // Figure out which buffer size to use + static_bool log_once = true; + int const buffer_size_order = pevent_compute_min_mmap_order( + k_default_buffer_size_shift, stack_sample_size, + k_min_number_samples_per_ring_buffer); + if (buffer_size_order > k_default_buffer_size_shift && log_once) { +# warning TODO add more errors here + } + + // Make a PEvent now for the next part; this will get moved into storage if the next operations are successful, but it can't + // be moved yet because mapping may still fail + PEvent event = { + fd, + fd, + cpu, + attr_id, + buffer_size_order, + RingBufferType::kPerfRingBuffer, + false, + {} + }; + + // We have enough information to configure the ringbuffer. If this CPU + // already has a perf event on it, then multiplex the ringbuffer + auto fd_it = cpu_to_fd.find(cpu); + if (fd_it != fd_it.end()) { + // This CPU already has a perf_event ringbuffer, so just use that + auto cpu_fd = fd_it->second; + if (ioctl(event->mapfd, PERF_EVENT_IOC_SET_OUTPUT, cpu_fd)) { +# warning TODO add more errors + } + event->mapfd = fd_it->second; + } else { + // This CPU does not have a perf_event ringbuffer, so make one + pevent_mmap_event(event); + cpu_to_fd[cpu] = event->mapfd; + } + + // Successful, don't retry anymore! + id_to_pevent.emplace(sample_id, std::move(event)); + } // try to open + } // cpu + } + + bool open_watcher(PerfWatcher &watcher, pid_t pid, PerfClockSource perf_clock_source) { + if (watcher->type < kDDPROF_TYPE_CUSTOM) { + ... + } else { + } + + } + + DDRes enable_all() { + // Just before we enter the main loop, force the enablement of the perf + // contexts + for (const auto& [_, event] : id_to_pevent) { + (void)_; + if (event.watcher->type < kDDPROF_TYPE_CUSTOM) { +# warning TODO better error +// DDRES_CHECK_INT(ioctl(event.fd, PERF_EVENT_IOC_ENABLE), +// DD_WHAT_IOCTL, "Error ioctl fd=%d (idx#%zu)", +// event.fd, i); + } + } + return {}; + } + + DDRes cleanup() { + DDRes ret = ddres_init(); + + // Cleanup both, storing the error if one was generated + for (const auto& [_, event] : id_to_pevent) { + (void)_; + if (DDRes const ret_tmp = pevent_munmap_event(event), !IsDDResOK((ret_tmp))) { + ret = ret_tmp; + } + if (DDRes const ret_tmp = pevent_close_event(event), !IsDDResOK((ret_tmp))) { + ret = ret_tmp; + } + } + + // Now let's reset the storage + id_to_pevent.clear(); + cpu_to_fd.clear(); + attrs.clear(); + + return ret; + } + + void pollfd_setup(struct pollfd *pfd, int *pfd_len) { + // Setup poll() to watch perf_event file descriptors + for (const auto& [_, event] : id_to_pevent) { + // NOTE: if fd is negative, it will be ignored + pfd[i].fd = event.fd; + pfd[i].events = POLLIN | POLLERR | POLLHUP; + } + } + +}; + +} // namespace ddprof diff --git a/src/ddprof.cc b/src/ddprof.cc index e6a9c51cd..19dbd3899 100644 --- a/src/ddprof.cc +++ b/src/ddprof.cc @@ -68,17 +68,14 @@ void display_system_info() { } // namespace DDRes ddprof_setup(DDProfContext &ctx) { - PEventHdr *pevent_hdr = &ctx.worker_ctx.pevent_hdr; try { - pevent_init(pevent_hdr); - display_system_info(); // Open perf events and mmap events right now to start receiving events - // mmaps from perf fds will be lost after fork, that why we mmap them again - // in worker (but kernel only accounts for the pinned memory once). - DDRES_CHECK_FWD( - pevent_setup(ctx, ctx.params.pid, ctx.params.num_cpu, pevent_hdr)); + auto pe_table = PEventTable::get_instance(); + for (auto *watcher : ctx.watchers) { + pe_table.open_watcher(watcher, pid, num_cpu, ctx.perf_clock_source); + } // Setup signal handler if defined if (ctx.params.fault_info) { @@ -100,17 +97,17 @@ DDRes ddprof_setup(DDProfContext &ctx) { DDRES_CHECK_FWD(ddprof_stats_init()); - DDRES_CHECK_FWD(pevent_enable(pevent_hdr)); + DDRES_CHECK_FWD(pe_table.enable_all()); } CatchExcept2DDRes(); return {}; } -DDRes ddprof_teardown(DDProfContext &ctx) { - PEventHdr *pevent_hdr = &ctx.worker_ctx.pevent_hdr; - - if (IsDDResNotOK(pevent_cleanup(pevent_hdr))) { - LG_WRN("Error when calling pevent_cleanup."); +DDRes ddprof_teardown() { + + auto &pe_table = PEventTable::get_instance(); + if (IsDDResNotOK(pe_table.cleanup())) { + LG_WRN("Error when calling pe_table.cleanup."); } if (IsDDResNotOK(ddprof_stats_free())) { diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index d28e97321..95f641a25 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -337,7 +337,7 @@ DDRes worker_library_init(DDProfContext &ctx, // register the existing persistent storage for the state ctx.worker_ctx.persistent_worker_state = persistent_worker_state; - PEventHdr *pevent_hdr = &ctx.worker_ctx.pevent_hdr; + auto &pe_table = PETable::get_instance(); // If we're here, then we are a child spawned during the startup operation. // That means we need to iterate through the perf_event_open() handles and diff --git a/src/perf_mainloop.cc b/src/perf_mainloop.cc index ca72f63ea..5244ed7a6 100644 --- a/src/perf_mainloop.cc +++ b/src/perf_mainloop.cc @@ -109,18 +109,6 @@ DDRes spawn_workers(PersistentWorkerState *persistent_worker_state, return {}; } -void pollfd_setup(const PEventHdr *pevent_hdr, struct pollfd *pfd, - int *pfd_len) { - *pfd_len = pevent_hdr->size; - const PEvent *pes = pevent_hdr->pes; - // Setup poll() to watch perf_event file descriptors - for (int i = 0; i < *pfd_len; ++i) { - // NOTE: if fd is negative, it will be ignored - pfd[i].fd = pes[i].fd; - pfd[i].events = POLLIN | POLLERR | POLLHUP; - } -} - DDRes signalfd_setup(pollfd *pfd) { sigset_t mask; diff --git a/src/perf_watcher.cc b/src/perf_watcher.cc index d2eb6e82c..5128822df 100644 --- a/src/perf_watcher.cc +++ b/src/perf_watcher.cc @@ -13,9 +13,9 @@ namespace ddprof { -#define BASE_STYPES \ - (PERF_SAMPLE_STACK_USER | PERF_SAMPLE_REGS_USER | PERF_SAMPLE_TID | \ - PERF_SAMPLE_TIME | PERF_SAMPLE_PERIOD) +#define BASE_STYPES \ + (PERF_SAMPLE_IDENTIFIER | PERF_SAMPLE_STACK_USER | PERF_SAMPLE_REGS_USER | \ + PERF_SAMPLE_TID | PERF_SAMPLE_TIME | PERF_SAMPLE_PERIOD) uint64_t perf_event_default_sample_type() { return BASE_STYPES; } diff --git a/src/pevent_lib.cc b/src/pevent_lib.cc index 976d4ceed..fe2b19b81 100644 --- a/src/pevent_lib.cc +++ b/src/pevent_lib.cc @@ -27,18 +27,6 @@ namespace ddprof { namespace { -DDRes pevent_create(PEventHdr *pevent_hdr, int watcher_idx, - size_t *pevent_idx) { - if (pevent_hdr->size >= pevent_hdr->max_size) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFOPEN, - "Reached max number of watchers (%lu)", - pevent_hdr->max_size); - } - *pevent_idx = pevent_hdr->size++; - pevent_hdr->pes[*pevent_idx].watcher_pos = watcher_idx; - return {}; -} - void display_system_config() { int val; DDRes const res = sys_perf_event_paranoid(val); @@ -48,108 +36,8 @@ void display_system_config() { LG_WRN("Unable to access system configuration"); } } - -// set info for a perf_event_open type of buffer -void pevent_set_info(int fd, int attr_idx, PEvent &pevent, - uint32_t stack_sample_size) { - static bool log_once = true; - pevent.fd = fd; - pevent.mapfd = fd; - int const buffer_size_order = pevent_compute_min_mmap_order( - k_default_buffer_size_shift, stack_sample_size, - k_min_number_samples_per_ring_buffer); - if (buffer_size_order > k_default_buffer_size_shift && log_once) { - LG_NTC("Increasing size order of the ring buffer to %d (from %d)", - buffer_size_order, k_default_buffer_size_shift); - log_once = false; // avoid flooding for all CPUs - } - pevent.ring_buffer_size = perf_mmap_size(buffer_size_order); - pevent.custom_event = false; - pevent.ring_buffer_type = RingBufferType::kPerfRingBuffer; - pevent.attr_idx = attr_idx; -} - -DDRes pevent_register_cpu_0(const PerfWatcher *watcher, int watcher_idx, - pid_t pid, PerfClockSource perf_clock_source, - PEventHdr *pevent_hdr, size_t &pevent_idx) { - // register cpu 0 and find a working config - PEvent *pes = pevent_hdr->pes; - std::vector perf_event_data = - all_perf_configs_from_watcher(watcher, true, perf_clock_source); - DDRES_CHECK_FWD(pevent_create(pevent_hdr, watcher_idx, &pevent_idx)); - - // attempt with different configs - for (auto &attr : perf_event_data) { - // register cpu 0 - int const fd = perf_event_open(&attr, pid, 0, -1, PERF_FLAG_FD_CLOEXEC); - if (fd != -1) { - // Copy the successful config - pevent_hdr->attrs[pevent_hdr->nb_attrs] = attr; - pevent_set_info(fd, pevent_hdr->nb_attrs, pes[pevent_idx], - watcher->options.stack_sample_size); - ++pevent_hdr->nb_attrs; - assert(pevent_hdr->nb_attrs <= kMaxTypeWatcher); - break; - } - LG_NFO("Expected failure (we retry with different settings) " - "perf_event_open for watcher: %s - with attr.type=%s, " - "exclude_kernel=%d", - watcher->desc.c_str(), perf_type_str(attr.type), - static_cast(attr.exclude_kernel)); - } - // check if one of the configs was successful - if (pes[pevent_idx].attr_idx == -1) { - display_system_config(); - DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFOPEN, - "Error calling perf_event_open on watcher %d.0 (%s)", - watcher_idx, strerror(errno)); - } - - return {}; -} - -DDRes pevent_open_all_cpus(const PerfWatcher *watcher, int watcher_idx, - pid_t pid, int num_cpu, - PerfClockSource perf_clock_source, - PEventHdr *pevent_hdr) { - PEvent *pes = pevent_hdr->pes; - - size_t template_pevent_idx = -1; - DDRES_CHECK_FWD(pevent_register_cpu_0(watcher, watcher_idx, pid, - perf_clock_source, pevent_hdr, - template_pevent_idx)); - int const template_attr_idx = pes[template_pevent_idx].attr_idx; - perf_event_attr *attr = &pevent_hdr->attrs[template_attr_idx]; - - // used the fixed attr for the others - for (int cpu_idx = 1; cpu_idx < num_cpu; ++cpu_idx) { - size_t pevent_idx = -1; - DDRES_CHECK_FWD(pevent_create(pevent_hdr, watcher_idx, &pevent_idx)); - int const fd = - perf_event_open(attr, pid, cpu_idx, -1, PERF_FLAG_FD_CLOEXEC); - if (fd == -1) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFOPEN, - "Error calling perfopen on watcher %d.%d (%s)", - watcher_idx, cpu_idx, strerror(errno)); - } - pevent_set_info(fd, pes[template_pevent_idx].attr_idx, pes[pevent_idx], - watcher->options.stack_sample_size); - } - return {}; -} - } // namespace -void pevent_init(PEventHdr *pevent_hdr) { - memset(pevent_hdr, 0, sizeof(PEventHdr)); - pevent_hdr->max_size = k_max_nb_perf_event_open; - for (size_t k = 0; k < pevent_hdr->max_size; ++k) { - pevent_hdr->pes[k].fd = -1; - pevent_hdr->pes[k].mapfd = -1; - pevent_hdr->pes[k].attr_idx = -1; - } -} - int pevent_compute_min_mmap_order(int min_buffer_size_order, uint32_t stack_sample_size, unsigned min_number_samples) { @@ -164,103 +52,40 @@ int pevent_compute_min_mmap_order(int min_buffer_size_order, return ret_order; } -DDRes pevent_open(DDProfContext &ctx, pid_t pid, int num_cpu, - PEventHdr *pevent_hdr) { - assert(pevent_hdr->size == 0); // check for previous init - for (unsigned long watcher_idx = 0; watcher_idx < ctx.watchers.size(); - ++watcher_idx) { - PerfWatcher *watcher = &ctx.watchers[watcher_idx]; - if (watcher->type < kDDPROF_TYPE_CUSTOM) { - DDRES_CHECK_FWD(pevent_open_all_cpus(watcher, watcher_idx, pid, num_cpu, - ctx.perf_clock_source, pevent_hdr)); - } else { - // custom event, eg.allocation profiling - size_t pevent_idx = 0; - DDRES_CHECK_FWD(pevent_create(pevent_hdr, watcher_idx, &pevent_idx)); - int const order = pevent_compute_min_mmap_order( - k_mpsc_buffer_size_shift, watcher->options.stack_sample_size, - k_min_number_samples_per_ring_buffer); - DDRES_CHECK_FWD(ring_buffer_create(order, RingBufferType::kMPSCRingBuffer, - true, &pevent_hdr->pes[pevent_idx])); - } - } - return {}; -} - DDRes pevent_mmap_event(PEvent *event) { - if (event->mapfd != -1) { - void *region = perfown_sz(event->mapfd, event->ring_buffer_size); - if (!region) { - DDRES_RETURN_ERROR_LOG( - DD_WHAT_PERFMMAP, - "Could not mmap memory for watcher #%d: %s. " - "Please increase kernel limits on pinned memory (ulimit -l). " - "OR associate the IPC_LOCK capability to this process.", - event->watcher_pos, strerror(errno)); - } - if (!rb_init(&event->rb, region, event->ring_buffer_size, - event->ring_buffer_type)) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFMMAP, - "Could not initialize ring buffer for watcher #%d", - event->watcher_pos); - } - } - return {}; -} + if (event->mapfd == -1) + return {}; -DDRes pevent_mmap(PEventHdr *pevent_hdr, bool use_override) { - // Switch user if needed (when root switch to nobody user) - // Pinned memory is accounted by the kernel by (real) uid across containers - // (uid 1000 in the host and in containers will share the same count). - // Sometimes root allowance (when no CAP_IPC_LOCK/CAP_SYS_ADMIN in a - // container) is already exhausted, hence we switch to a different user. UIDInfo info; - if (use_override) { - /* perf_event_mlock_kb is accounted per real user id */ + void *region; + if (!(region = perfown_sz(event->mapfd, event->ring_buffer_size))) { + // Switch user if needed (when root switch to nobody user) + // Pinned memory is accounted by the kernel by (real) uid across containers + // (uid 1000 in the host and in containers will share the same count). + // Sometimes root allowance (when no CAP_IPC_LOCK/CAP_SYS_ADMIN in a + // container) is already exhausted, hence we switch to a different user. DDRES_CHECK_FWD(user_override_to_nobody_if_root(&info)); - } - defer { - if (use_override) { - user_override(info.uid, info.gid); - } - }; - - auto defer_munmap = make_defer([&] { pevent_munmap(pevent_hdr); }); - - PEvent *pes = pevent_hdr->pes; - for (size_t k = 0; k < pevent_hdr->size; ++k) { - DDRES_CHECK_FWD(pevent_mmap_event(&pes[k])); + if (!(region = perfown_sz(event->mapfd, event->ring_buffer_size))) { +# warning TODO return some kind of error here? +// DDRES_RETURN_ERROR_LOG( +// DD_WHAT_PERFMMAP, +// "Could not mmap memory for watcher #%d: %s. " +// "Please increase kernel limits on pinned memory (ulimit -l). " +// "OR associate the IPC_LOCK capability to this process.", +// event->watcher_pos, strerror(errno)); +// } } - defer_munmap.release(); - - return {}; -} - -DDRes pevent_setup(DDProfContext &ctx, pid_t pid, int num_cpu, - PEventHdr *pevent_hdr) { - DDRES_CHECK_FWD(pevent_open(ctx, pid, num_cpu, pevent_hdr)); - if (!IsDDResOK(pevent_mmap(pevent_hdr, true))) { - LG_NTC("Retrying attachment without user override"); - DDRES_CHECK_FWD(pevent_mmap(pevent_hdr, false)); + if (!rb_init(&event->rb, region, event->ring_buffer_size, +// event->ring_buffer_type)) { +# warning TODO add even more errors here +// DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFMMAP, +// "Could not initialize ring buffer for watcher #%d", +// event->watcher_pos); } - - return {}; } -DDRes pevent_enable(PEventHdr *pevent_hdr) { - // Just before we enter the main loop, force the enablement of the perf - // contexts - for (size_t i = 0; i < pevent_hdr->size; ++i) { - if (!pevent_hdr->pes[i].custom_event) { - DDRES_CHECK_INT(ioctl(pevent_hdr->pes[i].fd, PERF_EVENT_IOC_ENABLE), - DD_WHAT_IOCTL, "Error ioctl fd=%d (idx#%zu)", - pevent_hdr->pes[i].fd, i); - } - } - return {}; -} DDRes pevent_munmap_event(PEvent *event) { if (event->rb.base) { @@ -276,20 +101,6 @@ DDRes pevent_munmap_event(PEvent *event) { } /// Clean the mmap buffer -DDRes pevent_munmap(PEventHdr *pevent_hdr) { - PEvent *pes = pevent_hdr->pes; - DDRes res{}; - - for (size_t k = 0; k < pevent_hdr->size; ++k) { - DDRes const local_res = pevent_munmap_event(&pes[k]); - if (!IsDDResOK(local_res)) { - res = local_res; - } - } - - return res; -} - DDRes pevent_close_event(PEvent *event) { if (event->fd != -1) { if (close(event->fd) == -1) { @@ -309,38 +120,4 @@ DDRes pevent_close_event(PEvent *event) { return {}; } -DDRes pevent_close(PEventHdr *pevent_hdr) { - PEvent *pes = pevent_hdr->pes; - DDRes res{}; - for (size_t k = 0; k < pevent_hdr->size; ++k) { - DDRes const local_res = pevent_close_event(&pes[k]); - if (!IsDDResOK(local_res)) { - res = local_res; - } - } - pevent_hdr->size = 0; - return res; -} - -bool pevent_include_kernel_events(const PEventHdr *pevent_hdr) { - for (size_t i = 0; i < pevent_hdr->nb_attrs; ++i) { - if (pevent_hdr->attrs[i].exclude_kernel == 0) { - return true; - } - } - return false; -} - -DDRes pevent_cleanup(PEventHdr *pevent_hdr) { - DDRes ret = ddres_init(); - - // Cleanup both, storing the error if one was generated - if (DDRes const ret_tmp = pevent_munmap(pevent_hdr); !IsDDResOK((ret_tmp))) { - ret = ret_tmp; - } - if (DDRes const ret_tmp = pevent_close(pevent_hdr); !IsDDResOK((ret_tmp))) { - ret = ret_tmp; - } - return ret; -} } // namespace ddprof diff --git a/src/ringbuffer_utils.cc b/src/ringbuffer_utils.cc index 15f32f22f..fb2f7cc2b 100644 --- a/src/ringbuffer_utils.cc +++ b/src/ringbuffer_utils.cc @@ -72,7 +72,6 @@ DDRes ring_buffer_create(size_t buffer_size_page_order, "Error calling evenfd on watcher %d (%s)", pevent->watcher_pos, strerror(errno)); } - pevent->custom_event = custom_event; pevent->ring_buffer_type = ring_buffer_type; pevent->ring_buffer_size = buffer_size; From bb95946862c6831a0e53e22f585bb791afc8ca37 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 25 Oct 2023 20:05:59 +0000 Subject: [PATCH 11/11] Fix broken comments --- src/pevent_lib.cc | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/pevent_lib.cc b/src/pevent_lib.cc index fe2b19b81..e693aafa8 100644 --- a/src/pevent_lib.cc +++ b/src/pevent_lib.cc @@ -67,23 +67,20 @@ DDRes pevent_mmap_event(PEvent *event) { DDRES_CHECK_FWD(user_override_to_nobody_if_root(&info)); if (!(region = perfown_sz(event->mapfd, event->ring_buffer_size))) { -# warning TODO return some kind of error here? -// DDRES_RETURN_ERROR_LOG( -// DD_WHAT_PERFMMAP, -// "Could not mmap memory for watcher #%d: %s. " -// "Please increase kernel limits on pinned memory (ulimit -l). " -// "OR associate the IPC_LOCK capability to this process.", -// event->watcher_pos, strerror(errno)); -// } + DDRES_RETURN_ERROR_LOG( + DD_WHAT_PERFMMAP, + "Could not mmap memory for watcher" + "Please increase kernel limits on pinned memory (ulimit -l). " + "OR associate the IPC_LOCK capability to this process."); + } } if (!rb_init(&event->rb, region, event->ring_buffer_size, -// event->ring_buffer_type)) { -# warning TODO add even more errors here -// DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFMMAP, -// "Could not initialize ring buffer for watcher #%d", -// event->watcher_pos); + event->ring_buffer_type)) { + DDRES_RETURN_ERROR_LOG(DD_WHAT_PERFMMAP, + "Could not initialize ring buffer for watcher"); } + return {}; }