From 52d6c5587fa8ec432ca8f6256b037f0ca5ee326c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Sat, 15 Jun 2024 12:41:36 -0700 Subject: [PATCH] SingleWriterBpfMap - introduce getSingleton with caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test: TreeHugger, atest BpfMapTest finally happy! Signed-off-by: Maciej Żenczykowski Change-Id: Ia4ddcbb4994bb0fd5df7d36e0d306ca5061d8963 --- .../networkstack/tethering/BpfMapTest.java | 18 ++++++------- .../src/com/android/server/BpfNetMaps.java | 10 +++---- .../net/module/util/SingleWriterBpfMap.java | 26 ++++++++++++++++++- 3 files changed, 39 insertions(+), 15 deletions(-) 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 a0928c5b81..47aebe8684 100644 --- a/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java +++ b/Tethering/tests/privileged/src/com/android/networkstack/tethering/BpfMapTest.java @@ -112,8 +112,8 @@ public final class BpfMapTest { private BpfMap openTestMap() throws Exception { return mShouldTestSingleWriterMap - ? new SingleWriterBpfMap<>(TETHER2_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class, - Tether6Value.class) + ? SingleWriterBpfMap.getSingleton(TETHER2_DOWNSTREAM6_FS_PATH, + TetherDownstream6Key.class, Tether6Value.class) : new BpfMap<>(TETHER_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class, Tether6Value.class); } @@ -510,12 +510,6 @@ public final class BpfMapTest { @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; @@ -524,11 +518,17 @@ public final class BpfMapTest { // Only require 3x to reduce test flakiness. final int expectedSpeedup = 3; - final BpfMap cachedMap = new SingleWriterBpfMap(TETHER_DOWNSTREAM6_FS_PATH, + final BpfMap cachedMap = SingleWriterBpfMap.getSingleton(TETHER2_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class, Tether6Value.class); final BpfMap uncachedMap = new BpfMap(TETHER_DOWNSTREAM6_FS_PATH, TetherDownstream6Key.class, Tether6Value.class); + // Ensure the maps are not empty. + for (int i = 0; i < mTestData.size(); i++) { + cachedMap.insertEntry(mTestData.keyAt(i), mTestData.valueAt(i)); + uncachedMap.insertEntry(mTestData.keyAt(i), mTestData.valueAt(i)); + } + final CompletableFuture cachedResult = new CompletableFuture<>(); final CompletableFuture uncachedResult = new CompletableFuture<>(); diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java index a30735ae82..b3e7d8cd25 100644 --- a/service/src/com/android/server/BpfNetMaps.java +++ b/service/src/com/android/server/BpfNetMaps.java @@ -189,7 +189,7 @@ public class BpfNetMaps { @RequiresApi(Build.VERSION_CODES.TIRAMISU) private static IBpfMap getConfigurationMap() { try { - return new SingleWriterBpfMap<>( + return SingleWriterBpfMap.getSingleton( CONFIGURATION_MAP_PATH, S32.class, U32.class); } catch (ErrnoException e) { throw new IllegalStateException("Cannot open netd configuration map", e); @@ -199,7 +199,7 @@ public class BpfNetMaps { @RequiresApi(Build.VERSION_CODES.TIRAMISU) private static IBpfMap getUidOwnerMap() { try { - return new SingleWriterBpfMap<>( + return SingleWriterBpfMap.getSingleton( UID_OWNER_MAP_PATH, S32.class, UidOwnerValue.class); } catch (ErrnoException e) { throw new IllegalStateException("Cannot open uid owner map", e); @@ -209,7 +209,7 @@ public class BpfNetMaps { @RequiresApi(Build.VERSION_CODES.TIRAMISU) private static IBpfMap getUidPermissionMap() { try { - return new SingleWriterBpfMap<>( + return SingleWriterBpfMap.getSingleton( UID_PERMISSION_MAP_PATH, S32.class, U8.class); } catch (ErrnoException e) { throw new IllegalStateException("Cannot open uid permission map", e); @@ -230,7 +230,7 @@ public class BpfNetMaps { @RequiresApi(Build.VERSION_CODES.TIRAMISU) private static IBpfMap getDataSaverEnabledMap() { try { - return new SingleWriterBpfMap<>( + return SingleWriterBpfMap.getSingleton( DATA_SAVER_ENABLED_MAP_PATH, S32.class, U8.class); } catch (ErrnoException e) { throw new IllegalStateException("Cannot open data saver enabled map", e); @@ -240,7 +240,7 @@ public class BpfNetMaps { @RequiresApi(Build.VERSION_CODES.TIRAMISU) private static IBpfMap getIngressDiscardMap() { try { - return new SingleWriterBpfMap<>(INGRESS_DISCARD_MAP_PATH, + return SingleWriterBpfMap.getSingleton(INGRESS_DISCARD_MAP_PATH, IngressDiscardKey.class, IngressDiscardValue.class); } catch (ErrnoException e) { throw new IllegalStateException("Cannot open ingress discard map", e); diff --git a/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java b/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java index bbd921d179..cd6bfec294 100644 --- a/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java +++ b/staticlibs/device/com/android/net/module/util/SingleWriterBpfMap.java @@ -17,6 +17,7 @@ package com.android.net.module.util; import android.os.Build; import android.system.ErrnoException; +import android.util.Pair; import androidx.annotation.NonNull; import androidx.annotation.RequiresApi; @@ -61,7 +62,12 @@ public class SingleWriterBpfMap extends BpfM // our code can contain hundreds of items. private final HashMap mCache = new HashMap<>(); - public SingleWriterBpfMap(@NonNull final String path, final Class key, + // This should only ever be called (hence private) once for a given 'path'. + // Java-wise what matters is the entire {path, key, value} triplet, + // but of course the kernel exclusive lock is just on the path (fd), + // and any BpfMap has (or should have...) well defined key/value types + // (or at least their sizes) so in practice it doesn't really matter. + private SingleWriterBpfMap(@NonNull final String path, final Class key, final Class value) throws ErrnoException, NullPointerException { super(path, BPF_F_RDWR_EXCLUSIVE, key, value); @@ -73,6 +79,24 @@ public class SingleWriterBpfMap extends BpfM } } + // This allows reuse of SingleWriterBpfMap objects for the same {path, keyClass, valueClass}. + // These are never destroyed, so once created the lock is (effectively) held till process death + // (even if fixed, there would still be a write-only fd cache in underlying BpfMap base class). + private static final HashMap>, SingleWriterBpfMap> + singletonCache = new HashMap<>(); + + // This is the public 'factory method' that (creates if needed and) returns a singleton instance + // for a given map. This holds an exclusive lock and has a permanent write-through cache. + // It will not be released until process death (or at least unload of the relevant class loader) + public synchronized static SingleWriterBpfMap + getSingleton(@NonNull final String path, final Class key, final Class value) + throws ErrnoException, NullPointerException { + var cacheKey = new Pair<>(path, new Pair(key, value)); + if (!singletonCache.containsKey(cacheKey)) + singletonCache.put(cacheKey, new SingleWriterBpfMap(path, key, value)); + return singletonCache.get(cacheKey); + } + @Override public synchronized void updateEntry(K key, V value) throws ErrnoException { super.updateEntry(key, value);