From a530ebb62decfaf73e19df14511b5ba46173dbce Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Fri, 24 Apr 2020 12:19:37 +0000 Subject: [PATCH 1/4] Address comments on NetworkStack AIDL v6 Address issues found during AIDL review: - Rename clientAddr to singleClientAddr - Do not use a ParcelableBundle for notifyNetworkTested or notifyDataStallSuspected; instead use AIDL parcelables for stronger backwards compatibility guarantees. Test: atest NetworkMonitorTest ConnectivityServiceTest ConnectivityServiceIntegrationTest, manual Bug: 153500847 Merged-In: Id9b71784e5f6294d203230e57737979e063ff0f8 Change-Id: Id9b71784e5f6294d203230e57737979e063ff0f8 --- Tethering/src/android/net/dhcp/DhcpServingParamsParcelExt.java | 2 +- .../src/android/net/dhcp/DhcpServingParamsParcelExtTest.java | 2 +- .../src/com/android/networkstack/tethering/TetheringTest.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Tethering/src/android/net/dhcp/DhcpServingParamsParcelExt.java b/Tethering/src/android/net/dhcp/DhcpServingParamsParcelExt.java index 82a26beada..4f8ad8a221 100644 --- a/Tethering/src/android/net/dhcp/DhcpServingParamsParcelExt.java +++ b/Tethering/src/android/net/dhcp/DhcpServingParamsParcelExt.java @@ -169,7 +169,7 @@ public class DhcpServingParamsParcelExt extends DhcpServingParamsParcel { *

If not set, the default value is null. */ public DhcpServingParamsParcelExt setSingleClientAddr(@Nullable Inet4Address clientAddr) { - this.clientAddr = clientAddr == null ? 0 : inet4AddressToIntHTH(clientAddr); + this.singleClientAddr = clientAddr == null ? 0 : inet4AddressToIntHTH(clientAddr); return this; } diff --git a/Tethering/tests/unit/src/android/net/dhcp/DhcpServingParamsParcelExtTest.java b/Tethering/tests/unit/src/android/net/dhcp/DhcpServingParamsParcelExtTest.java index f8eb1476ba..a8857b2e5c 100644 --- a/Tethering/tests/unit/src/android/net/dhcp/DhcpServingParamsParcelExtTest.java +++ b/Tethering/tests/unit/src/android/net/dhcp/DhcpServingParamsParcelExtTest.java @@ -110,7 +110,7 @@ public class DhcpServingParamsParcelExtTest { @Test public void testSetClientAddr() { mParcel.setSingleClientAddr(TEST_CLIENT_ADDRESS); - assertEquals(TEST_CLIENT_ADDRESS_PARCELED, mParcel.clientAddr); + assertEquals(TEST_CLIENT_ADDRESS_PARCELED, mParcel.singleClientAddr); } private static Inet4Address inet4Addr(String addr) { diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index 0c86eeb6cd..0363f5f998 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -1689,7 +1689,7 @@ public class TetheringTest { final DhcpServingParamsParcel params = dhcpParamsCaptor.getValue(); assertEquals(serverAddr, intToInet4AddressHTH(params.serverAddr).getHostAddress()); assertEquals(24, params.serverAddrPrefixLength); - assertEquals(clientAddrParceled, params.clientAddr); + assertEquals(clientAddrParceled, params.singleClientAddr); } @Test From bf6fa99b2aebddac4ac13f65fcbd73d253b2522c Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Wed, 29 Apr 2020 12:25:33 +0000 Subject: [PATCH 2/4] [SP18] Poll network stats in OffloadController to support data warning The OEM implemented tether offload does not support data warning since the HAL only tells the hardware about data limit but not warning. However, to add such interface in HAL needs OEM to comply and implement in hardware. Thus, as a short-term solution, polls network statistics from HAL and notify upper layer when it reaches the alert quota set by NetworkStatsService. Note that when CPU is sleeping, the data warning of tethering offload will not work since the polling is also suspended. Test: manual Test: atest OffloadControllerTest Bug: 149467454 Change-Id: I2467b64779b74cd5fec73b42fb303584f52cb1cb Merged-In: I2467b64779b74cd5fec73b42fb303584f52cb1cb (cherry picked from commit 93660e382c7717c4a3e05dafdf917654fababeae) --- .../tethering/OffloadController.java | 74 +++++++++++++++++++ .../tethering/OffloadHardwareInterface.java | 1 - 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/Tethering/src/com/android/networkstack/tethering/OffloadController.java b/Tethering/src/com/android/networkstack/tethering/OffloadController.java index c007c174fe..445a09d761 100644 --- a/Tethering/src/com/android/networkstack/tethering/OffloadController.java +++ b/Tethering/src/com/android/networkstack/tethering/OffloadController.java @@ -23,6 +23,7 @@ import static android.net.NetworkStats.SET_DEFAULT; import static android.net.NetworkStats.TAG_NONE; import static android.net.NetworkStats.UID_ALL; import static android.net.NetworkStats.UID_TETHERING; +import static android.net.netstats.provider.NetworkStatsProvider.QUOTA_UNLIMITED; import static android.provider.Settings.Global.TETHER_OFFLOAD_DISABLED; import android.annotation.NonNull; @@ -76,6 +77,7 @@ public class OffloadController { private static final boolean DBG = false; private static final String ANYIP = "0.0.0.0"; private static final ForwardedStats EMPTY_STATS = new ForwardedStats(); + private static final int DEFAULT_PERFORM_POLL_INTERVAL_MS = 5000; @VisibleForTesting enum StatsType { @@ -115,6 +117,16 @@ public class OffloadController { // includes upstream interfaces that have a quota set. private HashMap mInterfaceQuotas = new HashMap<>(); + // Tracking remaining alert quota. Unlike limit quota is subject to interface, the alert + // quota is interface independent and global for tether offload. Note that this is only + // accessed on the handler thread and in the constructor. + private long mRemainingAlertQuota = QUOTA_UNLIMITED; + // Runnable that used to schedule the next stats poll. + private final Runnable mScheduledPollingTask = () -> { + updateStatsForCurrentUpstream(); + maybeSchedulePollingStats(); + }; + private int mNatUpdateCallbacksReceived; private int mNatUpdateNetlinkErrors; @@ -240,6 +252,7 @@ public class OffloadController { mLog.log("tethering offload started"); mNatUpdateCallbacksReceived = 0; mNatUpdateNetlinkErrors = 0; + maybeSchedulePollingStats(); } return isStarted; } @@ -255,6 +268,9 @@ public class OffloadController { mHwInterface.stopOffloadControl(); mControlInitialized = false; mConfigInitialized = false; + if (mHandler.hasCallbacks(mScheduledPollingTask)) { + mHandler.removeCallbacks(mScheduledPollingTask); + } if (wasStarted) mLog.log("tethering offload stopped"); } @@ -345,6 +361,11 @@ public class OffloadController { @Override public void onSetAlert(long quotaBytes) { // TODO: Ask offload HAL to notify alert without stopping traffic. + // Post it to handler thread since it access remaining quota bytes. + mHandler.post(() -> { + updateAlertQuota(quotaBytes); + maybeSchedulePollingStats(); + }); } } @@ -366,15 +387,66 @@ public class OffloadController { // the stats for each interface, and does not observe partial writes where rxBytes is // updated and txBytes is not. ForwardedStats diff = mHwInterface.getForwardedStats(iface); + final long usedAlertQuota = diff.rxBytes + diff.txBytes; ForwardedStats base = mForwardedStats.get(iface); if (base != null) { diff.add(base); } + + // Update remaining alert quota if it is still positive. + if (mRemainingAlertQuota > 0 && usedAlertQuota > 0) { + // Trim to zero if overshoot. + final long newQuota = Math.max(mRemainingAlertQuota - usedAlertQuota, 0); + updateAlertQuota(newQuota); + } + 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. } + /** + * Update remaining alert quota, fire the {@link NetworkStatsProvider#notifyAlertReached()} + * callback when it reaches zero. This can be invoked either from service setting the alert, or + * {@code maybeUpdateStats} when updating stats. Note that this can be only called on + * handler thread. + * + * @param newQuota non-negative value to indicate the new quota, or + * {@link NetworkStatsProvider#QUOTA_UNLIMITED} to indicate there is no + * quota. + */ + private void updateAlertQuota(long newQuota) { + if (newQuota < QUOTA_UNLIMITED) { + throw new IllegalArgumentException("invalid quota value " + newQuota); + } + if (mRemainingAlertQuota == newQuota) return; + + mRemainingAlertQuota = newQuota; + if (mRemainingAlertQuota == 0) { + mLog.i("notifyAlertReached"); + if (mStatsProvider != null) mStatsProvider.notifyAlertReached(); + } + } + + /** + * Schedule polling if needed, this will be stopped if offload has been + * stopped or remaining quota reaches zero or upstream is empty. + * Note that this can be only called on handler thread. + */ + private void maybeSchedulePollingStats() { + if (!isPollingStatsNeeded()) return; + + if (mHandler.hasCallbacks(mScheduledPollingTask)) { + mHandler.removeCallbacks(mScheduledPollingTask); + } + mHandler.postDelayed(mScheduledPollingTask, DEFAULT_PERFORM_POLL_INTERVAL_MS); + } + + private boolean isPollingStatsNeeded() { + return started() && mRemainingAlertQuota > 0 + && !TextUtils.isEmpty(currentUpstreamInterface()); + } + private boolean maybeUpdateDataLimit(String iface) { // setDataLimit may only be called while offload is occurring on this upstream. if (!started() || !TextUtils.equals(iface, currentUpstreamInterface())) { @@ -414,6 +486,8 @@ public class OffloadController { final String iface = currentUpstreamInterface(); if (!TextUtils.isEmpty(iface)) mForwardedStats.putIfAbsent(iface, EMPTY_STATS); + maybeSchedulePollingStats(); + // TODO: examine return code and decide what to do if programming // upstream parameters fails (probably just wait for a subsequent // onOffloadEvent() callback to tell us offload is available again and diff --git a/Tethering/src/com/android/networkstack/tethering/OffloadHardwareInterface.java b/Tethering/src/com/android/networkstack/tethering/OffloadHardwareInterface.java index c4a1078d0b..293f8eae32 100644 --- a/Tethering/src/com/android/networkstack/tethering/OffloadHardwareInterface.java +++ b/Tethering/src/com/android/networkstack/tethering/OffloadHardwareInterface.java @@ -308,7 +308,6 @@ public class OffloadHardwareInterface { return stats; } - mLog.log(logmsg + YIELDS + stats); return stats; } From 40092b1fa75292d9b9641f5e831093272663ccdc Mon Sep 17 00:00:00 2001 From: Treehugger Robot Date: Wed, 29 Apr 2020 13:28:12 +0000 Subject: [PATCH 3/4] [SP18.1] add dependency object to OffloadController In order to mock constant in unit test, a dependency object is introduced with minimum code change to achieve this. Test: atest TetheringTests Bug: 149467454 Change-Id: I38628daddcb7be7c74846e78d36dc88f065b97d9 Merged-In: I38628daddcb7be7c74846e78d36dc88f065b97d9 (cherry picked from commit 29aee20bfafce3ca8a5477398b953a06fa2d4823) --- .../tethering/OffloadController.java | 17 +++++++++++++++-- .../networkstack/tethering/Tethering.java | 2 +- .../tethering/OffloadControllerTest.java | 8 +++++++- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/OffloadController.java b/Tethering/src/com/android/networkstack/tethering/OffloadController.java index 445a09d761..1817f35f1d 100644 --- a/Tethering/src/com/android/networkstack/tethering/OffloadController.java +++ b/Tethering/src/com/android/networkstack/tethering/OffloadController.java @@ -130,8 +130,20 @@ public class OffloadController { private int mNatUpdateCallbacksReceived; private int mNatUpdateNetlinkErrors; + @NonNull + private final Dependencies mDeps; + + // TODO: Put more parameters in constructor into dependency object. + static class Dependencies { + int getPerformPollInterval() { + // TODO: Consider make this configurable. + return DEFAULT_PERFORM_POLL_INTERVAL_MS; + } + } + public OffloadController(Handler h, OffloadHardwareInterface hwi, - ContentResolver contentResolver, NetworkStatsManager nsm, SharedLog log) { + ContentResolver contentResolver, NetworkStatsManager nsm, SharedLog log, + @NonNull Dependencies deps) { mHandler = h; mHwInterface = hwi; mContentResolver = contentResolver; @@ -147,6 +159,7 @@ public class OffloadController { provider = null; } mStatsProvider = provider; + mDeps = deps; } /** Start hardware offload. */ @@ -439,7 +452,7 @@ public class OffloadController { if (mHandler.hasCallbacks(mScheduledPollingTask)) { mHandler.removeCallbacks(mScheduledPollingTask); } - mHandler.postDelayed(mScheduledPollingTask, DEFAULT_PERFORM_POLL_INTERVAL_MS); + mHandler.postDelayed(mScheduledPollingTask, mDeps.getPerformPollInterval()); } private boolean isPollingStatsNeeded() { diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 4e16c49caa..0a95a5e007 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -273,7 +273,7 @@ public class Tethering { mHandler = mTetherMasterSM.getHandler(); mOffloadController = new OffloadController(mHandler, mDeps.getOffloadHardwareInterface(mHandler, mLog), mContext.getContentResolver(), - statsManager, mLog); + statsManager, mLog, new OffloadController.Dependencies()); mUpstreamNetworkMonitor = mDeps.getUpstreamNetworkMonitor(mContext, mTetherMasterSM, mLog, TetherMasterSM.EVENT_UPSTREAM_CALLBACK); mForwardedDownstreams = new LinkedHashSet<>(); diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/OffloadControllerTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/OffloadControllerTest.java index 65797200fa..088a663190 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/OffloadControllerTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/OffloadControllerTest.java @@ -116,6 +116,12 @@ public class OffloadControllerTest { private final ArgumentCaptor mControlCallbackCaptor = ArgumentCaptor.forClass(OffloadHardwareInterface.ControlCallback.class); private MockContentResolver mContentResolver; + private OffloadController.Dependencies mDeps = new OffloadController.Dependencies() { + @Override + int getPerformPollInterval() { + return 0; + } + }; @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -150,7 +156,7 @@ public class OffloadControllerTest { private OffloadController makeOffloadController() throws Exception { OffloadController offload = new OffloadController(new Handler(Looper.getMainLooper()), - mHardware, mContentResolver, mStatsManager, new SharedLog("test")); + mHardware, mContentResolver, mStatsManager, new SharedLog("test"), mDeps); final ArgumentCaptor tetherStatsProviderCaptor = ArgumentCaptor.forClass(OffloadController.OffloadTetheringStatsProvider.class); From dc8e0fc1a11d53e54fa2d318872d1ca85e006960 Mon Sep 17 00:00:00 2001 From: Anton Hansson Date: Thu, 30 Apr 2020 17:02:07 +0100 Subject: [PATCH 4/4] Fix tethering module lib stub default It was using the systemapi stub defaults, but should be using the module_lib default. Bug: 144149403 Test: m Change-Id: Iaab154d9d71900284d92d518a086fc1227c00d5c --- Tethering/common/TetheringLib/Android.bp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tethering/common/TetheringLib/Android.bp b/Tethering/common/TetheringLib/Android.bp index ee6b9f12f9..b520bc8808 100644 --- a/Tethering/common/TetheringLib/Android.bp +++ b/Tethering/common/TetheringLib/Android.bp @@ -134,5 +134,5 @@ java_library { java_library { name: "framework-tethering-stubs-module_libs_api", srcs: [":framework-tethering-stubs-srcs-module_libs_api"], - defaults: ["framework-module-stubs-lib-defaults-systemapi"], + defaults: ["framework-module-stubs-lib-defaults-module_libs_api"], }