From 572bbe444f949083b1cf30d434445c621c7d1232 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Thu, 23 Feb 2023 11:14:01 -0800 Subject: [PATCH 1/3] Refactor Network Tracing into separate data source This splits the polling and DataSource aspects into separate classes in preparation for making polling a singleton. This change is not expected to change behavior. Note: * OnSetup saves the poll duration and passes it to Start to simplify the Poller's life cycle. * OnStart's handling of mTaskRunner/Loop is moved to the end of Start. * OnStop's handling of mTaskRunner is moved to the end of Stop and Stop is changed to continue on failure (so that the task runner is reset in all cases as it was before this change). Bug: 246985031 Test: atest libnetworkstats_test Change-Id: I521eb50cd00916aa4d98174f0db461508b532732 --- .../libnetworkstats/NetworkTraceHandler.cpp | 39 +++++++------ .../NetworkTraceHandlerTest.cpp | 17 +++--- .../include/netdbpf/NetworkTraceHandler.h | 57 ++++++++++++------- 3 files changed, 64 insertions(+), 49 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index aeadb4a256..894ff7d3b1 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -56,10 +56,10 @@ void NetworkTraceHandler::InitPerfettoTracing() { } NetworkTraceHandler::NetworkTraceHandler() - : NetworkTraceHandler([this](const PacketTrace& pkt) { + : mPoller([](const PacketTrace& pkt) { NetworkTraceHandler::Trace( - [this, pkt](NetworkTraceHandler::TraceContext ctx) { - Fill(pkt, *ctx.NewTracePacket()); + [pkt](NetworkTraceHandler::TraceContext ctx) { + NetworkTraceHandler::Fill(pkt, *ctx.NewTracePacket()); }); }) {} @@ -74,22 +74,16 @@ void NetworkTraceHandler::OnSetup(const SetupArgs& args) { } } -void NetworkTraceHandler::OnStart(const StartArgs&) { - if (!Start()) return; - mTaskRunner = perfetto::Platform::GetDefaultPlatform()->CreateTaskRunner({}); - Loop(); -} +void NetworkTraceHandler::OnStart(const StartArgs&) { mPoller.Start(mPollMs); } +void NetworkTraceHandler::OnStop(const StopArgs&) { mPoller.Stop(); } -void NetworkTraceHandler::OnStop(const StopArgs&) { - Stop(); - mTaskRunner.reset(); -} - -void NetworkTraceHandler::Loop() { - mTaskRunner->PostDelayedTask([this]() { Loop(); }, mPollMs); +void NetworkTracePoller::SchedulePolling() { + // Schedules another run of ourselves to recursively poll periodically. + mTaskRunner->PostDelayedTask([this]() { SchedulePolling(); }, mPollMs); ConsumeAll(); } +// static class method void NetworkTraceHandler::Fill(const PacketTrace& src, TracePacket& dst) { dst.set_timestamp(src.timestampNs); auto* event = dst.set_network_packet(); @@ -113,7 +107,7 @@ void NetworkTraceHandler::Fill(const PacketTrace& src, TracePacket& dst) { } } -bool NetworkTraceHandler::Start() { +bool NetworkTracePoller::Start(uint32_t pollMs) { ALOGD("Starting datasource"); auto status = mConfigurationMap.init(PACKET_TRACE_ENABLED_MAP_PATH); @@ -136,24 +130,29 @@ bool NetworkTraceHandler::Start() { return false; } + // Start a task runner to run ConsumeAll every mPollMs milliseconds. + mTaskRunner = perfetto::Platform::GetDefaultPlatform()->CreateTaskRunner({}); + mPollMs = pollMs; + SchedulePolling(); + return true; } -bool NetworkTraceHandler::Stop() { +bool NetworkTracePoller::Stop() { ALOGD("Stopping datasource"); auto res = mConfigurationMap.writeValue(0, false, BPF_ANY); if (!res.ok()) { ALOGW("Failed to disable tracing: %s", res.error().message().c_str()); - return false; } + mTaskRunner.reset(); mRingBuffer.reset(); - return true; + return res.ok(); } -bool NetworkTraceHandler::ConsumeAll() { +bool NetworkTracePoller::ConsumeAll() { if (mRingBuffer == nullptr) { ALOGW("Tracing is not active"); return false; diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp index 560194f6df..8d03e2a0f1 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp @@ -39,6 +39,9 @@ using ::testing::Test; namespace android { namespace bpf { +// Use uint32 max to cause the handler to never Loop. Instead, the tests will +// manually drive things by calling ConsumeAll explicitly. +constexpr uint32_t kNeverPoll = std::numeric_limits::max(); __be16 bindAndListen(int s) { sockaddr_in sin = {.sin_family = AF_INET}; @@ -83,7 +86,7 @@ struct PacketPrinter { } }; -class NetworkTraceHandlerTest : public testing::Test { +class NetworkTracePollerTest : public testing::Test { protected: void SetUp() { if (access(PACKET_TRACE_RINGBUF_PATH, R_OK)) { @@ -95,31 +98,31 @@ class NetworkTraceHandlerTest : public testing::Test { } }; -TEST_F(NetworkTraceHandlerTest, PollWhileInactive) { - NetworkTraceHandler handler([&](const PacketTrace& pkt) {}); +TEST_F(NetworkTracePollerTest, PollWhileInactive) { + NetworkTracePoller handler([&](const PacketTrace& pkt) {}); // One succeed after start and before stop. EXPECT_FALSE(handler.ConsumeAll()); - ASSERT_TRUE(handler.Start()); + ASSERT_TRUE(handler.Start(kNeverPoll)); EXPECT_TRUE(handler.ConsumeAll()); ASSERT_TRUE(handler.Stop()); EXPECT_FALSE(handler.ConsumeAll()); } -TEST_F(NetworkTraceHandlerTest, TraceTcpSession) { +TEST_F(NetworkTracePollerTest, TraceTcpSession) { __be16 server_port = 0; std::vector packets; // Record all packets with the bound address and current uid. This callback is // involked only within ConsumeAll, at which point the port should have // already been filled in and all packets have been processed. - NetworkTraceHandler handler([&](const PacketTrace& pkt) { + NetworkTracePoller handler([&](const PacketTrace& pkt) { if (pkt.sport != server_port && pkt.dport != server_port) return; if (pkt.uid != getuid()) return; packets.push_back(pkt); }); - ASSERT_TRUE(handler.Start()); + ASSERT_TRUE(handler.Start(kNeverPoll)); const uint32_t kClientTag = 2468; const uint32_t kServerTag = 1357; diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index c257aa090b..1ab35ef173 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -31,37 +31,22 @@ namespace android { namespace bpf { -class NetworkTraceHandler : public perfetto::DataSource { +// NetworkTracePoller is responsible for interactions with the BPF ring buffer +// including polling. This class is an internal helper for NetworkTraceHandler, +// it is not meant to be used elsewhere. +class NetworkTracePoller { public: - // Registers this DataSource. - static void RegisterDataSource(); - - // Connects to the system Perfetto daemon and registers the trace handler. - static void InitPerfettoTracing(); - - // Initialize with the default Perfetto callback. - NetworkTraceHandler(); - // Testonly: initialize with a callback capable of intercepting data. - NetworkTraceHandler(std::function callback) + NetworkTracePoller(std::function callback) : mCallback(std::move(callback)) {} // Testonly: standalone functions without perfetto dependency. - bool Start(); + bool Start(uint32_t pollMs); bool Stop(); bool ConsumeAll(); - // perfetto::DataSource overrides: - void OnSetup(const SetupArgs&) override; - void OnStart(const StartArgs&) override; - void OnStop(const StopArgs&) override; - - // Convert a PacketTrace into a Perfetto trace packet. - void Fill(const PacketTrace& src, - ::perfetto::protos::pbzero::TracePacket& dst); - private: - void Loop(); + void SchedulePolling(); // How often to poll the ring buffer, defined by the trace config. uint32_t mPollMs; @@ -80,5 +65,33 @@ class NetworkTraceHandler : public perfetto::DataSource { std::unique_ptr mTaskRunner; }; +// NetworkTraceHandler implements the android.network_packets data source. This +// class is registered with Perfetto and is instantiated when tracing starts and +// destroyed when tracing ends. There is one instance per trace session. +class NetworkTraceHandler : public perfetto::DataSource { + public: + // Registers this DataSource. + static void RegisterDataSource(); + + // Connects to the system Perfetto daemon and registers the trace handler. + static void InitPerfettoTracing(); + + // Initialize with the default Perfetto callback. + NetworkTraceHandler(); + + // perfetto::DataSource overrides: + void OnSetup(const SetupArgs& args) override; + void OnStart(const StartArgs&) override; + void OnStop(const StopArgs&) override; + + private: + // Convert a PacketTrace into a Perfetto trace packet. + static void Fill(const PacketTrace& src, + ::perfetto::protos::pbzero::TracePacket& dst); + + NetworkTracePoller mPoller; + uint32_t mPollMs; +}; + } // namespace bpf } // namespace android From 74e873607b6945db8daf3303f599347f223d0132 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Thu, 23 Feb 2023 16:22:39 -0800 Subject: [PATCH 2/3] Make NetworkTraceHandler thread safe This adds a mutex to guard access to all fields. The lock is taken liberally since calls should rarely (if ever) collide (for example, ConsumeAll is called at most every 100ms and shouldn't overlap). Bug: 246985031 Test: atest libnetworkstats_test Change-Id: I97791a808771bafe789091c9b54cbec0a31d1721 --- .../libnetworkstats/NetworkTraceHandler.cpp | 17 +++++++++-- .../include/netdbpf/NetworkTraceHandler.h | 28 ++++++++++++------- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index 894ff7d3b1..c9c79df4fa 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -79,8 +79,14 @@ void NetworkTraceHandler::OnStop(const StopArgs&) { mPoller.Stop(); } void NetworkTracePoller::SchedulePolling() { // Schedules another run of ourselves to recursively poll periodically. - mTaskRunner->PostDelayedTask([this]() { SchedulePolling(); }, mPollMs); - ConsumeAll(); + mTaskRunner->PostDelayedTask( + [this]() { + mMutex.lock(); + SchedulePolling(); + ConsumeAllLocked(); + mMutex.unlock(); + }, + mPollMs); } // static class method @@ -110,6 +116,7 @@ void NetworkTraceHandler::Fill(const PacketTrace& src, TracePacket& dst) { bool NetworkTracePoller::Start(uint32_t pollMs) { ALOGD("Starting datasource"); + std::scoped_lock lock(mMutex); auto status = mConfigurationMap.init(PACKET_TRACE_ENABLED_MAP_PATH); if (!status.ok()) { ALOGW("Failed to bind config map: %s", status.error().message().c_str()); @@ -141,6 +148,7 @@ bool NetworkTracePoller::Start(uint32_t pollMs) { bool NetworkTracePoller::Stop() { ALOGD("Stopping datasource"); + std::scoped_lock lock(mMutex); auto res = mConfigurationMap.writeValue(0, false, BPF_ANY); if (!res.ok()) { ALOGW("Failed to disable tracing: %s", res.error().message().c_str()); @@ -153,6 +161,11 @@ bool NetworkTracePoller::Stop() { } bool NetworkTracePoller::ConsumeAll() { + std::scoped_lock lock(mMutex); + return ConsumeAllLocked(); +} + +bool NetworkTracePoller::ConsumeAllLocked() { if (mRingBuffer == nullptr) { ALOGW("Tracing is not active"); return false; diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index 1ab35ef173..2a2243bb6f 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -22,6 +22,7 @@ #include #include +#include "android-base/thread_annotations.h" #include "bpf/BpfMap.h" #include "bpf/BpfRingbuf.h" @@ -40,29 +41,36 @@ class NetworkTracePoller { NetworkTracePoller(std::function callback) : mCallback(std::move(callback)) {} - // Testonly: standalone functions without perfetto dependency. - bool Start(uint32_t pollMs); - bool Stop(); - bool ConsumeAll(); + // Starts tracing with the given poll interval. + bool Start(uint32_t pollMs) EXCLUDES(mMutex); + + // Stops tracing and release any held state. + bool Stop() EXCLUDES(mMutex); + + // Consumes all available events from the ringbuffer. + bool ConsumeAll() EXCLUDES(mMutex); private: - void SchedulePolling(); + void SchedulePolling() REQUIRES(mMutex); + bool ConsumeAllLocked() REQUIRES(mMutex); + + std::mutex mMutex; // How often to poll the ring buffer, defined by the trace config. - uint32_t mPollMs; + uint32_t mPollMs GUARDED_BY(mMutex); // The function to process PacketTrace, typically a Perfetto sink. - std::function mCallback; + std::function mCallback GUARDED_BY(mMutex); // The BPF ring buffer handle. - std::unique_ptr> mRingBuffer; + std::unique_ptr> mRingBuffer GUARDED_BY(mMutex); // The packet tracing config map (really a 1-element array). - BpfMap mConfigurationMap; + BpfMap mConfigurationMap GUARDED_BY(mMutex); // This must be the last member, causing it to be the first deleted. If it is // not, members required for callbacks can be deleted before it's stopped. - std::unique_ptr mTaskRunner; + std::unique_ptr mTaskRunner GUARDED_BY(mMutex); }; // NetworkTraceHandler implements the android.network_packets data source. This From 8b4dc5e27343abeef658c1dec1c5c74bb5bcecf6 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Thu, 23 Feb 2023 16:34:01 -0800 Subject: [PATCH 3/3] Add support for concurrent data sources. Perfetto allows multiple trace sessions to run in parallel. Each trace session creates an instance of the registered DataSource. Bpf ring buffers only support a single consumer, so we don't want multiple instances reading concurrently. This patch fixes things by making the DataSource a very thing wrapper which delegates everything to a singleton. The singleton counts the number of active sessions so that start is only called if not already started, and stop is called if there are no remaining sessions. Note: it's not clear whether it would be better to take the min or max of poll_ms for active sessions. Min would be good for callers wanting high throughput data collection, but doing so could jeopordise callers using the poll_ms to limit the trace size (e.g. longer traces that are alright dropping >5kpps scenarios). In this change, we use whichever poll_ms was set first and make no promises. Bug: 246985031 Test: atest libnetworkstats_test Change-Id: Ic85cab2205e6d426bcfc913450edff50be373bb0 --- .../libnetworkstats/NetworkTraceHandler.cpp | 42 +++++++++++++++---- .../NetworkTraceHandlerTest.cpp | 18 ++++++++ .../include/netdbpf/NetworkTraceHandler.h | 12 ++++-- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index c9c79df4fa..be4ffe3a2f 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -55,13 +55,12 @@ void NetworkTraceHandler::InitPerfettoTracing() { NetworkTraceHandler::RegisterDataSource(); } -NetworkTraceHandler::NetworkTraceHandler() - : mPoller([](const PacketTrace& pkt) { - NetworkTraceHandler::Trace( - [pkt](NetworkTraceHandler::TraceContext ctx) { - NetworkTraceHandler::Fill(pkt, *ctx.NewTracePacket()); - }); - }) {} +// static +NetworkTracePoller NetworkTraceHandler::sPoller([](const PacketTrace& pkt) { + NetworkTraceHandler::Trace([pkt](NetworkTraceHandler::TraceContext ctx) { + NetworkTraceHandler::Fill(pkt, *ctx.NewTracePacket()); + }); +}); void NetworkTraceHandler::OnSetup(const SetupArgs& args) { const std::string& raw = args.config->network_packet_trace_config_raw(); @@ -74,8 +73,14 @@ void NetworkTraceHandler::OnSetup(const SetupArgs& args) { } } -void NetworkTraceHandler::OnStart(const StartArgs&) { mPoller.Start(mPollMs); } -void NetworkTraceHandler::OnStop(const StopArgs&) { mPoller.Stop(); } +void NetworkTraceHandler::OnStart(const StartArgs&) { + mStarted = sPoller.Start(mPollMs); +} + +void NetworkTraceHandler::OnStop(const StopArgs&) { + if (mStarted) sPoller.Stop(); + mStarted = false; +} void NetworkTracePoller::SchedulePolling() { // Schedules another run of ourselves to recursively poll periodically. @@ -117,6 +122,19 @@ bool NetworkTracePoller::Start(uint32_t pollMs) { ALOGD("Starting datasource"); std::scoped_lock lock(mMutex); + if (mSessionCount > 0) { + if (mPollMs != pollMs) { + // Nothing technical prevents mPollMs from changing, it's just unclear + // what the right behavior is. Taking the min of active values could poll + // too frequently giving some sessions too much data. Taking the max could + // be too infrequent. For now, do nothing. + ALOGI("poll_ms can't be changed while running, ignoring poll_ms=%d", + pollMs); + } + mSessionCount++; + return true; + } + auto status = mConfigurationMap.init(PACKET_TRACE_ENABLED_MAP_PATH); if (!status.ok()) { ALOGW("Failed to bind config map: %s", status.error().message().c_str()); @@ -142,6 +160,7 @@ bool NetworkTracePoller::Start(uint32_t pollMs) { mPollMs = pollMs; SchedulePolling(); + mSessionCount++; return true; } @@ -149,6 +168,11 @@ bool NetworkTracePoller::Stop() { ALOGD("Stopping datasource"); std::scoped_lock lock(mMutex); + if (mSessionCount == 0) return false; // This should never happen + + // If this isn't the last session, don't clean up yet. + if (--mSessionCount > 0) return true; + auto res = mConfigurationMap.writeValue(0, false, BPF_ANY); if (!res.ok()) { ALOGW("Failed to disable tracing: %s", res.error().message().c_str()); diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp index 8d03e2a0f1..543be214fc 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp @@ -109,6 +109,24 @@ TEST_F(NetworkTracePollerTest, PollWhileInactive) { EXPECT_FALSE(handler.ConsumeAll()); } +TEST_F(NetworkTracePollerTest, ConcurrentSessions) { + // Simulate two concurrent sessions (two starts followed by two stops). Check + // that tracing is stopped only after both sessions finish. + NetworkTracePoller handler([&](const PacketTrace& pkt) {}); + + ASSERT_TRUE(handler.Start(kNeverPoll)); + EXPECT_TRUE(handler.ConsumeAll()); + + ASSERT_TRUE(handler.Start(kNeverPoll)); + EXPECT_TRUE(handler.ConsumeAll()); + + ASSERT_TRUE(handler.Stop()); + EXPECT_TRUE(handler.ConsumeAll()); + + ASSERT_TRUE(handler.Stop()); + EXPECT_FALSE(handler.ConsumeAll()); +} + TEST_F(NetworkTracePollerTest, TraceTcpSession) { __be16 server_port = 0; std::vector packets; diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index 2a2243bb6f..3f244b338f 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -56,6 +56,12 @@ class NetworkTracePoller { std::mutex mMutex; + // Records the number of successfully started active sessions so that only the + // first active session attempts setup and only the last cleans up. Note that + // the session count will remain zero if Start fails. It is expected that Stop + // will not be called for any trace session where Start fails. + int mSessionCount GUARDED_BY(mMutex); + // How often to poll the ring buffer, defined by the trace config. uint32_t mPollMs GUARDED_BY(mMutex); @@ -84,9 +90,6 @@ class NetworkTraceHandler : public perfetto::DataSource { // Connects to the system Perfetto daemon and registers the trace handler. static void InitPerfettoTracing(); - // Initialize with the default Perfetto callback. - NetworkTraceHandler(); - // perfetto::DataSource overrides: void OnSetup(const SetupArgs& args) override; void OnStart(const StartArgs&) override; @@ -97,8 +100,9 @@ class NetworkTraceHandler : public perfetto::DataSource { static void Fill(const PacketTrace& src, ::perfetto::protos::pbzero::TracePacket& dst); - NetworkTracePoller mPoller; + static NetworkTracePoller sPoller; uint32_t mPollMs; + bool mStarted; }; } // namespace bpf