diff options
| -rw-r--r-- | services/core/java/com/android/server/connectivity/tethering/OffloadController.java | 67 | ||||
| -rw-r--r-- | tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java | 16 |
2 files changed, 58 insertions, 25 deletions
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 4393e3527e4d..bcc74c0d49e4 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.NetworkStats; 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.net.InetAddress; 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 @@ public class OffloadController { // 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 @@ public class OffloadController { private class OffloadTetheringStatsProvider extends ITetheringStatsProvider.Stub { @Override public NetworkStats getTetherStats(int how) { - NetworkStats stats = new NetworkStats(SystemClock.elapsedRealtime(), 0); + // 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); + } - // 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); + 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; + + 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 @@ public class OffloadController { 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 53a9b5a6befb..c7290e77ba2d 100644 --- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java +++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java @@ -400,23 +400,39 @@ public class OffloadControllerTest { 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()); |