From 7bfdb000c6bd30a6b7d2833a4a5c34e3620cfe9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Tue, 12 Aug 2025 21:55:00 -0700 Subject: [PATCH 1/2] BpfMap.h: on 5.10+ use bulk bpf map api MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit for: forAll(f(K)) forAll(f(K,V)) consume(f(K,V)) Test: TreeHugger ct_m_boot && a_test BpfBitmapTest libnetworkstats_test libbpf_android_test netd_updatable_unit_test bpf_existence_test Flag: EXEMPT mainline Signed-off-by: Maciej Żenczykowski Change-Id: I6b4985796db0aa48ad16c153179cfd37682bb1c2 --- bpf/headers/include/bpf/BpfMap.h | 53 ++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/bpf/headers/include/bpf/BpfMap.h b/bpf/headers/include/bpf/BpfMap.h index db0d20ec09..c247ba23a3 100644 --- a/bpf/headers/include/bpf/BpfMap.h +++ b/bpf/headers/include/bpf/BpfMap.h @@ -147,6 +147,50 @@ class BpfMapRO { return {}; } + // Observed 4 <= sizeof(Key) <= 16 (48 for Java), 1 <= sizeof(Value) <= 32 (64 for Java) + // You can uncomment the following to check: + // static_assert(sizeof(Key) >= 4); + // static_assert(sizeof(Key) <= 16); // 48 observed, but not in C++ + // static_assert(sizeof(Value) >= 1); + // static_assert(sizeof(Value) <= 32); // 64 observed, but not in C++ + + // ~16KiB initial stack usage seems reasonable + static constexpr int BATCHSIZE = 16384 / (sizeof(Key) + sizeof(Value)); + static_assert(BATCHSIZE >= 256, "consider Key/Value size, whether incr mem limit, decr batch req"); + static_assert(BATCHSIZE * sizeof(Key) + BATCHSIZE * sizeof(Value) <= 16384); + + Result doBulkLookupAndMaybeDelete(bool del, const function &f) const { + union { Key k; uint32_t nr; } batch; + bool first = true; + + // starting with N == 1 fails with -28/ENOSPC in: + // BpfNetworkStatsTest.cpp BpfNetworkStatsHelperTest#TestGetStatsSortedAndGrouped + // requiring us to loop back around, kernel code itself claims that in practice 5 + // is almost always enough for a bucket (which is what you'd expect, it's not a good + // hashtable if there's lots of items in a single bucket) + // + // Since we start with 256+ we shouldn't ever actually need to increase N... + // Also note that the 'true' condition is not really an infinite loop, + // as we'll blow up the stack and crash instead of looping infinitely. + // But that also shouldn't happen cause it would imply/require a ridiculously + // large bpf map sitting entirely in one bucket... + for (int N = BATCHSIZE; true; N *= 2) { + // N is how many we have space for, can grow on demand as needed + Key keys[N]; + Value values[N]; + for (;;) { + uint32_t count = N; // how many to fetch (and possibly delete) + int rv = batchLookupAndMaybeDelete(mMapFd, first ? NULL : &batch, &batch, &keys, &values, &count, del); + if (rv && errno == ENOSPC) break; // not enough space for full HASH bucket, go around the *outer* loop + if (rv && errno != ENOENT) return ErrnoErrorf("BpfMap::doBulkLookupAndMaybeDelete() failed"); + // count is now how many *were* fetched (and possibly delete) + for (unsigned i = 0; i < count; ++i) f(keys[i], values[i]); + if (rv) return {}; // ENOENT -> success + first = false; + } + } + } + public: // Function that tries to get map from a pinned path. [[clang::reinitializes]] Result init(const char* path) { @@ -168,6 +212,12 @@ class BpfMapRO { // Does not allow early termination (via f erroring out) - may be implemented with bulk api Result forAll(const function &f) const { + // No kernel bpfmap bulk lookup api which doesn't return both keys & values. + if (isAtLeastKernelVersion(5, 10, 0)) return doBulkLookupAndMaybeDelete(/*delete*/ false, + [&f](const Key &key, const Value &) { + f(key); + } + ); return iterate( [&f](const Key &key) -> Result { f(key); @@ -193,6 +243,7 @@ class BpfMapRO { // Does not allow early termination (via f erroring out) - maybe implemented with bulk api Result forAll(const function &f) const { + if (isAtLeastKernelVersion(5, 10, 0)) return doBulkLookupAndMaybeDelete(/*delete*/ false, f); return iterate( [&f](const Key &key, const Value &value) -> Result { f(key, value); @@ -301,6 +352,7 @@ template class BpfMap : public BpfMapRW { protected: using BpfMapRW::mMapFd; + using BpfMapRW::doBulkLookupAndMaybeDelete; public: using BpfMapRW::BpfMapRW; @@ -356,6 +408,7 @@ class BpfMap : public BpfMapRW { // Does not allow early termination (via f erroring out) - maybe implemented with bulk api Result consume(const std::function& f) { + if (isAtLeastKernelVersion(5, 10, 0)) return doBulkLookupAndMaybeDelete(/*delete*/true, f); Result curKey = getFirstKey(); while (curKey.ok()) { const Result &nextKey = getNextKey(curKey.value()); From 1ad3d7f43d404c95fa692f6e42eba4a8918b043f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Wed, 13 Aug 2025 12:46:26 -0700 Subject: [PATCH 2/2] BpfNetworkStats: use bulk api on 5.10+ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most spots autoconverted, but this one needs special love. Test: TreeHugger Flag: EXEMPT mainline/noop Signed-off-by: Maciej Żenczykowski Change-Id: I40b29d989c5a51082ac8e9ab09b7644eee46d7cb --- .../libs/libnetworkstats/BpfNetworkStats.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp index e314196f1f..93b7bbc425 100644 --- a/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp +++ b/service-t/native/libs/libnetworkstats/BpfNetworkStats.cpp @@ -112,6 +112,23 @@ int bpfGetIfaceStatsInternal(const char* iface, StatsValue* stats, // specific iface int64_t unknownIfaceBytesTotal = 0; + + if (isAtLeastKernelVersion(5, 10, 0)) { + // On 5.10+ bulk lookup api returns values for free, so just use forAll(f(K,V)) + auto res = ifaceStatsMap.forAll( + [iface, stats, ifindex2name, &unknownIfaceBytesTotal, &ifaceStatsMap](const uint32_t& key, const StatsValue& value) { + Result ifname = ifindex2name(key); + if (!ifname.ok()) { + maybeLogUnknownIface(key, ifaceStatsMap, key, &unknownIfaceBytesTotal); + return; + } + if (!strcmp(iface, ifname.value().name)) *stats += value; + } + ); + return res.ok() ? 0 : -res.error().code(); + } + + // Before 5.10 we try to avoid doing readValue unless needed auto res = ifaceStatsMap.iterate( [iface, stats, ifindex2name, &unknownIfaceBytesTotal, &ifaceStatsMap](const uint32_t& key) -> Result { Result ifname = ifindex2name(key); @@ -120,7 +137,6 @@ int bpfGetIfaceStatsInternal(const char* iface, StatsValue* stats, return {}; } if (!strcmp(iface, ifname.value().name)) { - // TODO: bulk api makes this readValue() inefficient - use forAll(K, V) Result statsEntry = ifaceStatsMap.readValue(key); if (!statsEntry.ok()) return statsEntry.error(); *stats += statsEntry.value();