From 2d03985d8e658f779a8897e8ede8b55c81d76e84 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Tue, 18 Apr 2023 11:30:38 +0200 Subject: [PATCH 1/2] Minor refactoring - Extract presets - As the preset logics grows, I wanted to extract it from the ddprof_context init - Minor renaming / namespacing refactoring --- include/ddprof_context_lib.hpp | 8 ++-- include/perf_mainloop.hpp | 2 + include/presets.hpp | 23 +++++++++++ src/ddprof.cc | 2 +- src/ddprof_context_lib.cc | 70 ++++------------------------------ src/exe/main.cc | 19 ++++----- src/perf_mainloop.cc | 5 ++- src/presets.cc | 69 +++++++++++++++++++++++++++++++++ test/CMakeLists.txt | 2 + test/ddprof_input-ut.cc | 42 ++++++++++---------- 10 files changed, 145 insertions(+), 97 deletions(-) create mode 100644 include/presets.hpp create mode 100644 src/presets.cc diff --git a/include/ddprof_context_lib.hpp b/include/ddprof_context_lib.hpp index af352f5e3..e864ec201 100644 --- a/include/ddprof_context_lib.hpp +++ b/include/ddprof_context_lib.hpp @@ -10,8 +10,10 @@ typedef struct DDProfInput DDProfInput; typedef struct DDProfContext DDProfContext; typedef struct PerfWatcher PerfWatcher; +namespace ddprof { /***************************** Context Management *****************************/ -DDRes ddprof_context_set(DDProfInput *input, DDProfContext *); -void ddprof_context_free(DDProfContext *); +DDRes context_set(DDProfInput *input, DDProfContext *); +void context_free(DDProfContext *); -int ddprof_context_allocation_profiling_watcher_idx(const DDProfContext *ctx); +int context_allocation_profiling_watcher_idx(const DDProfContext *ctx); +} // namespace ddprof diff --git a/include/perf_mainloop.hpp b/include/perf_mainloop.hpp index 4eb3385b2..77bd79221 100644 --- a/include/perf_mainloop.hpp +++ b/include/perf_mainloop.hpp @@ -8,6 +8,7 @@ #include "ddprof_context.hpp" #include "worker_attr.hpp" +namespace ddprof { /** * Continuously poll for new events and process them accordingly * @@ -23,3 +24,4 @@ DDRes main_loop(const WorkerAttr *, DDProfContext *); // Same as main loop without any forks void main_loop_lib(const WorkerAttr *attr, DDProfContext *ctx); +} // namespace ddprof diff --git a/include/presets.hpp b/include/presets.hpp new file mode 100644 index 000000000..1a685fb77 --- /dev/null +++ b/include/presets.hpp @@ -0,0 +1,23 @@ +// 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 "ddprof_context.hpp" +#include "ddres_def.hpp" +#include + +namespace ddprof { + +struct Preset { + static constexpr size_t k_max_events = 10; + const char *name; + const char *events[k_max_events]; +}; + +DDRes add_preset(DDProfContext *ctx, const char *preset, + bool pid_or_global_mode); + +} // namespace ddprof diff --git a/src/ddprof.cc b/src/ddprof.cc index fe5b704a8..074186ad5 100644 --- a/src/ddprof.cc +++ b/src/ddprof.cc @@ -133,5 +133,5 @@ DDRes ddprof_start_profiler(DDProfContext *ctx) { // Enter the main loop -- this will not return unless there is an error. LG_NFO("Entering main loop"); - return main_loop(&perf_funs, ctx); + return ddprof::main_loop(&perf_funs, ctx); } diff --git a/src/ddprof_context_lib.cc b/src/ddprof_context_lib.cc index 37b64c335..13e3b670f 100644 --- a/src/ddprof_context_lib.cc +++ b/src/ddprof_context_lib.cc @@ -11,15 +11,15 @@ #include "ddprof_input.hpp" #include "logger.hpp" #include "logger_setup.hpp" +#include "presets.hpp" #include "span.hpp" #include #include -#include #include -#include #include +namespace ddprof { static const PerfWatcher * find_duplicate_event(ddprof::span watchers) { bool seen[DDPROF_PWE_LENGTH] = {}; @@ -42,66 +42,8 @@ static void order_watchers(ddprof::span watchers) { }); } -struct Preset { - static constexpr size_t k_max_events = 10; - const char *name; - const char *events[k_max_events]; -}; - -DDRes add_preset(DDProfContext *ctx, const char *preset, - bool pid_or_global_mode) { - using namespace std::literals; - static Preset presets[] = { - {"default", {"sCPU", "sALLOC"}}, - {"default-pid", {"sCPU"}}, - {"cpu_only", {"sCPU"}}, - {"alloc_only", {"sALLOC"}}, - {"cpu_live_heap", {"sCPU", "sALLOC mode=l"}}, - }; - - if (preset == "default"sv && pid_or_global_mode) { - preset = "default-pid"; - } - - ddprof::span presets_span{presets}; - std::string_view preset_sv{preset}; - - auto it = std::find_if(presets_span.begin(), presets_span.end(), - [&preset_sv](auto &e) { return e.name == preset_sv; }); - if (it == presets_span.end()) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, "Unknown preset (%s)", - preset); - } - - for (const char *event : it->events) { - if (event == nullptr) { - break; - } - if (ctx->num_watchers == MAX_TYPE_WATCHER) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, "Too many input events"); - } - PerfWatcher *watcher = &ctx->watchers[ctx->num_watchers]; - if (!watcher_from_str(event, watcher)) { - DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, - "Invalid event/tracepoint (%s)", event); - } - ddprof::span watchers{ctx->watchers, - static_cast(ctx->num_watchers)}; - - // ignore event if it was already present in watchers - if (watcher->ddprof_event_type == DDPROF_PWE_TRACEPOINT || - std::find_if(watchers.begin(), watchers.end(), [&watcher](auto &w) { - return w.ddprof_event_type == watcher->ddprof_event_type; - }) == watchers.end()) { - ++ctx->num_watchers; - } - } - - return {}; -} - /**************************** Argument Processor ***************************/ -DDRes ddprof_context_set(DDProfInput *input, DDProfContext *ctx) { +DDRes context_set(DDProfInput *input, DDProfContext *ctx) { *ctx = {}; setup_logger(input->log_mode, input->log_level); @@ -290,7 +232,7 @@ DDRes ddprof_context_set(DDProfInput *input, DDProfContext *ctx) { return ddres_init(); } -void ddprof_context_free(DDProfContext *ctx) { +void context_free(DDProfContext *ctx) { if (ctx->initialized) { exporter_input_free(&ctx->exp_input); free((char *)ctx->params.internal_stats); @@ -305,7 +247,7 @@ void ddprof_context_free(DDProfContext *ctx) { LOG_close(); } -int ddprof_context_allocation_profiling_watcher_idx(const DDProfContext *ctx) { +int context_allocation_profiling_watcher_idx(const DDProfContext *ctx) { ddprof::span watchers{ctx->watchers, static_cast(ctx->num_watchers)}; auto it = std::find_if(watchers.begin(), watchers.end(), [](const auto &watcher) { @@ -318,3 +260,5 @@ int ddprof_context_allocation_profiling_watcher_idx(const DDProfContext *ctx) { } return -1; } + +} // namespace ddprof diff --git a/src/exe/main.cc b/src/exe/main.cc index 3f827c76b..840dd3617 100644 --- a/src/exe/main.cc +++ b/src/exe/main.cc @@ -172,13 +172,13 @@ static InputResult parse_input(int *argc, char ***argv, DDProfContext *ctx) { return IsDDResOK(res) ? InputResult::kStop : InputResult::kError; } - // logger can be closed (as it is opened in ddprof_context_set) + // logger can be closed (as it is opened in context_set) LOG_close(); // cmdline args have been processed. Set the ctx - if (IsDDResNotOK(ddprof_context_set(&input, ctx))) { + if (IsDDResNotOK(ddprof::context_set(&input, ctx))) { LG_ERR("Error setting up profiling context, exiting"); - ddprof_context_free(ctx); + ddprof::context_free(ctx); return InputResult::kError; } // Adjust input parameters for execvp() (we do this even if unnecessary) @@ -200,7 +200,7 @@ static InputResult parse_input(int *argc, char ***argv, DDProfContext *ctx) { return InputResult::kError; } - if (ddprof_context_allocation_profiling_watcher_idx(ctx) != -1 && + if (ddprof::context_allocation_profiling_watcher_idx(ctx) != -1 && ctx->params.pid && ctx->params.sockfd == -1) { LG_ERR("Memory allocation profiling is not supported in PID / global mode"); return InputResult::kError; @@ -210,7 +210,7 @@ static InputResult parse_input(int *argc, char ***argv, DDProfContext *ctx) { } static int start_profiler_internal(DDProfContext *ctx, bool &is_profiler) { - auto defer_context_free = make_defer([ctx] { ddprof_context_free(ctx); }); + auto defer_context_free = make_defer([ctx] { ddprof::context_free(ctx); }); is_profiler = false; @@ -229,7 +229,7 @@ static int start_profiler_internal(DDProfContext *ctx, bool &is_profiler) { // Determine if library should be injected into target process // (ie. only if allocation profiling is active) bool allocation_profiling_started_from_wrapper = - ddprof_context_allocation_profiling_watcher_idx(ctx) != -1; + ddprof::context_allocation_profiling_watcher_idx(ctx) != -1; enum { kParentIdx, kChildIdx }; int sockfds[2] = {-1, -1}; @@ -259,7 +259,8 @@ static int start_profiler_internal(DDProfContext *ctx, bool &is_profiler) { } ctx->params.pid = getpid(); - auto daemonize_res = ddprof::daemonize([ctx] { ddprof_context_free(ctx); }); + auto daemonize_res = + ddprof::daemonize([ctx] { ddprof::context_free(ctx); }); if (daemonize_res.temp_pid == -1) { return -1; @@ -339,7 +340,7 @@ static int start_profiler_internal(DDProfContext *ctx, bool &is_profiler) { reply.pid = getpid(); int alloc_watcher_idx = - ddprof_context_allocation_profiling_watcher_idx(ctx); + ddprof::context_allocation_profiling_watcher_idx(ctx); if (alloc_watcher_idx != -1) { ddprof::span pevents{ctx->worker_ctx.pevent_hdr.pes, ctx->worker_ctx.pevent_hdr.size}; @@ -436,7 +437,7 @@ int main(int argc, char *argv[]) { } { - defer { ddprof_context_free(&ctx); }; + defer { ddprof::context_free(&ctx); }; /****************************************************************************\ | Run the Profiler | \****************************************************************************/ diff --git a/src/perf_mainloop.cc b/src/perf_mainloop.cc index 0a6ac216a..6bcacbbe7 100644 --- a/src/perf_mainloop.cc +++ b/src/perf_mainloop.cc @@ -30,6 +30,7 @@ #include #include +namespace ddprof { static pid_t g_child_pid = 0; static bool g_termination_requested = false; @@ -321,7 +322,7 @@ DDRes main_loop(const WorkerAttr *attr, DDProfContext *ctx) { // Ensure worker does not return, // because we don't want to free resources (perf_event fds,...) that are // shared between processes. Only free the context. - ddprof_context_free(ctx); + context_free(ctx); exit(0); } return {}; @@ -335,3 +336,5 @@ void main_loop_lib(const WorkerAttr *attr, DDProfContext *ctx) { LG_NFO("Request to exit"); } } + +} // namespace ddprof diff --git a/src/presets.cc b/src/presets.cc new file mode 100644 index 000000000..174c2fe46 --- /dev/null +++ b/src/presets.cc @@ -0,0 +1,69 @@ +// 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. + +#include "presets.hpp" + +#include "ddres.hpp" + +#include "ddprof_cmdline.hpp" +#include "span.hpp" + +#include +#include + +namespace ddprof { + +DDRes add_preset(DDProfContext *ctx, const char *preset, + bool pid_or_global_mode) { + using namespace std::literals; + static Preset presets[] = { + {"default", {"sCPU", "sALLOC"}}, + {"default-pid", {"sCPU"}}, + {"cpu_only", {"sCPU"}}, + {"alloc_only", {"sALLOC"}}, + {"cpu_live_heap", {"sCPU", "sALLOC mode=l"}}, + }; + + if (preset == "default"sv && pid_or_global_mode) { + preset = "default-pid"; + } + + ddprof::span presets_span{presets}; + std::string_view preset_sv{preset}; + + auto it = std::find_if(presets_span.begin(), presets_span.end(), + [&preset_sv](auto &e) { return e.name == preset_sv; }); + if (it == presets_span.end()) { + DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, "Unknown preset (%s)", + preset); + } + + for (const char *event : it->events) { + if (event == nullptr) { + break; + } + if (ctx->num_watchers == MAX_TYPE_WATCHER) { + DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, "Too many input events"); + } + PerfWatcher *watcher = &ctx->watchers[ctx->num_watchers]; + if (!watcher_from_str(event, watcher)) { + DDRES_RETURN_ERROR_LOG(DD_WHAT_INPUT_PROCESS, + "Invalid event/tracepoint (%s)", event); + } + ddprof::span watchers{ctx->watchers, + static_cast(ctx->num_watchers)}; + + // ignore event if it was already present in watchers + if (watcher->ddprof_event_type == DDPROF_PWE_TRACEPOINT || + std::find_if(watchers.begin(), watchers.end(), [&watcher](auto &w) { + return w.ddprof_event_type == watcher->ddprof_event_type; + }) == watchers.end()) { + ++ctx->num_watchers; + } + } + + return {}; +} +} // namespace ddprof diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4b35fe2f5..e8105decf 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -131,7 +131,9 @@ add_unit_test( ../src/ddprof_cpumask.cc ../src/logger_setup.cc ../src/perf_watcher.cc + ../src/presets.cc ../src/tracepoint_config.cc + ../src/version.cc ddprof_input-ut.cc LIBRARIES DDProf::Parser DEFINITIONS MYNAME="ddprof_input-ut") diff --git a/test/ddprof_input-ut.cc b/test/ddprof_input-ut.cc index 0b6f6637e..13d3751a8 100644 --- a/test/ddprof_input-ut.cc +++ b/test/ddprof_input-ut.cc @@ -20,6 +20,7 @@ using namespace std::literals; +namespace ddprof { class InputTest : public ::testing::Test { protected: void SetUp() override {} @@ -211,11 +212,11 @@ TEST_F(InputTest, duplicate_events) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_FALSE(IsDDResOK(res)); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Duplicate events (except tracepoints) are disallowed @@ -230,11 +231,11 @@ TEST_F(InputTest, duplicate_events) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_FALSE(IsDDResOK(res)); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } } @@ -252,7 +253,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); ddprof::span watchers{ctx.watchers, static_cast(ctx.num_watchers)}; @@ -271,7 +272,7 @@ TEST_F(InputTest, presets) { watchers.end()); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { @@ -286,14 +287,14 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 1); EXPECT_EQ(ctx.watchers[0].ddprof_event_type, DDPROF_PWE_sCPU); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Check cpu_only preset @@ -307,7 +308,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); ddprof::span watchers{ctx.watchers, static_cast(ctx.num_watchers)}; @@ -316,7 +317,7 @@ TEST_F(InputTest, presets) { EXPECT_EQ(ctx.watchers[0].ddprof_event_type, DDPROF_PWE_sCPU); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Check alloc_only preset @@ -331,7 +332,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 2); @@ -339,7 +340,7 @@ TEST_F(InputTest, presets) { EXPECT_EQ(ctx.watchers[0].ddprof_event_type, DDPROF_PWE_sDUM); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Check manual setting of live allocation @@ -353,7 +354,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 2); @@ -363,7 +364,7 @@ TEST_F(InputTest, presets) { log_watcher(&ctx.watchers[1], 1); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Check cpu_live_heap preset @@ -378,7 +379,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 2); @@ -388,7 +389,7 @@ TEST_F(InputTest, presets) { EXPECT_EQ(ctx.watchers[0].output_mode, EventConfMode::kCallgraph); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // Default preset should not be loaded if an event is given in input @@ -402,14 +403,14 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 1); EXPECT_EQ(ctx.watchers[0].ddprof_event_type, DDPROF_PWE_sCPU); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } { // If preset is explicit given in input, then another event with the same @@ -425,7 +426,7 @@ TEST_F(InputTest, presets) { EXPECT_TRUE(contine_exec); DDProfContext ctx; - res = ddprof_context_set(&input, &ctx); + res = context_set(&input, &ctx); EXPECT_TRUE(IsDDResOK(res)); EXPECT_EQ(ctx.num_watchers, 2); @@ -434,6 +435,7 @@ TEST_F(InputTest, presets) { EXPECT_EQ(ctx.watchers[1].ddprof_event_type, DDPROF_PWE_sALLOC); ddprof_input_free(&input); - ddprof_context_free(&ctx); + context_free(&ctx); } } +} // namespace ddprof From 457aef5f74aabb384708ab40a08b4f0449f6d1d8 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Wed, 19 Apr 2023 15:19:14 +0200 Subject: [PATCH 2/2] Minor ddprof_input-ut fix Revert to the usage of a mock for the version APIs --- test/CMakeLists.txt | 1 - test/ddprof_input-ut.cc | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e8105decf..034477bdc 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -133,7 +133,6 @@ add_unit_test( ../src/perf_watcher.cc ../src/presets.cc ../src/tracepoint_config.cc - ../src/version.cc ddprof_input-ut.cc LIBRARIES DDProf::Parser DEFINITIONS MYNAME="ddprof_input-ut") diff --git a/test/ddprof_input-ut.cc b/test/ddprof_input-ut.cc index 13d3751a8..a11be06a4 100644 --- a/test/ddprof_input-ut.cc +++ b/test/ddprof_input-ut.cc @@ -20,6 +20,11 @@ using namespace std::literals; +// mocks +bool s_version_called = false; +void print_version() { s_version_called = true; } +string_view str_version() { return STRING_VIEW_LITERAL("1.2.3"); } + namespace ddprof { class InputTest : public ::testing::Test { protected: @@ -28,10 +33,6 @@ class InputTest : public ::testing::Test { LogHandle _handle; }; -bool s_version_called = false; -void print_version() { s_version_called = true; } -string_view str_version() { return STRING_VIEW_LITERAL("1.2.3"); } - TEST_F(InputTest, watcher_from_str) { LogHandle handle; const char *str_event = "sALLOC mode=l";