Merge changes I181a490b,Ie2619949,I4bd5fbdf,Ifc0c5baa,I437e6607 into main
* changes:
Use /proc/self instead of calling Os.getPid().
Use SingleWriterBpfMap in BpfNetMaps.
Add a microbenchmark test for BpfMap.
Add a SingleWriterBpfMap class that caches reads.
Deflake BpfMapTest#testNoFdLeaks.
diff --git a/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java b/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java
index d5d71bc..f8aa69f 100644
--- a/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java
+++ b/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java
@@ -22,19 +22,24 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
import android.net.MacAddress;
import android.os.Build;
+import android.os.SystemClock;
import android.system.ErrnoException;
import android.system.Os;
import android.system.OsConstants;
import android.util.ArrayMap;
import com.android.net.module.util.BpfMap;
+import com.android.net.module.util.SingleWriterBpfMap;
import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo;
import com.android.testutils.DevSdkIgnoreRunner;
@@ -42,10 +47,18 @@
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
import java.io.File;
import java.net.InetAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collection;
import java.util.NoSuchElementException;
+import java.util.Random;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
@@ -61,6 +74,17 @@
private BpfMap<TetherDownstream6Key, Tether6Value> mTestMap;
+ private final boolean mShouldTestSingleWriterMap;
+
+ @Parameterized.Parameters
+ public static Collection<Boolean> shouldTestSingleWriterMap() {
+ return Arrays.asList(true, false);
+ }
+
+ public BpfMapTest(boolean shouldTestSingleWriterMap) {
+ mShouldTestSingleWriterMap = shouldTestSingleWriterMap;
+ }
+
@BeforeClass
public static void setupOnce() {
System.loadLibrary(getTetheringJniLibraryName());
@@ -82,11 +106,16 @@
initTestMap();
}
- private void initTestMap() throws Exception {
- mTestMap = new BpfMap<>(
- TETHER_DOWNSTREAM6_FS_PATH,
- TetherDownstream6Key.class, Tether6Value.class);
+ private BpfMap<TetherDownstream6Key, Tether6Value> openTestMap() throws Exception {
+ return mShouldTestSingleWriterMap
+ ? new SingleWriterBpfMap<>(TETHER_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class,
+ Tether6Value.class)
+ : new BpfMap<>(TETHER_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class,
+ Tether6Value.class);
+ }
+ private void initTestMap() throws Exception {
+ mTestMap = openTestMap();
mTestMap.forEach((key, value) -> {
try {
assertTrue(mTestMap.deleteEntry(key));
@@ -357,6 +386,25 @@
}
@Test
+ public void testMapContentsCorrectOnOpen() throws Exception {
+ final BpfMap<TetherDownstream6Key, Tether6Value> map1, map2;
+
+ map1 = openTestMap();
+ map1.clear();
+ for (int i = 0; i < mTestData.size(); i++) {
+ map1.insertEntry(mTestData.keyAt(i), mTestData.valueAt(i));
+ }
+
+ // We can't close and reopen map1, because close does nothing. Open another map instead.
+ map2 = openTestMap();
+ for (int i = 0; i < mTestData.size(); i++) {
+ assertEquals(mTestData.valueAt(i), map2.getValue(mTestData.keyAt(i)));
+ }
+
+ map1.clear();
+ }
+
+ @Test
public void testInsertOverflow() throws Exception {
final ArrayMap<TetherDownstream6Key, Tether6Value> testData =
new ArrayMap<>();
@@ -396,8 +444,18 @@
}
}
- private static int getNumOpenFds() {
- return new File("/proc/" + Os.getpid() + "/fd").listFiles().length;
+ private static int getNumOpenBpfMapFds() throws Exception {
+ int numFds = 0;
+ File[] openFiles = new File("/proc/self/fd").listFiles();
+ for (int i = 0; i < openFiles.length; i++) {
+ final Path path = openFiles[i].toPath();
+ if (!Files.isSymbolicLink(path)) continue;
+ if ("anon_inode:bpf-map".equals(Files.readSymbolicLink(path).toString())) {
+ numFds++;
+ }
+ }
+ assertNotEquals("Couldn't find any BPF map fds opened by this process", 0, numFds);
+ return numFds;
}
@Test
@@ -406,7 +464,7 @@
// cache, expect that the fd amount is not increased in the iterations.
// See the comment of BpfMap#close.
final int iterations = 1000;
- final int before = getNumOpenFds();
+ final int before = getNumOpenBpfMapFds();
for (int i = 0; i < iterations; i++) {
try (BpfMap<TetherDownstream6Key, Tether6Value> map = new BpfMap<>(
TETHER_DOWNSTREAM6_FS_PATH,
@@ -414,15 +472,74 @@
// do nothing
}
}
- final int after = getNumOpenFds();
+ final int after = getNumOpenBpfMapFds();
// Check that the number of open fds is the same as before.
- // If this exact match becomes flaky, we probably need to distinguish that fd is belong
- // to "bpf-map".
- // ex:
- // $ adb shell ls -all /proc/16196/fd
- // [..] network_stack 64 2022-07-26 22:01:02.300002956 +0800 749 -> anon_inode:bpf-map
- // [..] network_stack 64 2022-07-26 22:01:02.188002956 +0800 75 -> anon_inode:[eventfd]
assertEquals("Fd leak after " + iterations + " iterations: ", before, after);
}
+
+ @Test
+ public void testNullKey() {
+ assertThrows(NullPointerException.class, () ->
+ mTestMap.insertOrReplaceEntry(null, mTestData.valueAt(0)));
+ }
+
+ private void runBenchmarkThread(BpfMap<TetherDownstream6Key, Tether6Value> map,
+ CompletableFuture<Integer> future, int runtimeMs) {
+ int numReads = 0;
+ final Random r = new Random();
+ final long start = SystemClock.elapsedRealtime();
+ final long stop = start + runtimeMs;
+ while (SystemClock.elapsedRealtime() < stop) {
+ try {
+ final Tether6Value v = map.getValue(mTestData.keyAt(r.nextInt(mTestData.size())));
+ assertNotNull(v);
+ numReads++;
+ } catch (Exception e) {
+ future.completeExceptionally(e);
+ return;
+ }
+ }
+ future.complete(numReads);
+ }
+
+ @Test
+ public void testSingleWriterCacheEffectiveness() throws Exception {
+ assumeTrue(mShouldTestSingleWriterMap);
+
+ // Ensure the map is not empty.
+ for (int i = 0; i < mTestData.size(); i++) {
+ mTestMap.insertEntry(mTestData.keyAt(i), mTestData.valueAt(i));
+ }
+
+ // Benchmark parameters.
+ final int timeoutMs = 5_000; // Only hit if threads don't complete.
+ final int benchmarkTimeMs = 2_000;
+ final int minReads = 50;
+ // Local testing on cuttlefish suggests that caching is ~10x faster.
+ // Only require 3x to reduce test flakiness.
+ final int expectedSpeedup = 3;
+
+ final BpfMap cachedMap = new SingleWriterBpfMap(TETHER_DOWNSTREAM6_FS_PATH,
+ TetherDownstream6Key.class, Tether6Value.class);
+ final BpfMap uncachedMap = new BpfMap(TETHER_DOWNSTREAM6_FS_PATH,
+ TetherDownstream6Key.class, Tether6Value.class);
+
+ final CompletableFuture<Integer> cachedResult = new CompletableFuture<>();
+ final CompletableFuture<Integer> uncachedResult = new CompletableFuture<>();
+
+ new Thread(() -> runBenchmarkThread(uncachedMap, uncachedResult, benchmarkTimeMs)).start();
+ new Thread(() -> runBenchmarkThread(cachedMap, cachedResult, benchmarkTimeMs)).start();
+
+ final int cached = cachedResult.get(timeoutMs, TimeUnit.MILLISECONDS);
+ final int uncached = uncachedResult.get(timeoutMs, TimeUnit.MILLISECONDS);
+
+ // Uncomment to see benchmark results.
+ // fail("Cached " + cached + ", uncached " + uncached + ": " + cached / uncached +"x");
+
+ assertTrue("Less than " + minReads + "cached reads observed", cached > minReads);
+ assertTrue("Less than " + minReads + "uncached reads observed", uncached > minReads);
+ assertTrue("Cached map not at least " + expectedSpeedup + "x faster",
+ cached > expectedSpeedup * uncached);
+ }
}
diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java
index 1047232..a30735a 100644
--- a/service/src/com/android/server/BpfNetMaps.java
+++ b/service/src/com/android/server/BpfNetMaps.java
@@ -72,6 +72,7 @@
import com.android.net.module.util.BpfDump;
import com.android.net.module.util.BpfMap;
import com.android.net.module.util.IBpfMap;
+import com.android.net.module.util.SingleWriterBpfMap;
import com.android.net.module.util.Struct;
import com.android.net.module.util.Struct.S32;
import com.android.net.module.util.Struct.U32;
@@ -188,7 +189,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<S32, U32> getConfigurationMap() {
try {
- return new BpfMap<>(
+ return new SingleWriterBpfMap<>(
CONFIGURATION_MAP_PATH, S32.class, U32.class);
} catch (ErrnoException e) {
throw new IllegalStateException("Cannot open netd configuration map", e);
@@ -198,7 +199,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<S32, UidOwnerValue> getUidOwnerMap() {
try {
- return new BpfMap<>(
+ return new SingleWriterBpfMap<>(
UID_OWNER_MAP_PATH, S32.class, UidOwnerValue.class);
} catch (ErrnoException e) {
throw new IllegalStateException("Cannot open uid owner map", e);
@@ -208,7 +209,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<S32, U8> getUidPermissionMap() {
try {
- return new BpfMap<>(
+ return new SingleWriterBpfMap<>(
UID_PERMISSION_MAP_PATH, S32.class, U8.class);
} catch (ErrnoException e) {
throw new IllegalStateException("Cannot open uid permission map", e);
@@ -218,6 +219,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<CookieTagMapKey, CookieTagMapValue> getCookieTagMap() {
try {
+ // Cannot use SingleWriterBpfMap because it's written by ClatCoordinator as well.
return new BpfMap<>(COOKIE_TAG_MAP_PATH,
CookieTagMapKey.class, CookieTagMapValue.class);
} catch (ErrnoException e) {
@@ -228,7 +230,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<S32, U8> getDataSaverEnabledMap() {
try {
- return new BpfMap<>(
+ return new SingleWriterBpfMap<>(
DATA_SAVER_ENABLED_MAP_PATH, S32.class, U8.class);
} catch (ErrnoException e) {
throw new IllegalStateException("Cannot open data saver enabled map", e);
@@ -238,7 +240,7 @@
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
private static IBpfMap<IngressDiscardKey, IngressDiscardValue> getIngressDiscardMap() {
try {
- return new BpfMap<>(INGRESS_DISCARD_MAP_PATH,
+ return new SingleWriterBpfMap<>(INGRESS_DISCARD_MAP_PATH,
IngressDiscardKey.class, IngressDiscardValue.class);
} catch (ErrnoException e) {
throw new IllegalStateException("Cannot open ingress discard map", e);
diff --git a/staticlibs/Android.bp b/staticlibs/Android.bp
index ede6d3f..e2834b0 100644
--- a/staticlibs/Android.bp
+++ b/staticlibs/Android.bp
@@ -135,6 +135,7 @@
"device/com/android/net/module/util/BpfUtils.java",
"device/com/android/net/module/util/IBpfMap.java",
"device/com/android/net/module/util/JniUtil.java",
+ "device/com/android/net/module/util/SingleWriterBpfMap.java",
"device/com/android/net/module/util/TcUtils.java",
],
sdk_version: "module_current",
diff --git a/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java b/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java
new file mode 100644
index 0000000..3eb59d8
--- /dev/null
+++ b/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java
@@ -0,0 +1,129 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.android.net.module.util;
+
+import android.os.Build;
+import android.system.ErrnoException;
+
+import androidx.annotation.NonNull;
+import androidx.annotation.RequiresApi;
+
+import java.util.HashMap;
+import java.util.NoSuchElementException;
+
+/**
+ * Subclass of BpfMap for maps that are only ever written by one userspace writer.
+ *
+ * This class stores all map data in a userspace HashMap in addition to in the BPF map. This makes
+ * reads (but not iterations) much faster because they do not require a system call or converting
+ * the raw map read to the Value struct. See, e.g., b/343166906 .
+ *
+ * Users of this class must ensure that no BPF program ever writes to the map, and that all
+ * userspace writes to the map occur through this object. Other userspace code may still read from
+ * the map; only writes are required to go through this object.
+ *
+ * Reads and writes to this object are thread-safe and internally synchronized. The read and write
+ * methods are synchronized to ensure that current writers always result in a consistent internal
+ * state (without synchronization, two concurrent writes might update the underlying map and the
+ * cache in the opposite order, resulting in the cache being out of sync with the map).
+ *
+ * getNextKey and iteration over the map are not synchronized or cached and always access the
+ * isunderlying map. The values returned by these calls may be temporarily out of sync with the
+ * values read and written through this object.
+ *
+ * TODO: consider caching reads on iterations as well. This is not trivial because the semantics for
+ * iterating BPF maps require passing in the previously-returned key, and Java iterators only
+ * support iterating from the beginning. It could be done by implementing forEach and possibly by
+ * making getFirstKey and getNextKey private (if no callers are using them). Because HashMap is not
+ * thread-safe, implementing forEach would require either making that method synchronized (and
+ * block reads and updates from other threads until iteration is complete) or switching the
+ * internal HashMap to ConcurrentHashMap.
+ *
+ * @param <K> the key of the map.
+ * @param <V> the value of the map.
+ */
+@RequiresApi(Build.VERSION_CODES.S)
+public class SingleWriterBpfMap<K extends Struct, V extends Struct> extends BpfMap<K, V> {
+ // HashMap instead of ArrayMap because it performs better on larger maps, and many maps used in
+ // our code can contain hundreds of items.
+ private final HashMap<K, V> mCache = new HashMap<>();
+
+ protected SingleWriterBpfMap(@NonNull final String path, final int flag, final Class<K> key,
+ final Class<V> value) throws ErrnoException, NullPointerException {
+ super(path, flag, key, value);
+
+ if (flag != BPF_F_RDWR) {
+ throw new IllegalArgumentException(
+ "Using " + getClass().getName() + " for read-only maps does not make sense");
+ }
+
+ // Populate cache with the current map contents.
+ K currentKey = super.getFirstKey();
+ while (currentKey != null) {
+ mCache.put(currentKey, super.getValue(currentKey));
+ currentKey = super.getNextKey(currentKey);
+ }
+ }
+
+ public SingleWriterBpfMap(@NonNull final String path, final Class<K> key,
+ final Class<V> value) throws ErrnoException, NullPointerException {
+ this(path, BPF_F_RDWR, key, value);
+ }
+
+ @Override
+ public synchronized void updateEntry(K key, V value) throws ErrnoException {
+ super.updateEntry(key, value);
+ mCache.put(key, value);
+ }
+
+ @Override
+ public synchronized void insertEntry(K key, V value)
+ throws ErrnoException, IllegalStateException {
+ super.insertEntry(key, value);
+ mCache.put(key, value);
+ }
+
+ @Override
+ public synchronized void replaceEntry(K key, V value)
+ throws ErrnoException, NoSuchElementException {
+ super.replaceEntry(key, value);
+ mCache.put(key, value);
+ }
+
+ @Override
+ public synchronized boolean insertOrReplaceEntry(K key, V value) throws ErrnoException {
+ final boolean ret = super.insertOrReplaceEntry(key, value);
+ mCache.put(key, value);
+ return ret;
+ }
+
+ @Override
+ public synchronized boolean deleteEntry(K key) throws ErrnoException {
+ final boolean ret = super.deleteEntry(key);
+ mCache.remove(key);
+ return ret;
+ }
+
+ @Override
+ public synchronized boolean containsKey(@NonNull K key) throws ErrnoException {
+ return mCache.containsKey(key);
+ }
+
+ @Override
+ public synchronized V getValue(@NonNull K key) throws ErrnoException {
+ return mCache.get(key);
+ }
+}