From dca784af3c30bd077483d37001b8528b9d9ee490 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 14 Aug 2017 13:07:42 +0900 Subject: [PATCH 1/2] Don't time out when fetching tether offload stats. Currently, fetching tethering offload stats has a 1000ms timeout, after which we return stats of zero. However, returning zero is invalid and will cause various parts of the network stats accounting code to complain that statistics are moving backwards, causing at least a log.wtf, and possibly a crash. This CL removes the timeout entirely. An alternative would have been to keep the timeout and return null if the stats fetch failed. However, this complicates the code, and if the HAL is persistently slow, could cause no stats to be counted, ever. Given the impact of such behaviour on users' data plans it is likely better to block until the stats are collected. (cherry picked from commit fef69a126793c83aa0d99a8c597037c3a29929c0) Bug: 29337859 Bug: 32163131 Test: builds Test: OffloadControllerTest passes Change-Id: I1e017b6fef2d8a02a7edbf35bed3e4402f447ab9 --- .../tethering/OffloadController.java | 40 +++++++------------ 1 file changed, 14 insertions(+), 26 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 1a5ff778010cd..ccbac31c82c31 100644 --- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java +++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java @@ -46,7 +46,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Objects; import java.util.Set; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; /** @@ -58,8 +57,6 @@ import java.util.concurrent.TimeUnit; public class OffloadController { private static final String TAG = OffloadController.class.getSimpleName(); - private static final int STATS_FETCH_TIMEOUT_MS = 1000; - private final Handler mHandler; private final OffloadHardwareInterface mHwInterface; private final ContentResolver mContentResolver; @@ -177,33 +174,24 @@ public class OffloadController { @Override public NetworkStats getTetherStats() { NetworkStats stats = new NetworkStats(SystemClock.elapsedRealtime(), 0); - CountDownLatch latch = new CountDownLatch(1); - mHandler.post(() -> { - try { - NetworkStats.Entry entry = new NetworkStats.Entry(); - entry.set = SET_DEFAULT; - entry.tag = TAG_NONE; - entry.uid = UID_TETHERING; + // 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(() -> { + NetworkStats.Entry entry = new NetworkStats.Entry(); + entry.set = SET_DEFAULT; + entry.tag = TAG_NONE; + entry.uid = UID_TETHERING; - updateStatsForCurrentUpstream(); + updateStatsForCurrentUpstream(); - for (String iface : mForwardedStats.keySet()) { - entry.iface = iface; - entry.rxBytes = mForwardedStats.get(iface).rxBytes; - entry.txBytes = mForwardedStats.get(iface).txBytes; - stats.addValues(entry); - } - } finally { - latch.countDown(); + for (String iface : mForwardedStats.keySet()) { + entry.iface = iface; + entry.rxBytes = mForwardedStats.get(iface).rxBytes; + entry.txBytes = mForwardedStats.get(iface).txBytes; + stats.addValues(entry); } - }); - - try { - latch.await(STATS_FETCH_TIMEOUT_MS, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - mLog.e("Tethering stats fetch timed out after " + STATS_FETCH_TIMEOUT_MS + "ms"); - } + }, 0); return stats; } From 62801ec2b7d2c56d9c092324950b320fd3f62c51 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 11 Aug 2017 13:47:49 +0900 Subject: [PATCH 2/2] Pass data usage limits to tethering offload code. (cherry picked from commit 50b60fc34dcf879007b5ab6e9b67f806e5a57215) Bug: 29337859 Bug: 32163131 Test: builds Test: OffloadControllerTest passes Change-Id: Ifbd1e8d9057aa12b956e3b4501c32aa6bcf420bd --- .../android/net/ITetheringStatsProvider.aidl | 11 ++- .../server/NetworkManagementService.java | 27 +++++++ .../tethering/OffloadController.java | 50 ++++++++++++- .../tethering/OffloadHardwareInterface.java | 21 ++++++ .../tethering/OffloadControllerTest.java | 72 +++++++++++++++++++ 5 files changed, 179 insertions(+), 2 deletions(-) diff --git a/core/java/android/net/ITetheringStatsProvider.aidl b/core/java/android/net/ITetheringStatsProvider.aidl index 769086da42b47..1aeabc1e62de2 100644 --- a/core/java/android/net/ITetheringStatsProvider.aidl +++ b/core/java/android/net/ITetheringStatsProvider.aidl @@ -19,7 +19,7 @@ package android.net; import android.net.NetworkStats; /** - * Interface that allows NetworkManagementService to query for tethering statistics. + * Interface for NetworkManagementService to query tethering statistics and set data limits. * * TODO: this does not really need to be an interface since Tethering runs in the same process * as NetworkManagementService. Consider refactoring Tethering to use direct access to @@ -29,5 +29,14 @@ import android.net.NetworkStats; * @hide */ interface ITetheringStatsProvider { + // Returns cumulative statistics for all tethering sessions since boot, on all upstreams. NetworkStats getTetherStats(); + + // Sets the interface quota for the specified upstream interface. This is defined as the number + // of bytes, starting from zero and counting from now, after which data should stop being + // forwarded to/from the specified upstream. A value of QUOTA_UNLIMITED means there is no limit. + void setInterfaceQuota(String iface, long quotaBytes); + + // Indicates that no data usage limit is set. + const int QUOTA_UNLIMITED = -1; } diff --git a/services/core/java/com/android/server/NetworkManagementService.java b/services/core/java/com/android/server/NetworkManagementService.java index ac4423dbba6b0..3a4c9f805219c 100644 --- a/services/core/java/com/android/server/NetworkManagementService.java +++ b/services/core/java/com/android/server/NetworkManagementService.java @@ -1569,6 +1569,17 @@ public class NetworkManagementService extends INetworkManagementService.Stub } catch (NativeDaemonConnectorException e) { throw e.rethrowAsParcelableException(); } + + synchronized (mTetheringStatsProviders) { + for (ITetheringStatsProvider provider : mTetheringStatsProviders.keySet()) { + try { + provider.setInterfaceQuota(iface, quotaBytes); + } catch (RemoteException e) { + Log.e(TAG, "Problem setting tethering data limit on provider " + + mTetheringStatsProviders.get(provider) + ": " + e); + } + } + } } } @@ -1595,6 +1606,17 @@ public class NetworkManagementService extends INetworkManagementService.Stub } catch (NativeDaemonConnectorException e) { throw e.rethrowAsParcelableException(); } + + synchronized (mTetheringStatsProviders) { + for (ITetheringStatsProvider provider : mTetheringStatsProviders.keySet()) { + try { + provider.setInterfaceQuota(iface, ITetheringStatsProvider.QUOTA_UNLIMITED); + } catch (RemoteException e) { + Log.e(TAG, "Problem removing tethering data limit on provider " + + mTetheringStatsProviders.get(provider) + ": " + e); + } + } + } } } @@ -1853,6 +1875,11 @@ public class NetworkManagementService extends INetworkManagementService.Stub } return stats; } + + @Override + public void setInterfaceQuota(String iface, long quotaBytes) { + // Do nothing. netd is already informed of quota changes in setInterfaceQuota. + } } @Override 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 ccbac31c82c31..55e290a4215e2 100644 --- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java +++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java @@ -73,9 +73,17 @@ public class OffloadController { private Set mLastLocalPrefixStrs; // 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 mForwardedStats = new HashMap<>(); + // Maps upstream interface names to interface quotas. + // Always contains the latest value received from the framework for each interface, regardless + // of whether offload is currently running (or is even supported) on that interface. Only + // includes upstream interfaces that have a quota set. + private HashMap mInterfaceQuotas = new HashMap<>(); + public OffloadController(Handler h, OffloadHardwareInterface hwi, ContentResolver contentResolver, INetworkManagementService nms, SharedLog log) { mHandler = h; @@ -195,6 +203,17 @@ public class OffloadController { return stats; } + + public void setInterfaceQuota(String iface, long quotaBytes) { + mHandler.post(() -> { + if (quotaBytes == ITetheringStatsProvider.QUOTA_UNLIMITED) { + mInterfaceQuotas.remove(iface); + } else { + mInterfaceQuotas.put(iface, quotaBytes); + } + maybeUpdateDataLimit(iface); + }); + } } private void maybeUpdateStats(String iface) { @@ -208,6 +227,22 @@ public class OffloadController { mForwardedStats.get(iface).add(mHwInterface.getForwardedStats(iface)); } + private boolean maybeUpdateDataLimit(String iface) { + // setDataLimit may only be called while offload is occuring on this upstream. + if (!started() || + mUpstreamLinkProperties == null || + !TextUtils.equals(iface, mUpstreamLinkProperties.getInterfaceName())) { + return true; + } + + Long limit = mInterfaceQuotas.get(iface); + if (limit == null) { + limit = Long.MAX_VALUE; + } + + return mHwInterface.setDataLimit(iface, limit); + } + private void updateStatsForCurrentUpstream() { if (mUpstreamLinkProperties != null) { maybeUpdateStats(mUpstreamLinkProperties.getInterfaceName()); @@ -297,8 +332,21 @@ public class OffloadController { } } - return mHwInterface.setUpstreamParameters( + boolean success = mHwInterface.setUpstreamParameters( iface, v4addr, v4gateway, (v6gateways.isEmpty() ? null : v6gateways)); + + if (!success) { + return success; + } + + // Data limits can only be set once offload is running on the upstream. + success = maybeUpdateDataLimit(iface); + if (!success) { + mLog.log("Setting data limit for " + iface + " failed, disabling offload."); + stop(); + } + + return success; } private boolean computeAndPushLocalPrefixes() { diff --git a/services/core/java/com/android/server/connectivity/tethering/OffloadHardwareInterface.java b/services/core/java/com/android/server/connectivity/tethering/OffloadHardwareInterface.java index 25fab9e403a19..cf5685d928d2f 100644 --- a/services/core/java/com/android/server/connectivity/tethering/OffloadHardwareInterface.java +++ b/services/core/java/com/android/server/connectivity/tethering/OffloadHardwareInterface.java @@ -188,6 +188,27 @@ public class OffloadHardwareInterface { return results.success; } + public boolean setDataLimit(String iface, long limit) { + + final String logmsg = String.format("setDataLimit(%s, %d)", iface, limit); + + final CbResults results = new CbResults(); + try { + mOffloadControl.setDataLimit( + iface, limit, + (boolean success, String errMsg) -> { + results.success = success; + results.errMsg = errMsg; + }); + } catch (RemoteException e) { + record(logmsg, e); + return false; + } + + record(logmsg, results); + return results.success; + } + public boolean setUpstreamParameters( String iface, String v4addr, String v4gateway, ArrayList v6gws) { iface = (iface != null) ? iface : NO_INTERFACE_NAME; 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 dcb9723fa5e2e..c4965a0b5cde9 100644 --- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java +++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; @@ -45,6 +46,7 @@ import android.net.LinkProperties; import android.net.NetworkStats; import android.net.RouteInfo; import android.net.util.SharedLog; +import android.os.ConditionVariable; import android.os.Handler; import android.os.Looper; import android.os.INetworkManagementService; @@ -110,6 +112,12 @@ public class OffloadControllerTest { Settings.Global.putInt(mContentResolver, TETHER_OFFLOAD_DISABLED, 0); } + private void waitForIdle() { + ConditionVariable cv = new ConditionVariable(); + new Handler(Looper.getMainLooper()).post(() -> { cv.open(); }); + cv.block(); + } + private OffloadController makeOffloadController() throws Exception { OffloadController offload = new OffloadController(new Handler(Looper.getMainLooper()), mHardware, mContentResolver, mNMService, new SharedLog("test")); @@ -417,4 +425,68 @@ public class OffloadControllerTest { entry = stats.getValues(ethernetPosition, entry); assertNetworkStats(ethernetIface, ethernetStats, entry); } + + @Test + public void testSetInterfaceQuota() throws Exception { + setupFunctioningHardwareInterface(); + enableOffload(); + + final OffloadController offload = makeOffloadController(); + offload.start(); + + final String ethernetIface = "eth1"; + final String mobileIface = "rmnet_data0"; + final long ethernetLimit = 12345; + final long mobileLimit = 12345678; + + final LinkProperties lp = new LinkProperties(); + lp.setInterfaceName(ethernetIface); + offload.setUpstreamLinkProperties(lp); + + ITetheringStatsProvider provider = mTetherStatsProviderCaptor.getValue(); + final InOrder inOrder = inOrder(mHardware); + when(mHardware.setUpstreamParameters(any(), any(), any(), any())).thenReturn(true); + when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(true); + + // Applying an interface quota to the current upstream immediately sends it to the hardware. + provider.setInterfaceQuota(ethernetIface, ethernetLimit); + waitForIdle(); + inOrder.verify(mHardware).setDataLimit(ethernetIface, ethernetLimit); + inOrder.verifyNoMoreInteractions(); + + // Applying an interface quota to another upstream does not take any immediate action. + provider.setInterfaceQuota(mobileIface, mobileLimit); + waitForIdle(); + inOrder.verify(mHardware, never()).setDataLimit(anyString(), anyLong()); + + // Switching to that upstream causes the quota to be applied if the parameters were applied + // correctly. + lp.setInterfaceName(mobileIface); + offload.setUpstreamLinkProperties(lp); + waitForIdle(); + inOrder.verify(mHardware).setDataLimit(mobileIface, mobileLimit); + + // Setting a limit of ITetheringStatsProvider.QUOTA_UNLIMITED causes the limit to be set + // to Long.MAX_VALUE. + provider.setInterfaceQuota(mobileIface, ITetheringStatsProvider.QUOTA_UNLIMITED); + waitForIdle(); + inOrder.verify(mHardware).setDataLimit(mobileIface, Long.MAX_VALUE); + + // If setting upstream parameters fails, then the data limit is not set. + when(mHardware.setUpstreamParameters(any(), any(), any(), any())).thenReturn(false); + lp.setInterfaceName(ethernetIface); + offload.setUpstreamLinkProperties(lp); + provider.setInterfaceQuota(mobileIface, mobileLimit); + waitForIdle(); + inOrder.verify(mHardware, never()).setDataLimit(anyString(), anyLong()); + + // If setting the data limit fails while changing upstreams, offload is stopped. + when(mHardware.setUpstreamParameters(any(), any(), any(), any())).thenReturn(true); + when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(false); + lp.setInterfaceName(mobileIface); + offload.setUpstreamLinkProperties(lp); + provider.setInterfaceQuota(mobileIface, mobileLimit); + waitForIdle(); + inOrder.verify(mHardware).stopOffloadControl(); + } }