From b104cd7c1245095286d0fa224c0f47e4bea357d4 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:32:12 +0000 Subject: [PATCH 1/8] Revert "[NS D07] Use the unmodified legacy score" Revert submission 10343065 Reason for revert: This is part of a feature that was punted out of R. Reverted Changes: Ic9a3d3363:[NS D05] Rework how to tear down networks I7d815f873:[NS D06] Implement more policies I561098476:[NS D07] Use the unmodified legacy score Change-Id: I184817e3aa290afdbe6721a7c36332b940434d3a --- .../java/com/android/server/connectivity/NetworkRanker.java | 2 +- tests/net/java/com/android/server/ConnectivityServiceTest.java | 2 +- .../java/com/android/server/connectivity/NetworkRankerTest.kt | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index 80d46e0370..fd5a4e82de 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -58,7 +58,7 @@ public class NetworkRanker { NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; for (final NetworkAgentInfo nai : candidates) { - final int score = nai.getNetworkScore().getLegacyScore(); + final int score = nai.getCurrentScore(); if (score > bestScore) { bestNetwork = nai; bestScore = score; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 220cdce0d1..5aabc60b22 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5846,7 +5846,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); - trustedCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); + trustedCallback.expectAvailableDoubleValidatedCallbacks(mWiFiNetworkAgent); verify(mNetworkManagementService).setDefaultNetId(eq(mWiFiNetworkAgent.getNetwork().netId)); reset(mNetworkManagementService); diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt index d2532c2ce3..2b0c2c7215 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -28,6 +28,8 @@ import androidx.test.filters.SmallTest import androidx.test.runner.AndroidJUnit4 import org.junit.Test import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.any +import org.mockito.Mockito.doReturn import org.mockito.Mockito.mock import kotlin.test.assertEquals import kotlin.test.assertNull From 751e5a1c37c0592189c0f30edb86d1e0d8623d50 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:32:12 +0000 Subject: [PATCH 2/8] Revert "[NS D06] Implement more policies" Revert submission 10343065 Reason for revert: This is part of a feature that was punted out of R. Reverted Changes: Ic9a3d3363:[NS D05] Rework how to tear down networks I7d815f873:[NS D06] Implement more policies I561098476:[NS D07] Use the unmodified legacy score Change-Id: I378449443f99eb0a7f01f31f53398b8f55ce87f1 --- .../server/connectivity/NetworkRanker.java | 48 ++----------------- .../server/connectivity/NetworkRankerTest.kt | 28 ++--------- 2 files changed, 8 insertions(+), 68 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index fd5a4e82de..c536ab25e9 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -16,9 +16,6 @@ package com.android.server.connectivity; -import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; -import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; -import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkScore.POLICY_IGNORE_ON_WIFI; import static com.android.internal.util.FunctionalUtils.findFirst; @@ -45,15 +42,8 @@ public class NetworkRanker { @NonNull final Collection nais) { final ArrayList candidates = new ArrayList<>(nais); candidates.removeIf(nai -> !nai.satisfies(request)); - - // Enforce policy. The order in which the policy is computed is essential, because each - // step may remove some of the candidates. For example, filterValidated drops non-validated - // networks in presence of validated networks for INTERNET requests, but the bad wifi - // avoidance policy takes priority over this, so it must be done before. - filterVpn(candidates); - filterExplicitlySelected(candidates); - filterBadWifiAvoidance(candidates); - filterValidated(request, candidates); + // Enforce policy. + filterBadWifiAvoidancePolicy(candidates); NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; @@ -67,27 +57,9 @@ public class NetworkRanker { return bestNetwork; } - // If a network is a VPN it has priority. - private void filterVpn(@NonNull final ArrayList candidates) { - final NetworkAgentInfo vpn = findFirst(candidates, - nai -> nai.networkCapabilities.hasTransport(TRANSPORT_VPN)); - if (null == vpn) return; // No VPN : this policy doesn't apply. - candidates.removeIf(nai -> !nai.networkCapabilities.hasTransport(TRANSPORT_VPN)); - } - - // If some network is explicitly selected and set to accept unvalidated connectivity, then - // drop all networks that are not explicitly selected. - private void filterExplicitlySelected( - @NonNull final ArrayList candidates) { - final NetworkAgentInfo explicitlySelected = findFirst(candidates, - nai -> nai.networkAgentConfig.explicitlySelected - && nai.networkAgentConfig.acceptUnvalidated); - if (null == explicitlySelected) return; // No explicitly selected network accepting unvalid - candidates.removeIf(nai -> !nai.networkAgentConfig.explicitlySelected); - } - // If some network with wifi transport is present, drop all networks with POLICY_IGNORE_ON_WIFI. - private void filterBadWifiAvoidance(@NonNull final ArrayList candidates) { + private void filterBadWifiAvoidancePolicy( + @NonNull final ArrayList candidates) { final NetworkAgentInfo wifi = findFirst(candidates, nai -> nai.networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) && nai.everValidated @@ -99,16 +71,4 @@ public class NetworkRanker { if (null == wifi) return; // No wifi : this policy doesn't apply candidates.removeIf(nai -> nai.getNetworkScore().hasPolicy(POLICY_IGNORE_ON_WIFI)); } - - // If some network is validated and the request asks for INTERNET, drop all networks that are - // not validated. - private void filterValidated(@NonNull final NetworkRequest request, - @NonNull final ArrayList candidates) { - if (!request.hasCapability(NET_CAPABILITY_INTERNET)) return; - final NetworkAgentInfo validated = findFirst(candidates, - nai -> nai.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)); - if (null == validated) return; // No validated network - candidates.removeIf(nai -> - !nai.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)); - } } diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt index 2b0c2c7215..a6b371a23b 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -16,14 +16,8 @@ package com.android.server.connectivity -import android.net.ConnectivityManager.TYPE_WIFI -import android.net.LinkProperties -import android.net.Network -import android.net.NetworkAgentConfig import android.net.NetworkCapabilities -import android.net.NetworkInfo import android.net.NetworkRequest -import android.net.NetworkScore import androidx.test.filters.SmallTest import androidx.test.runner.AndroidJUnit4 import org.junit.Test @@ -39,24 +33,10 @@ import kotlin.test.assertNull class NetworkRankerTest { private val ranker = NetworkRanker() - private fun makeNai(satisfy: Boolean, score: Int) = object : NetworkAgentInfo( - null /* messenger */, - null /* asyncChannel*/, - Network(100), - NetworkInfo(TYPE_WIFI, 0 /* subtype */, "" /* typename */, "" /* subtypename */), - LinkProperties(), - NetworkCapabilities(), - NetworkScore.Builder().setLegacyScore(score).build(), - null /* context */, - null /* handler */, - NetworkAgentConfig(), - null /* connectivityService */, - null /* netd */, - null /* dnsResolver */, - null /* networkManagementService */, - 0 /* factorySerialNumber */) { - override fun satisfies(request: NetworkRequest?): Boolean = satisfy - override fun getCurrentScore(): Int = score + private fun makeNai(satisfy: Boolean, score: Int) = mock(NetworkAgentInfo::class.java).also { + doReturn(satisfy).`when`(it).satisfies(any()) + doReturn(score).`when`(it).currentScore + it.networkCapabilities = NetworkCapabilities() } @Test From b3ce7501b78065a19d8e57283bf7f3c699b99648 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:32:12 +0000 Subject: [PATCH 3/8] Revert "[NS D05] Rework how to tear down networks" Revert submission 10343065 Reason for revert: This is part of a feature that was punted out of R. Reverted Changes: Ic9a3d3363:[NS D05] Rework how to tear down networks I7d815f873:[NS D06] Implement more policies I561098476:[NS D07] Use the unmodified legacy score Change-Id: I8d2696d15999265d79abfc9163e7e5ccad873cfe --- .../android/server/ConnectivityService.java | 20 +++--- .../server/connectivity/NetworkAgentInfo.java | 67 +++++++------------ .../server/ConnectivityServiceTest.java | 9 ++- 3 files changed, 39 insertions(+), 57 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index f7eabac3b2..7083281eaa 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3430,16 +3430,16 @@ public class ConnectivityService extends IConnectivityManager.Stub // there is hope for it to become one if it validated, then it is needed. ensureRunningOnConnectivityServiceThread(); if (nri.request.isRequest() && nai.satisfies(nri.request) && - (nai.isSatisfyingRequest(nri.request.requestId) - // Note that canPossiblyBeat catches two important cases: - // 1. Unvalidated slow networks will not be reaped when an unvalidated fast - // network is currently satisfying the request. This is desirable for example - // when cellular ends up validating but WiFi/Ethernet does not. - // 2. Fast networks will not be reaped when a validated slow network is - // currently satisfying the request. This is desirable for example when - // Ethernet ends up validating and out scoring WiFi, or WiFi/Ethernet ends - // up validating and out scoring cellular. - || nai.canPossiblyBeat(nri.mSatisfier))) { + (nai.isSatisfyingRequest(nri.request.requestId) || + // Note that this catches two important cases: + // 1. Unvalidated cellular will not be reaped when unvalidated WiFi + // is currently satisfying the request. This is desirable when + // cellular ends up validating but WiFi does not. + // 2. Unvalidated WiFi will not be reaped when validated cellular + // is currently satisfying the request. This is desirable when + // WiFi ends up validating and out scoring cellular. + nri.mSatisfier.getCurrentScore() + < nai.getCurrentScoreAsValidated())) { return false; } } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 3860904a3f..4612cfd0f7 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -16,8 +16,6 @@ package com.android.server.connectivity; -import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED; -import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; import static android.net.NetworkCapabilities.transportNamesOf; import android.annotation.NonNull; @@ -477,16 +475,24 @@ public class NetworkAgentInfo implements Comparable { return networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN); } - /** Gets the current score */ - public int getCurrentScore() { + private int getCurrentScore(boolean pretendValidated) { + // TODO: We may want to refactor this into a NetworkScore class that takes a base score from + // the NetworkAgent and signals from the NetworkAgent and uses those signals to modify the + // score. The NetworkScore class would provide a nice place to centralize score constants + // so they are not scattered about the transports. + // If this network is explicitly selected and the user has decided to use it even if it's - // unvalidated, give it the maximum score. - if (networkAgentConfig.explicitlySelected && networkAgentConfig.acceptUnvalidated) { + // unvalidated, give it the maximum score. Also give it the maximum score if it's explicitly + // selected and we're trying to see what its score could be. This ensures that we don't tear + // down an explicitly selected network before the user gets a chance to prefer it when + // a higher-scoring network (e.g., Ethernet) is available. + if (networkAgentConfig.explicitlySelected + && (networkAgentConfig.acceptUnvalidated || pretendValidated)) { return ConnectivityConstants.EXPLICITLY_SELECTED_NETWORK_SCORE; } int score = mNetworkScore.getLegacyScore(); - if (!lastValidated && !ignoreWifiUnvalidationPenalty() && !isVPN()) { + if (!lastValidated && !pretendValidated && !ignoreWifiUnvalidationPenalty() && !isVPN()) { score -= ConnectivityConstants.UNVALIDATED_SCORE_PENALTY; } if (score < 0) score = 0; @@ -502,6 +508,18 @@ public class NetworkAgentInfo implements Comparable { return isWifi && !avoidBadWifi && everValidated; } + // Get the current score for this Network. This may be modified from what the + // NetworkAgent sent, as it has modifiers applied to it. + public int getCurrentScore() { + return getCurrentScore(false); + } + + // Get the current score for this Network as if it was validated. This may be modified from + // what the NetworkAgent sent, as it has modifiers applied to it. + public int getCurrentScoreAsValidated() { + return getCurrentScore(true); + } + public void setNetworkScore(@NonNull NetworkScore ns) { mNetworkScore = ns; } @@ -611,41 +629,6 @@ public class NetworkAgentInfo implements Comparable { mLingering = false; } - /** - * Returns whether this NAI has any chance of ever beating this other agent. - * - * The chief use case of this is the decision to tear down this network. ConnectivityService - * tears down networks that don't satisfy any request, unless they have a chance to beat any - * existing satisfier. - * - * @param other the agent to beat - * @return whether this should be given more time to try and beat the other agent - * TODO : remove this and migrate to a ranker-based approach - */ - public boolean canPossiblyBeat(@NonNull final NetworkAgentInfo other) { - // Any explicitly selected network should be held on. - if (networkAgentConfig.explicitlySelected) return true; - // An outscored exiting network should be torn down. - if (mNetworkScore.isExiting()) return false; - // If this network is validated it can be torn down as it can't hope to be better than - // it already is. - if (networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)) return false; - // If neither network is validated, keep both until at least one does. - if (!other.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)) return true; - // If this network is not metered but the other is, it should be preferable if it validates. - if (networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED) - && !other.networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED)) { - return true; - } - - // If the control comes here : - // • This network is neither exiting or explicitly selected - // • This network is not validated, but the other is - // • This network is metered, or both networks are unmetered - // Keep it if it's expected to be faster than the other., should it validate. - return mNetworkScore.probablyFasterThan(other.mNetworkScore); - } - public void dumpLingerTimers(PrintWriter pw) { for (LingerTimer timer : mLingerTimers) { pw.println(timer); } } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 5aabc60b22..08869751b9 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -2054,15 +2054,14 @@ public class ConnectivityServiceTest { assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - // Bring up validated wifi. + // Bring up wifi with a score of 70. // Cell is lingered because it would not satisfy any request, even if it validated. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - mWiFiNetworkAgent.connect(true); // Score: 60 + mWiFiNetworkAgent.adjustScore(50); + mWiFiNetworkAgent.connect(false); // Score: 70 callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - // TODO: Investigate sending validated before losing. callback.expectCallback(CallbackEntry.LOSING, mCellNetworkAgent); - callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); - defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); + defaultCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); From 6f677ac620e5f0d3bbad03207984a5286dbe7f65 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:39:19 +0000 Subject: [PATCH 4/8] Revert "[NS D04] Implement a simple speed comparison between scores." Revert submission 10338939 Reason for revert: The feature was punted out of R. Reverted Changes: I32c12702c:[NS D04] Implement a simple speed comparison betwe... I688593cc0:[NS D03] Migrate the bad wifi avoidance policy Change-Id: I28172721d3f3af82ac79ef2c8f61df39943d997b --- core/java/android/net/NetworkScore.java | 86 ------------------------- 1 file changed, 86 deletions(-) diff --git a/core/java/android/net/NetworkScore.java b/core/java/android/net/NetworkScore.java index d207e7960d..d2e59eb893 100644 --- a/core/java/android/net/NetworkScore.java +++ b/core/java/android/net/NetworkScore.java @@ -83,65 +83,6 @@ public final class NetworkScore implements Parcelable { uplinkBandwidthKBps = uplinkBandwidth; } - /** - * Evaluate whether a metrics codes for faster network is faster than another. - * - * This is a simple comparison of expected speeds. If either of the tested attributes - * are unknown, this returns zero. This implementation just assumes downlink bandwidth - * is more important than uplink bandwidth, which is more important than latency. This - * is not a very good way of evaluating network speed, but it's a start. - * TODO : do something more representative of how fast the network feels - * - * @param other the Metrics to evaluate against - * @return a negative integer, zero, or a positive integer as this metrics is worse than, - * equally good as (or unknown), or better than the passed Metrics. - * @see #compareToPreferringKnown(Metrics) - * @hide - */ - // Can't implement Comparable because this is @hide. - public int compareTo(@NonNull final Metrics other) { - if (downlinkBandwidthKBps != BANDWIDTH_UNKNOWN - && other.downlinkBandwidthKBps != BANDWIDTH_UNKNOWN) { - if (downlinkBandwidthKBps > other.downlinkBandwidthKBps) return 1; - if (downlinkBandwidthKBps < other.downlinkBandwidthKBps) return -1; - } - if (uplinkBandwidthKBps != BANDWIDTH_UNKNOWN - && other.uplinkBandwidthKBps != BANDWIDTH_UNKNOWN) { - if (uplinkBandwidthKBps > other.uplinkBandwidthKBps) return 1; - if (uplinkBandwidthKBps < other.uplinkBandwidthKBps) return -1; - } - if (latencyMs != LATENCY_UNKNOWN && other.latencyMs != LATENCY_UNKNOWN) { - // Latency : lower is better - if (latencyMs > other.latencyMs) return -1; - if (latencyMs < other.latencyMs) return 1; - } - return 0; - } - - /** - * Evaluate whether a metrics codes for faster network is faster than another. - * - * This is a simple comparison of expected speeds. If either of the tested attributes - * are unknown, this prefers the known attributes. This implementation just assumes - * downlink bandwidth is more important than uplink bandwidth, which is more important than - * latency. This is not a very good way of evaluating network speed, but it's a start. - * TODO : do something more representative of how fast the network feels - * - * @param other the Metrics to evaluate against - * @return a negative integer, zero, or a positive integer as this metrics is worse than, - * equally good as (or unknown), or better than the passed Metrics. - * @see #compareTo(Metrics) - * @hide - */ - public int compareToPreferringKnown(@NonNull final Metrics other) { - if (downlinkBandwidthKBps > other.downlinkBandwidthKBps) return 1; - if (downlinkBandwidthKBps < other.downlinkBandwidthKBps) return -1; - if (uplinkBandwidthKBps > other.uplinkBandwidthKBps) return 1; - if (uplinkBandwidthKBps < other.uplinkBandwidthKBps) return -1; - // Latency : lower is better - return -Integer.compare(latencyMs, other.latencyMs); - } - /** toString */ public String toString() { return "latency = " + latencyMs + " downlinkBandwidth = " + downlinkBandwidthKBps @@ -405,33 +346,6 @@ public final class NetworkScore implements Parcelable { return mLegacyScore; } - /** - * Use the metrics to evaluate whether a network is faster than another. - * - * This is purely based on the metrics, and explicitly ignores policy or exiting. It's - * provided to get a decision on two networks when policy can not decide, or to evaluate - * how a network is expected to compare to another if it should validate. - * - * @param other the score to evaluate against - * @return whether this network is probably faster than the other - * @hide - */ - public boolean probablyFasterThan(@NonNull final NetworkScore other) { - if (mLegacyScore > other.mLegacyScore) return true; - final int atEndToEnd = mEndToEndMetrics.compareTo(other.mEndToEndMetrics); - if (atEndToEnd > 0) return true; - if (atEndToEnd < 0) return false; - final int atLinkLayer = mLinkLayerMetrics.compareTo(other.mLinkLayerMetrics); - if (atLinkLayer > 0) return true; - if (atLinkLayer < 0) return false; - final int atEndToEndPreferringKnown = - mEndToEndMetrics.compareToPreferringKnown(other.mEndToEndMetrics); - if (atEndToEndPreferringKnown > 0) return true; - if (atEndToEndPreferringKnown < 0) return false; - // If this returns 0, neither is "probably faster" so just return false. - return mLinkLayerMetrics.compareToPreferringKnown(other.mLinkLayerMetrics) > 0; - } - /** Builder for NetworkScore. */ public static class Builder { private int mPolicy = 0; From 7b6b7ecc19ac6032850e49bfb4171ff63a7bf2f7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:39:19 +0000 Subject: [PATCH 5/8] Revert "[NS D03] Migrate the bad wifi avoidance policy" Revert submission 10338939 Reason for revert: The feature was punted out of R. Reverted Changes: I32c12702c:[NS D04] Implement a simple speed comparison betwe... I688593cc0:[NS D03] Migrate the bad wifi avoidance policy Change-Id: I640635a1ed94bed3b53466abe2a988caf0eca2b0 --- .../server/connectivity/NetworkRanker.java | 27 ++----------------- .../server/connectivity/NetworkRankerTest.kt | 2 -- 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index c536ab25e9..1ae7dc5c36 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -16,13 +16,8 @@ package com.android.server.connectivity; -import static android.net.NetworkScore.POLICY_IGNORE_ON_WIFI; - -import static com.android.internal.util.FunctionalUtils.findFirst; - import android.annotation.NonNull; import android.annotation.Nullable; -import android.net.NetworkCapabilities; import android.net.NetworkRequest; import java.util.ArrayList; @@ -42,33 +37,15 @@ public class NetworkRanker { @NonNull final Collection nais) { final ArrayList candidates = new ArrayList<>(nais); candidates.removeIf(nai -> !nai.satisfies(request)); - // Enforce policy. - filterBadWifiAvoidancePolicy(candidates); NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; for (final NetworkAgentInfo nai : candidates) { - final int score = nai.getCurrentScore(); - if (score > bestScore) { + if (nai.getCurrentScore() > bestScore) { bestNetwork = nai; - bestScore = score; + bestScore = nai.getCurrentScore(); } } return bestNetwork; } - - // If some network with wifi transport is present, drop all networks with POLICY_IGNORE_ON_WIFI. - private void filterBadWifiAvoidancePolicy( - @NonNull final ArrayList candidates) { - final NetworkAgentInfo wifi = findFirst(candidates, - nai -> nai.networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) - && nai.everValidated - // Horrible hack : there is old UI that will let a user say they want to - // override the policy only for this network only at this time and it - // feeds into the following member. This old UI should probably be removed - // but for now keep backward compatibility. - && !nai.avoidUnvalidated); - if (null == wifi) return; // No wifi : this policy doesn't apply - candidates.removeIf(nai -> nai.getNetworkScore().hasPolicy(POLICY_IGNORE_ON_WIFI)); - } } diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt index a6b371a23b..86c91165f6 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -16,7 +16,6 @@ package com.android.server.connectivity -import android.net.NetworkCapabilities import android.net.NetworkRequest import androidx.test.filters.SmallTest import androidx.test.runner.AndroidJUnit4 @@ -36,7 +35,6 @@ class NetworkRankerTest { private fun makeNai(satisfy: Boolean, score: Int) = mock(NetworkAgentInfo::class.java).also { doReturn(satisfy).`when`(it).satisfies(any()) doReturn(score).`when`(it).currentScore - it.networkCapabilities = NetworkCapabilities() } @Test From 99ae3ad2e5195b8ce3c608e58a28e4cbd7ea6b64 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 21 Feb 2020 17:00:49 +0900 Subject: [PATCH 6/8] Remove useless thread checks Because we liked to be really sure. Test: ConnectivityServiceTests Change-Id: I8d66257777d4c5b6ca097a5f2575d0872fae05dd --- .../core/java/com/android/server/ConnectivityService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1eb77a4963..34b7be4a8e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3295,7 +3295,6 @@ public class ConnectivityService extends IConnectivityManager.Stub for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest request = nai.requestAt(i); final NetworkRequestInfo nri = mNetworkRequests.get(request); - ensureRunningOnConnectivityServiceThread(); final NetworkAgentInfo currentNetwork = nri.mSatisfier; if (currentNetwork != null && currentNetwork.network.netId == nai.network.netId) { nri.mSatisfier = null; @@ -3447,7 +3446,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // If this Network is already the highest scoring Network for a request, or if // there is hope for it to become one if it validated, then it is needed. - ensureRunningOnConnectivityServiceThread(); if (nri.request.isRequest() && nai.satisfies(nri.request) && (nai.isSatisfyingRequest(nri.request.requestId) || // Note that this catches two important cases: @@ -3486,7 +3484,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (mNetworkRequests.get(nri.request) == null) { return; } - ensureRunningOnConnectivityServiceThread(); if (nri.mSatisfier != null) { return; } @@ -3524,7 +3521,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetworkRequestInfoLogs.log("RELEASE " + nri); if (nri.request.isRequest()) { boolean wasKept = false; - ensureRunningOnConnectivityServiceThread(); final NetworkAgentInfo nai = nri.mSatisfier; if (nai != null) { boolean wasBackgroundNetwork = nai.isBackgroundNetwork(); From 1d9a206bcac2dfb3f531846aeccdfc35a5779daa Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 07:39:31 +0000 Subject: [PATCH 7/8] Revert "[NS D02] Mix in the ignore on wifi policy." This reverts commit 7a56387e2c4d7e8c63733f2a18fb138e56668697. Reason for revert: The feature was punted out of R. Change-Id: Id9ecdac5292eeddf7c12f2330421248b0f8355a9 --- core/java/android/net/NetworkScore.java | 21 +++------- .../android/server/ConnectivityService.java | 41 +------------------ 2 files changed, 8 insertions(+), 54 deletions(-) diff --git a/core/java/android/net/NetworkScore.java b/core/java/android/net/NetworkScore.java index d2e59eb893..ae17378cfc 100644 --- a/core/java/android/net/NetworkScore.java +++ b/core/java/android/net/NetworkScore.java @@ -21,6 +21,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.SystemApi; import android.annotation.TestApi; +import android.os.Bundle; import android.os.Parcel; import android.os.Parcelable; @@ -86,7 +87,7 @@ public final class NetworkScore implements Parcelable { /** toString */ public String toString() { return "latency = " + latencyMs + " downlinkBandwidth = " + downlinkBandwidthKBps - + " uplinkBandwidth = " + uplinkBandwidthKBps; + + "uplinkBandwidth = " + uplinkBandwidthKBps; } @NonNull @@ -353,27 +354,17 @@ public final class NetworkScore implements Parcelable { private Metrics mLinkLayerMetrics = new Metrics(Metrics.LATENCY_UNKNOWN, Metrics.BANDWIDTH_UNKNOWN, Metrics.BANDWIDTH_UNKNOWN); @NonNull - private Metrics mEndToEndMetrics = new Metrics(Metrics.LATENCY_UNKNOWN, + private Metrics mEndToMetrics = new Metrics(Metrics.LATENCY_UNKNOWN, Metrics.BANDWIDTH_UNKNOWN, Metrics.BANDWIDTH_UNKNOWN); private int mSignalStrength = UNKNOWN_SIGNAL_STRENGTH; private int mRange = RANGE_UNKNOWN; private boolean mExiting = false; private int mLegacyScore = 0; + @NonNull private Bundle mExtensions = new Bundle(); /** Create a new builder. */ public Builder() { } - /** @hide */ - public Builder(@NonNull final NetworkScore source) { - mPolicy = source.mPolicy; - mLinkLayerMetrics = source.mLinkLayerMetrics; - mEndToEndMetrics = source.mEndToEndMetrics; - mSignalStrength = source.mSignalStrength; - mRange = source.mRange; - mExiting = source.mExiting; - mLegacyScore = source.mLegacyScore; - } - /** Add a policy flag. */ @NonNull public Builder addPolicy(@Policy final int policy) { mPolicy |= policy; @@ -394,7 +385,7 @@ public final class NetworkScore implements Parcelable { /** Set the end-to-end metrics. */ @NonNull public Builder setEndToEndMetrics(@NonNull final Metrics endToEndMetrics) { - mEndToEndMetrics = endToEndMetrics; + mEndToMetrics = endToEndMetrics; return this; } @@ -426,7 +417,7 @@ public final class NetworkScore implements Parcelable { /** Build the NetworkScore object represented by this builder. */ @NonNull public NetworkScore build() { - return new NetworkScore(mPolicy, mLinkLayerMetrics, mEndToEndMetrics, + return new NetworkScore(mPolicy, mLinkLayerMetrics, mEndToMetrics, mSignalStrength, mRange, mExiting, mLegacyScore); } } diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 7dab90d5ce..60a30d3317 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3817,9 +3817,8 @@ public class ConnectivityService extends IConnectivityManager.Stub return avoidBadWifi(); } + private void rematchForAvoidBadWifiUpdate() { - ensureRunningOnConnectivityServiceThread(); - mixInAllNetworkScores(); rematchAllNetworksAndRequests(); for (NetworkAgentInfo nai: mNetworkAgentInfos.values()) { if (nai.networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) { @@ -7036,45 +7035,9 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - /** - * Re-mixin all network scores. - * This is called when some global setting like avoidBadWifi has changed. - * TODO : remove this when all usages have been removed. - */ - private void mixInAllNetworkScores() { - ensureRunningOnConnectivityServiceThread(); - for (final NetworkAgentInfo nai : mNetworkAgentInfos.values()) { - nai.setNetworkScore(mixInNetworkScore(nai, nai.getNetworkScore())); - } - } - - /** - * Mix in the Connectivity-managed parts of the NetworkScore. - * @param nai The NAI this score applies to. - * @param sourceScore the score sent by the network agent, or the previous score of this NAI. - * @return A new score with the Connectivity-managed parts mixed in. - */ - @NonNull - private NetworkScore mixInNetworkScore(@NonNull final NetworkAgentInfo nai, - @NonNull final NetworkScore sourceScore) { - final NetworkScore.Builder score = new NetworkScore.Builder(sourceScore); - - // TODO : this should be done in Telephony. It should be handled per-network because - // it's a carrier-dependent config. - if (nai.networkCapabilities.hasTransport(TRANSPORT_CELLULAR)) { - if (mMultinetworkPolicyTracker.getAvoidBadWifi()) { - score.clearPolicy(NetworkScore.POLICY_IGNORE_ON_WIFI); - } else { - score.addPolicy(NetworkScore.POLICY_IGNORE_ON_WIFI); - } - } - - return score.build(); - } - private void updateNetworkScore(NetworkAgentInfo nai, NetworkScore ns) { if (VDBG || DDBG) log("updateNetworkScore for " + nai.toShortString() + " to " + ns); - nai.setNetworkScore(mixInNetworkScore(nai, ns)); + nai.setNetworkScore(ns); rematchAllNetworksAndRequests(); sendUpdatedScoreToFactories(nai); } From d15ca10d6d3bd61f9770565a649ae19dca8f3053 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 20 Feb 2020 08:41:38 +0000 Subject: [PATCH 8/8] Revert "[NS D01] Remove candidates that don't satisfy the request." This reverts commit a19115d4828d45336fae5b9e407fe55f16686696. Reason for revert: The feature was punted out of R. Change-Id: Ia91b3b3c55f735dae64ffa3194614a6f2631a087 --- .../com/android/server/connectivity/NetworkRanker.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index 1ae7dc5c36..d0aabf95d5 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -20,7 +20,6 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.net.NetworkRequest; -import java.util.ArrayList; import java.util.Collection; /** @@ -32,15 +31,15 @@ public class NetworkRanker { /** * Find the best network satisfying this request among the list of passed networks. */ + // Almost equivalent to Collections.max(nais), but allows returning null if no network + // satisfies the request. @Nullable public NetworkAgentInfo getBestNetwork(@NonNull final NetworkRequest request, @NonNull final Collection nais) { - final ArrayList candidates = new ArrayList<>(nais); - candidates.removeIf(nai -> !nai.satisfies(request)); - NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; - for (final NetworkAgentInfo nai : candidates) { + for (final NetworkAgentInfo nai : nais) { + if (!nai.satisfies(request)) continue; if (nai.getCurrentScore() > bestScore) { bestNetwork = nai; bestScore = nai.getCurrentScore();