From ce7431f1fc767b17659c01ca1cee3248114baaec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Wed, 12 Apr 2023 05:13:54 +0000 Subject: [PATCH] bpf network stats - move double accounting tag!=0 out of bpf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of also accounting tag!=0 traffic against tag==0 slot, while the bpf code writes into the map, move this logic into the userspace jni code which reads from the map. Simplifies the bpf program making things easier on the kernel's bpf verifier, and is better for performance, since a per-packet fixup operation becomes a per-poll fixup. Test: TreeHugger, atest libnetworkstats_test FrameworksNetTests Bug: 276296921 Signed-off-by: Maciej Żenczykowski Change-Id: Ic220a201781a1170bcffe327fe5664fc12b65dd9 --- bpf_progs/netd.c | 5 -- .../libs/libnetworkstats/BpfNetworkStats.cpp | 27 ++++--- .../libnetworkstats/BpfNetworkStatsTest.cpp | 76 ++++++++++++------- 3 files changed, 64 insertions(+), 44 deletions(-) diff --git a/bpf_progs/netd.c b/bpf_progs/netd.c index e068d8aaa5..d98fa5f987 100644 --- a/bpf_progs/netd.c +++ b/bpf_progs/netd.c @@ -446,11 +446,6 @@ static __always_inline inline int bpf_traffic_account(struct __sk_buff* skb, boo return match; } - if (key.tag) { - update_stats_with_config(skb, egress, &key, *selectedMap); - key.tag = 0; - } - do_packet_tracing(skb, egress, uid, tag, enable_tracing, kver); update_stats_with_config(skb, egress, &key, *selectedMap); update_app_uid_stats_map(skb, egress, &uid); diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp index cdcb0f850f..64a7a98bc2 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp @@ -126,7 +126,13 @@ int parseBpfNetworkStatsDetailInternal(std::vector* lines, if (!statsEntry.ok()) { return base::ResultError(statsEntry.error().message(), statsEntry.error().code()); } - lines->push_back(populateStatsEntry(key, statsEntry.value(), ifname)); + stats_line newLine = populateStatsEntry(key, statsEntry.value(), ifname); + lines->push_back(newLine); + if (newLine.tag) { + // account tagged traffic in the untagged stats (for historical reasons?) + newLine.tag = 0; + lines->push_back(newLine); + } return Result(); }; Result res = statsMap.iterate(processDetailUidStats); @@ -236,21 +242,20 @@ void groupNetworkStats(std::vector* lines) { std::sort(lines->begin(), lines->end()); // Similar to std::unique(), but aggregates the duplicates rather than discarding them. - size_t nextOutput = 0; + size_t currentOutput = 0; for (size_t i = 1; i < lines->size(); i++) { - if (lines->at(nextOutput) == lines->at(i)) { - lines->at(nextOutput) += lines->at(i); + // note that == operator only compares the 'key' portion: iface/uid/tag/set + if (lines->at(currentOutput) == lines->at(i)) { + // while += operator only affects the 'data' portion: {rx,tx}{Bytes,Packets} + lines->at(currentOutput) += lines->at(i); } else { - nextOutput++; - if (nextOutput != i) { - lines->at(nextOutput) = lines->at(i); - } + // okay, we're done aggregating the current line, move to the next one + lines->at(++currentOutput) = lines->at(i); } } - if (lines->size() != nextOutput + 1) { - lines->resize(nextOutput + 1); - } + // possibly shrink the vector - currentOutput is the last line with valid data + lines->resize(currentOutput + 1); } // True if lhs equals to rhs, only compare iface, uid, tag and set. diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp index bf42b620a4..f8d9ec863c 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStatsTest.cpp @@ -291,7 +291,7 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsDetail) { populateFakeStats(TEST_UID2, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); std::vector lines; ASSERT_EQ(0, parseBpfNetworkStatsDetailInternal(&lines, mFakeStatsMap, mFakeIfaceIndexNameMap)); - ASSERT_EQ((unsigned long)4, lines.size()); + ASSERT_EQ((unsigned long)7, lines.size()); } TEST_F(BpfNetworkStatsHelperTest, TestGetStatsWithSkippedIface) { @@ -409,12 +409,18 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsSortedAndGrouped) { .txPackets = TEST_PACKET0, .txBytes = TEST_BYTES0, }; - StatsValue value3 = { + StatsValue value3 = { // value1 *2 .rxPackets = TEST_PACKET0 * 2, .rxBytes = TEST_BYTES0 * 2, .txPackets = TEST_PACKET1 * 2, .txBytes = TEST_BYTES1 * 2, }; + StatsValue value5 = { // value2 + value3 + .rxPackets = TEST_PACKET1 + TEST_PACKET0 * 2, + .rxBytes = TEST_BYTES1 + TEST_BYTES0 * 2, + .txPackets = TEST_PACKET0 + TEST_PACKET1 * 2, + .txBytes = TEST_BYTES0 + TEST_BYTES1 * 2, + }; std::vector lines; @@ -426,8 +432,9 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsSortedAndGrouped) { // Test 1 line stats. populateFakeStats(TEST_UID1, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); ASSERT_EQ(0, parseBpfNetworkStatsDetailInternal(&lines, mFakeStatsMap, mFakeIfaceIndexNameMap)); - ASSERT_EQ((size_t) 1, lines.size()); - expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[0]); + ASSERT_EQ((size_t) 2, lines.size()); // TEST_TAG != 0 -> 1 entry becomes 2 lines + expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, 0, lines[0]); + expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[1]); lines.clear(); // These items should not be grouped. @@ -437,7 +444,7 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsSortedAndGrouped) { mFakeStatsMap); populateFakeStats(TEST_UID2, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); ASSERT_EQ(0, parseBpfNetworkStatsDetailInternal(&lines, mFakeStatsMap, mFakeIfaceIndexNameMap)); - ASSERT_EQ((size_t) 5, lines.size()); + ASSERT_EQ((size_t) 9, lines.size()); lines.clear(); // These items should be grouped. @@ -445,14 +452,18 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsSortedAndGrouped) { populateFakeStats(TEST_UID2, TEST_TAG, IFACE_INDEX3, TEST_COUNTERSET0, value1, mFakeStatsMap); ASSERT_EQ(0, parseBpfNetworkStatsDetailInternal(&lines, mFakeStatsMap, mFakeIfaceIndexNameMap)); - ASSERT_EQ((size_t) 5, lines.size()); + ASSERT_EQ((size_t) 9, lines.size()); // Verify Sorted & Grouped. - expectStatsLineEqual(value3, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[0]); - expectStatsLineEqual(value2, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET1, TEST_TAG, lines[1]); - expectStatsLineEqual(value2, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG + 1, lines[2]); - expectStatsLineEqual(value3, IFACE_NAME1, TEST_UID2, TEST_COUNTERSET0, TEST_TAG, lines[3]); - expectStatsLineEqual(value2, IFACE_NAME2, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[4]); + expectStatsLineEqual(value5, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, 0, lines[0]); + expectStatsLineEqual(value2, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET1, 0, lines[1]); + expectStatsLineEqual(value3, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[2]); + expectStatsLineEqual(value2, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET1, TEST_TAG, lines[3]); + expectStatsLineEqual(value2, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, TEST_TAG + 1, lines[4]); + expectStatsLineEqual(value3, IFACE_NAME1, TEST_UID2, TEST_COUNTERSET0, 0, lines[5]); + expectStatsLineEqual(value3, IFACE_NAME1, TEST_UID2, TEST_COUNTERSET0, TEST_TAG, lines[6]); + expectStatsLineEqual(value2, IFACE_NAME2, TEST_UID1, TEST_COUNTERSET0, 0, lines[7]); + expectStatsLineEqual(value2, IFACE_NAME2, TEST_UID1, TEST_COUNTERSET0, TEST_TAG, lines[8]); lines.clear(); // Perform test on IfaceStats. @@ -485,39 +496,48 @@ TEST_F(BpfNetworkStatsHelperTest, TestGetStatsSortAndOverflow) { .txPackets = TEST_PACKET1, .txBytes = TEST_BYTES1, }; + StatsValue value4 = { // value1 * 4 + .rxPackets = TEST_PACKET0 * 4, + .rxBytes = TEST_BYTES0 * 4, + .txPackets = TEST_PACKET1 * 4, + .txBytes = TEST_BYTES1 * 4, + }; // Mutate uid, 0 < TEST_UID1 < INT_MAX < INT_MIN < UINT_MAX. - populateFakeStats(0, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); - populateFakeStats(UINT_MAX, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); - populateFakeStats(INT_MIN, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); - populateFakeStats(INT_MAX, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(0, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(UINT_MAX, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(INT_MIN, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(INT_MAX, TEST_TAG, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); // Mutate tag, 0 < TEST_TAG < INT_MAX < INT_MIN < UINT_MAX. - populateFakeStats(TEST_UID1, INT_MAX, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); - populateFakeStats(TEST_UID1, INT_MIN, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); - populateFakeStats(TEST_UID1, 0, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(TEST_UID1, INT_MAX, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(TEST_UID1, INT_MIN, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); + populateFakeStats(TEST_UID1, 0, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); populateFakeStats(TEST_UID1, UINT_MAX, IFACE_INDEX1, TEST_COUNTERSET0, value1, mFakeStatsMap); // TODO: Mutate counterSet and enlarge TEST_MAP_SIZE if overflow on counterSet is possible. std::vector lines; ASSERT_EQ(0, parseBpfNetworkStatsDetailInternal(&lines, mFakeStatsMap, mFakeIfaceIndexNameMap)); - ASSERT_EQ((size_t) 8, lines.size()); + ASSERT_EQ((size_t) 12, lines.size()); // Uid 0 first - expectStatsLineEqual(value1, IFACE_NAME1, 0, TEST_COUNTERSET0, TEST_TAG, lines[0]); + expectStatsLineEqual(value1, IFACE_NAME1, 0, TEST_COUNTERSET0, 0, lines[0]); + expectStatsLineEqual(value1, IFACE_NAME1, 0, TEST_COUNTERSET0, TEST_TAG, lines[1]); // Test uid, mutate tag. - expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, 0, lines[1]); - expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, INT_MAX, lines[2]); - expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, INT_MIN, lines[3]); - expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, UINT_MAX, lines[4]); + expectStatsLineEqual(value4, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, 0, lines[2]); + expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, INT_MAX, lines[3]); + expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, INT_MIN, lines[4]); + expectStatsLineEqual(value1, IFACE_NAME1, TEST_UID1, TEST_COUNTERSET0, UINT_MAX, lines[5]); // Mutate uid. - expectStatsLineEqual(value1, IFACE_NAME1, INT_MAX, TEST_COUNTERSET0, TEST_TAG, lines[5]); - expectStatsLineEqual(value1, IFACE_NAME1, INT_MIN, TEST_COUNTERSET0, TEST_TAG, lines[6]); - expectStatsLineEqual(value1, IFACE_NAME1, UINT_MAX, TEST_COUNTERSET0, TEST_TAG, lines[7]); - lines.clear(); + expectStatsLineEqual(value1, IFACE_NAME1, INT_MAX, TEST_COUNTERSET0, 0, lines[6]); + expectStatsLineEqual(value1, IFACE_NAME1, INT_MAX, TEST_COUNTERSET0, TEST_TAG, lines[7]); + expectStatsLineEqual(value1, IFACE_NAME1, INT_MIN, TEST_COUNTERSET0, 0, lines[8]); + expectStatsLineEqual(value1, IFACE_NAME1, INT_MIN, TEST_COUNTERSET0, TEST_TAG, lines[9]); + expectStatsLineEqual(value1, IFACE_NAME1, UINT_MAX, TEST_COUNTERSET0, 0, lines[10]); + expectStatsLineEqual(value1, IFACE_NAME1, UINT_MAX, TEST_COUNTERSET0, TEST_TAG, lines[11]); } } // namespace bpf } // namespace android