Tethering offload stats updates are eventually consistent
This patch removes the call to runWithScissors() in
OffloadController#getTetherStats() that was causing a deadlock when
NetworkStatsService would be polled for stats in certain threading
contexts.
Instead of trying to query the tethering offload HAL synchronously all
the time, this patch:
- changes getTetherStats() to only call into the offload HAL when it
detects that it is called on the same thread as the Tethering handler
thread.
- changes the map of interface to accumulated tethering forwarded stats
to be concurrent.
This makes stats reading from getTetherStats() eventually consistent.
From the point of view of getTetherStats(), it preserves the guarantees
that tethering stats are monotonically increasing, and also guarantees
no tearing between rx bytes and tx bytes.
Bug: 29337859
Bug: 32163131
Bug: 64771555
Test: runtest frameworks-net
Merged-In: Ibcd351ad0225ef146b00a807833f76d2a886f6c1
Merged-In: I61786d61fe1422e429c0dd9eadaff6f02eb850e7
Merged-In: I999d1d1bf72e7ab02c5d17f37aad00bc711d3fc5
(cherry pick from commit eb5e465edd78bea26289f779b635c7e94d934854)
Change-Id: I28646b962cee8c8a6efd66059f84873c02ac5810
diff --git a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
index 4393e35..bcc74c0 100644
--- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
+++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
@@ -32,11 +32,13 @@
import android.net.RouteInfo;
import android.net.util.SharedLog;
import android.os.Handler;
+import android.os.Looper;
import android.os.INetworkManagementService;
import android.os.RemoteException;
import android.os.SystemClock;
import android.provider.Settings;
import android.text.TextUtils;
+import com.android.server.connectivity.tethering.OffloadHardwareInterface.ForwardedStats;
import com.android.internal.util.IndentingPrintWriter;
@@ -46,8 +48,10 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.Map;
import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
/**
@@ -79,8 +83,8 @@
// Maps upstream interface names to offloaded traffic statistics.
// Always contains the latest value received from the hardware for each interface, regardless of
// whether offload is currently running on that interface.
- private HashMap<String, OffloadHardwareInterface.ForwardedStats>
- mForwardedStats = new HashMap<>();
+ private ConcurrentHashMap<String, ForwardedStats> mForwardedStats =
+ new ConcurrentHashMap<>(16, 0.75F, 1);
// Maps upstream interface names to interface quotas.
// Always contains the latest value received from the framework for each interface, regardless
@@ -205,27 +209,29 @@
private class OffloadTetheringStatsProvider extends ITetheringStatsProvider.Stub {
@Override
public NetworkStats getTetherStats(int how) {
+ // getTetherStats() is the only function in OffloadController that can be called from
+ // a different thread. Do not attempt to update stats by querying the offload HAL
+ // synchronously from a different thread than our Handler thread. http://b/64771555.
+ Runnable updateStats = () -> { updateStatsForCurrentUpstream(); };
+ if (Looper.myLooper() == mHandler.getLooper()) {
+ updateStats.run();
+ } else {
+ mHandler.post(updateStats);
+ }
+
NetworkStats stats = new NetworkStats(SystemClock.elapsedRealtime(), 0);
+ NetworkStats.Entry entry = new NetworkStats.Entry();
+ entry.set = SET_DEFAULT;
+ entry.tag = TAG_NONE;
+ entry.uid = (how == STATS_PER_UID) ? UID_TETHERING : UID_ALL;
- // We can't just post to mHandler because we are mostly (but not always) called by
- // NetworkStatsService#performPollLocked, which is (currently) on the same thread as us.
- mHandler.runWithScissors(() -> {
- // We have to report both per-interface and per-UID stats, because offloaded traffic
- // is not seen by kernel interface counters.
- NetworkStats.Entry entry = new NetworkStats.Entry();
- entry.set = SET_DEFAULT;
- entry.tag = TAG_NONE;
- entry.uid = (how == STATS_PER_UID) ? UID_TETHERING : UID_ALL;
-
- updateStatsForCurrentUpstream();
-
- for (String iface : mForwardedStats.keySet()) {
- entry.iface = iface;
- entry.rxBytes = mForwardedStats.get(iface).rxBytes;
- entry.txBytes = mForwardedStats.get(iface).txBytes;
- stats.addValues(entry);
- }
- }, 0);
+ for (Map.Entry<String, ForwardedStats> kv : mForwardedStats.entrySet()) {
+ ForwardedStats value = kv.getValue();
+ entry.iface = kv.getKey();
+ entry.rxBytes = value.rxBytes;
+ entry.txBytes = value.txBytes;
+ stats.addValues(entry);
+ }
return stats;
}
@@ -247,10 +253,21 @@
return;
}
- if (!mForwardedStats.containsKey(iface)) {
- mForwardedStats.put(iface, new OffloadHardwareInterface.ForwardedStats());
+ // Always called on the handler thread.
+ //
+ // Use get()/put() instead of updating ForwardedStats in place because we can be called
+ // concurrently with getTetherStats. In combination with the guarantees provided by
+ // ConcurrentHashMap, this ensures that getTetherStats always gets the most recent copy of
+ // the stats for each interface, and does not observe partial writes where rxBytes is
+ // updated and txBytes is not.
+ ForwardedStats diff = mHwInterface.getForwardedStats(iface);
+ ForwardedStats base = mForwardedStats.get(iface);
+ if (base != null) {
+ diff.add(base);
}
- mForwardedStats.get(iface).add(mHwInterface.getForwardedStats(iface));
+ mForwardedStats.put(iface, diff);
+ // diff is a new object, just created by getForwardedStats(). Therefore, anyone reading from
+ // mForwardedStats (i.e., any caller of getTetherStats) will see the new stats immediately.
}
private boolean maybeUpdateDataLimit(String iface) {
diff --git a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
index 360e5f6..3f17b9eb 100644
--- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
+++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
@@ -404,23 +404,39 @@
when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn(ethernetStats);
when(mHardware.getForwardedStats(eq(mobileIface))).thenReturn(mobileStats);
+ InOrder inOrder = inOrder(mHardware);
+
final LinkProperties lp = new LinkProperties();
lp.setInterfaceName(ethernetIface);
offload.setUpstreamLinkProperties(lp);
+ // Previous upstream was null, so no stats are fetched.
+ inOrder.verify(mHardware, never()).getForwardedStats(any());
lp.setInterfaceName(mobileIface);
offload.setUpstreamLinkProperties(lp);
+ // Expect that we fetch stats from the previous upstream.
+ inOrder.verify(mHardware, times(1)).getForwardedStats(eq(ethernetIface));
lp.setInterfaceName(ethernetIface);
offload.setUpstreamLinkProperties(lp);
+ // Expect that we fetch stats from the previous upstream.
+ inOrder.verify(mHardware, times(1)).getForwardedStats(eq(mobileIface));
+ ethernetStats = new ForwardedStats();
ethernetStats.rxBytes = 100000;
ethernetStats.txBytes = 100000;
+ when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn(ethernetStats);
offload.setUpstreamLinkProperties(null);
+ // Expect that we fetch stats from the previous upstream.
+ inOrder.verify(mHardware, times(1)).getForwardedStats(eq(ethernetIface));
ITetheringStatsProvider provider = mTetherStatsProviderCaptor.getValue();
NetworkStats stats = provider.getTetherStats(STATS_PER_IFACE);
NetworkStats perUidStats = provider.getTetherStats(STATS_PER_UID);
+ waitForIdle();
+ // There is no current upstream, so no stats are fetched.
+ inOrder.verify(mHardware, never()).getForwardedStats(eq(ethernetIface));
+ inOrder.verifyNoMoreInteractions();
assertEquals(2, stats.size());
assertEquals(2, perUidStats.size());