From 91df4360f6721a7b6354a350018308a42c626637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Sat, 12 Aug 2023 11:16:36 +0000 Subject: [PATCH 1/3] eliminate InitStats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: 294604315 Test: TreeHugger Signed-off-by: Maciej Żenczykowski Change-Id: Ie39de2b30a824f86c80517a64c76841980aa9739 --- .../libs/libnetworkstats/BpfNetworkStats.cpp | 46 +++++++++---------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp index 317c3f9f10..d6cec91c75 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp @@ -40,26 +40,20 @@ namespace bpf { using base::Result; -// This explicitly zero-initializes the relevant Stats fields. -void InitStats(Stats* stats) { - stats->rxBytes = 0; - stats->rxPackets = 0; - stats->txBytes = 0; - stats->txPackets = 0; -} - int bpfGetUidStatsInternal(uid_t uid, Stats* stats, const BpfMap& appUidStatsMap) { - InitStats(stats); auto statsEntry = appUidStatsMap.readValue(uid); - if (statsEntry.ok()) { - stats->rxPackets = statsEntry.value().rxPackets; - stats->txPackets = statsEntry.value().txPackets; - stats->rxBytes = statsEntry.value().rxBytes; - stats->txBytes = statsEntry.value().txBytes; + if (!statsEntry.ok()) { + *stats = {}; + return (statsEntry.error().code() == ENOENT) ? 0 : -statsEntry.error().code(); } - return (statsEntry.ok() || statsEntry.error().code() == ENOENT) ? 0 - : -statsEntry.error().code(); + *stats = { + .rxPackets = statsEntry.value().rxPackets, + .txPackets = statsEntry.value().txPackets, + .rxBytes = statsEntry.value().rxBytes, + .txBytes = statsEntry.value().txBytes, + }; + return 0; } int bpfGetUidStats(uid_t uid, Stats* stats) { @@ -70,7 +64,7 @@ int bpfGetUidStats(uid_t uid, Stats* stats) { int bpfGetIfaceStatsInternal(const char* iface, Stats* stats, const BpfMap& ifaceStatsMap, const BpfMap& ifaceNameMap) { - InitStats(stats); + *stats = {}; int64_t unknownIfaceBytesTotal = 0; const auto processIfaceStats = [iface, stats, &ifaceNameMap, &unknownIfaceBytesTotal]( @@ -105,16 +99,18 @@ int bpfGetIfaceStats(const char* iface, Stats* stats) { int bpfGetIfIndexStatsInternal(uint32_t ifindex, Stats* stats, const BpfMap& ifaceStatsMap) { - InitStats(stats); auto statsEntry = ifaceStatsMap.readValue(ifindex); - if (statsEntry.ok()) { - stats->rxPackets = statsEntry.value().rxPackets; - stats->txPackets = statsEntry.value().txPackets; - stats->rxBytes = statsEntry.value().rxBytes; - stats->txBytes = statsEntry.value().txBytes; - return 0; + if (!statsEntry.ok()) { + *stats = {}; + return (statsEntry.error().code() == ENOENT) ? 0 : -statsEntry.error().code(); } - return (statsEntry.error().code() == ENOENT) ? 0 : -statsEntry.error().code(); + *stats = { + .rxPackets = statsEntry.value().rxPackets, + .txPackets = statsEntry.value().txPackets, + .rxBytes = statsEntry.value().rxBytes, + .txBytes = statsEntry.value().txBytes, + }; + return 0; } int bpfGetIfIndexStats(int ifindex, Stats* stats) { From 4bcac5dadfe78809b144584f75e7667688681677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Sat, 12 Aug 2023 12:00:21 +0000 Subject: [PATCH 2/3] eliminate 'struct Stats' in favour of StatsValue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: 294604315 Test: TreeHugger Signed-off-by: Maciej Żenczykowski Change-Id: Ib45a130e7fa36ad90334c00de637d523455e000e --- bpf_progs/netd.h | 7 ----- ...android_server_net_NetworkStatsService.cpp | 10 +++---- .../libs/libnetworkstats/BpfNetworkStats.cpp | 26 ++++++------------- .../libnetworkstats/BpfNetworkStatsTest.cpp | 16 ++++++------ .../include/netdbpf/BpfNetworkStats.h | 12 ++++----- 5 files changed, 27 insertions(+), 44 deletions(-) diff --git a/bpf_progs/netd.h b/bpf_progs/netd.h index 1326db03d3..2995adad06 100644 --- a/bpf_progs/netd.h +++ b/bpf_progs/netd.h @@ -60,13 +60,6 @@ typedef struct { } IfaceValue; STRUCT_SIZE(IfaceValue, 16); -typedef struct { - uint64_t rxBytes; - uint64_t rxPackets; - uint64_t txBytes; - uint64_t txPackets; -} Stats; - typedef struct { uint64_t timestampNs; uint32_t ifindex; diff --git a/service-t/jni/com_android_server_net_NetworkStatsService.cpp b/service-t/jni/com_android_server_net_NetworkStatsService.cpp index ae553f0e59..4a6e7335c4 100644 --- a/service-t/jni/com_android_server_net_NetworkStatsService.cpp +++ b/service-t/jni/com_android_server_net_NetworkStatsService.cpp @@ -51,7 +51,7 @@ enum StatsType { TCP_TX_PACKETS = 5, // not supported, always returns UNKNOWN (-1) }; -static uint64_t getStatsType(Stats* stats, StatsType type) { +static uint64_t getStatsType(StatsValue* stats, StatsType type) { switch (type) { case RX_BYTES: return stats->rxBytes; @@ -69,7 +69,7 @@ static uint64_t getStatsType(Stats* stats, StatsType type) { } static jlong nativeGetTotalStat(JNIEnv* env, jclass clazz, jint type) { - Stats stats = {}; + StatsValue stats = {}; if (bpfGetIfaceStats(NULL, &stats) == 0) { return getStatsType(&stats, (StatsType) type); @@ -84,7 +84,7 @@ static jlong nativeGetIfaceStat(JNIEnv* env, jclass clazz, jstring iface, jint t return UNKNOWN; } - Stats stats = {}; + StatsValue stats = {}; if (bpfGetIfaceStats(iface8.c_str(), &stats) == 0) { return getStatsType(&stats, (StatsType) type); @@ -94,7 +94,7 @@ static jlong nativeGetIfaceStat(JNIEnv* env, jclass clazz, jstring iface, jint t } static jlong nativeGetIfIndexStat(JNIEnv* env, jclass clazz, jint ifindex, jint type) { - Stats stats = {}; + StatsValue stats = {}; if (bpfGetIfIndexStats(ifindex, &stats) == 0) { return getStatsType(&stats, (StatsType) type); } else { @@ -103,7 +103,7 @@ static jlong nativeGetIfIndexStat(JNIEnv* env, jclass clazz, jint ifindex, jint } static jlong nativeGetUidStat(JNIEnv* env, jclass clazz, jint uid, jint type) { - Stats stats = {}; + StatsValue stats = {}; if (bpfGetUidStats(uid, &stats) == 0) { return getStatsType(&stats, (StatsType) type); diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp index d6cec91c75..0d04c05c95 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp @@ -40,28 +40,23 @@ namespace bpf { using base::Result; -int bpfGetUidStatsInternal(uid_t uid, Stats* stats, +int bpfGetUidStatsInternal(uid_t uid, StatsValue* stats, const BpfMap& appUidStatsMap) { auto statsEntry = appUidStatsMap.readValue(uid); if (!statsEntry.ok()) { *stats = {}; return (statsEntry.error().code() == ENOENT) ? 0 : -statsEntry.error().code(); } - *stats = { - .rxPackets = statsEntry.value().rxPackets, - .txPackets = statsEntry.value().txPackets, - .rxBytes = statsEntry.value().rxBytes, - .txBytes = statsEntry.value().txBytes, - }; + *stats = statsEntry.value(); return 0; } -int bpfGetUidStats(uid_t uid, Stats* stats) { +int bpfGetUidStats(uid_t uid, StatsValue* stats) { static BpfMapRO appUidStatsMap(APP_UID_STATS_MAP_PATH); return bpfGetUidStatsInternal(uid, stats, appUidStatsMap); } -int bpfGetIfaceStatsInternal(const char* iface, Stats* stats, +int bpfGetIfaceStatsInternal(const char* iface, StatsValue* stats, const BpfMap& ifaceStatsMap, const BpfMap& ifaceNameMap) { *stats = {}; @@ -91,29 +86,24 @@ int bpfGetIfaceStatsInternal(const char* iface, Stats* stats, return res.ok() ? 0 : -res.error().code(); } -int bpfGetIfaceStats(const char* iface, Stats* stats) { +int bpfGetIfaceStats(const char* iface, StatsValue* stats) { static BpfMapRO ifaceStatsMap(IFACE_STATS_MAP_PATH); static BpfMapRO ifaceIndexNameMap(IFACE_INDEX_NAME_MAP_PATH); return bpfGetIfaceStatsInternal(iface, stats, ifaceStatsMap, ifaceIndexNameMap); } -int bpfGetIfIndexStatsInternal(uint32_t ifindex, Stats* stats, +int bpfGetIfIndexStatsInternal(uint32_t ifindex, StatsValue* stats, const BpfMap& ifaceStatsMap) { auto statsEntry = ifaceStatsMap.readValue(ifindex); if (!statsEntry.ok()) { *stats = {}; return (statsEntry.error().code() == ENOENT) ? 0 : -statsEntry.error().code(); } - *stats = { - .rxPackets = statsEntry.value().rxPackets, - .txPackets = statsEntry.value().txPackets, - .rxBytes = statsEntry.value().rxBytes, - .txBytes = statsEntry.value().txBytes, - }; + *stats = statsEntry.value(); return 0; } -int bpfGetIfIndexStats(int ifindex, Stats* stats) { +int bpfGetIfIndexStats(int ifindex, StatsValue* stats) { static BpfMapRO ifaceStatsMap(IFACE_STATS_MAP_PATH); return bpfGetIfIndexStatsInternal(ifindex, stats, ifaceStatsMap); } diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp index 4f85d9b3ba..76c56eb5ff 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp @@ -116,7 +116,7 @@ class BpfNetworkStatsHelperTest : public testing::Test { EXPECT_RESULT_OK(mFakeIfaceIndexNameMap.writeValue(ifaceIndex, iface, BPF_ANY)); } - void expectStatsEqual(const StatsValue& target, const Stats& result) { + void expectStatsEqual(const StatsValue& target, const StatsValue& result) { EXPECT_EQ(target.rxPackets, result.rxPackets); EXPECT_EQ(target.rxBytes, result.rxBytes); EXPECT_EQ(target.txPackets, result.txPackets); @@ -194,7 +194,7 @@ TEST_F(BpfNetworkStatsHelperTest, TestUidStatsNoTraffic) { .txPackets = 0, .txBytes = 0, }; - Stats result1 = {}; + StatsValue result1 = {}; ASSERT_EQ(0, bpfGetUidStatsInternal(TEST_UID1, &result1, mFakeAppUidStatsMap)); expectStatsEqual(value1, result1); } @@ -217,11 +217,11 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetUidStatsTotal) { }; ASSERT_RESULT_OK(mFakeAppUidStatsMap.writeValue(TEST_UID1, value1, BPF_ANY)); ASSERT_RESULT_OK(mFakeAppUidStatsMap.writeValue(TEST_UID2, value2, BPF_ANY)); - Stats result1 = {}; + StatsValue result1 = {}; ASSERT_EQ(0, bpfGetUidStatsInternal(TEST_UID1, &result1, mFakeAppUidStatsMap)); expectStatsEqual(value1, result1); - Stats result2 = {}; + StatsValue result2 = {}; ASSERT_EQ(0, bpfGetUidStatsInternal(TEST_UID2, &result2, mFakeAppUidStatsMap)); expectStatsEqual(value2, result2); std::vector lines; @@ -255,15 +255,15 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetIfaceStatsInternal) { ifaceStatsKey = IFACE_INDEX3; EXPECT_RESULT_OK(mFakeIfaceStatsMap.writeValue(ifaceStatsKey, value1, BPF_ANY)); - Stats result1 = {}; + StatsValue result1 = {}; ASSERT_EQ(0, bpfGetIfaceStatsInternal(IFACE_NAME1, &result1, mFakeIfaceStatsMap, mFakeIfaceIndexNameMap)); expectStatsEqual(value1, result1); - Stats result2 = {}; + StatsValue result2 = {}; ASSERT_EQ(0, bpfGetIfaceStatsInternal(IFACE_NAME2, &result2, mFakeIfaceStatsMap, mFakeIfaceIndexNameMap)); expectStatsEqual(value2, result2); - Stats totalResult = {}; + StatsValue totalResult = {}; ASSERT_EQ(0, bpfGetIfaceStatsInternal(NULL, &totalResult, mFakeIfaceStatsMap, mFakeIfaceIndexNameMap)); StatsValue totalValue = { @@ -284,7 +284,7 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetIfIndexStatsInternal) { }; EXPECT_RESULT_OK(mFakeIfaceStatsMap.writeValue(IFACE_INDEX1, value, BPF_ANY)); - Stats result = {}; + StatsValue result = {}; ASSERT_EQ(0, bpfGetIfIndexStatsInternal(IFACE_INDEX1, &result, mFakeIfaceStatsMap)); expectStatsEqual(value, result); } diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/BpfNetworkStats.h b/service-t/native/libs/libnetworkstats/include/netdbpf/BpfNetworkStats.h index 0a9c01271a..ea068fcdf2 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/BpfNetworkStats.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/BpfNetworkStats.h @@ -56,14 +56,14 @@ bool operator==(const stats_line& lhs, const stats_line& rhs); bool operator<(const stats_line& lhs, const stats_line& rhs); // For test only -int bpfGetUidStatsInternal(uid_t uid, Stats* stats, +int bpfGetUidStatsInternal(uid_t uid, StatsValue* stats, const BpfMap& appUidStatsMap); // For test only -int bpfGetIfaceStatsInternal(const char* iface, Stats* stats, +int bpfGetIfaceStatsInternal(const char* iface, StatsValue* stats, const BpfMap& ifaceStatsMap, const BpfMap& ifaceNameMap); // For test only -int bpfGetIfIndexStatsInternal(uint32_t ifindex, Stats* stats, +int bpfGetIfIndexStatsInternal(uint32_t ifindex, StatsValue* stats, const BpfMap& ifaceStatsMap); // For test only int parseBpfNetworkStatsDetailInternal(std::vector& lines, @@ -113,9 +113,9 @@ int parseBpfNetworkStatsDevInternal(std::vector& lines, const BpfMap& statsMap, const BpfMap& ifaceMap); -int bpfGetUidStats(uid_t uid, Stats* stats); -int bpfGetIfaceStats(const char* iface, Stats* stats); -int bpfGetIfIndexStats(int ifindex, Stats* stats); +int bpfGetUidStats(uid_t uid, StatsValue* stats); +int bpfGetIfaceStats(const char* iface, StatsValue* stats); +int bpfGetIfIndexStats(int ifindex, StatsValue* stats); int parseBpfNetworkStatsDetail(std::vector* lines); int parseBpfNetworkStatsDev(std::vector* lines); From a693bac18092ba8fac9b31f85755339af6d239d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Sun, 13 Aug 2023 07:30:15 +0000 Subject: [PATCH 3/3] StatsValue - define operator+= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit while this is a little bit more code, it seems much better for the accumulation operation to be next to the struct definition itself (in case we ever add more fields) Test: TreeHugger Signed-off-by: Maciej Żenczykowski Change-Id: I26022db4566e69c964298d7b3f2cc4fa4a9a5152 --- bpf_progs/netd.h | 10 ++++++++++ .../native/libs/libnetworkstats/BpfNetworkStats.cpp | 5 +---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/bpf_progs/netd.h b/bpf_progs/netd.h index 2995adad06..dcf6d6a554 100644 --- a/bpf_progs/netd.h +++ b/bpf_progs/netd.h @@ -55,6 +55,16 @@ typedef struct { } StatsValue; STRUCT_SIZE(StatsValue, 4 * 8); // 32 +#ifdef __cplusplus +static inline StatsValue& operator+=(StatsValue& lhs, const StatsValue& rhs) { + lhs.rxPackets += rhs.rxPackets; + lhs.rxBytes += rhs.rxBytes; + lhs.txPackets += rhs.txPackets; + lhs.txBytes += rhs.txBytes; + return lhs; +} +#endif + typedef struct { char name[IFNAMSIZ]; } IfaceValue; diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp index 0d04c05c95..fed297919d 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp @@ -75,10 +75,7 @@ int bpfGetIfaceStatsInternal(const char* iface, StatsValue* stats, if (!statsEntry.ok()) { return statsEntry.error(); } - stats->rxPackets += statsEntry.value().rxPackets; - stats->txPackets += statsEntry.value().txPackets; - stats->rxBytes += statsEntry.value().rxBytes; - stats->txBytes += statsEntry.value().txBytes; + *stats += statsEntry.value(); } return Result(); };