From 2ed7d4182af623b87d0f7144e8781e60463302a9 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Fri, 7 Oct 2022 16:52:21 +0800 Subject: [PATCH] Notify NetworkStats only when interfaces changed in updateLinkProperties In ConnectivityService, updateLinkProperties calls NetworkStats even though the ip address or interface name has not changed and only the tcp buffer size has changed. This is noisy and could be problematic when RAT change occurs frequently, since when RAT changes tcp buffer size configuration also changes. This CL also fixes a wrong nullability annotation where the oldLp of updateLinkProperties could be null when updateNetworkInfo is called. Test: atest ConnectivityServiceTest#testStatsIfacesChanged Fix: 232048480 Change-Id: Ic226eb4a8aa1f38cba293510813f1cb55f0ea658 --- .../src/com/android/server/ConnectivityService.java | 13 ++++++++++--- .../com/android/server/connectivity/Nat464Xlat.java | 5 ++++- .../com/android/server/ConnectivityServiceTest.java | 9 +++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 93265e51ca..dfa132e690 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -7435,7 +7435,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void updateLinkProperties(NetworkAgentInfo networkAgent, @NonNull LinkProperties newLp, - @NonNull LinkProperties oldLp) { + @Nullable LinkProperties oldLp) { int netId = networkAgent.network.getNetId(); // The NetworkAgent does not know whether clatd is running on its network or not, or whether @@ -7487,7 +7487,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } // Start or stop DNS64 detection and 464xlat according to network state. networkAgent.clatd.update(); - notifyIfacesChangedForNetworkStats(); + // Notify NSS when relevant events happened. Currently, NSS only cares about + // interface changed to update clat interfaces accounting. + final boolean interfacesChanged = oldLp == null + || !Objects.equals(newLp.getAllInterfaceNames(), oldLp.getAllInterfaceNames()); + if (interfacesChanged) { + notifyIfacesChangedForNetworkStats(); + } networkAgent.networkMonitor().notifyLinkPropertiesChanged( new LinkProperties(newLp, true /* parcelSensitiveFields */)); notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_IP_CHANGED); @@ -8246,7 +8252,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) { + public void handleUpdateLinkProperties(@NonNull NetworkAgentInfo nai, + @NonNull LinkProperties newLp) { ensureRunningOnConnectivityServiceThread(); if (!mNetworkAgentInfos.contains(nai)) { diff --git a/service/src/com/android/server/connectivity/Nat464Xlat.java b/service/src/com/android/server/connectivity/Nat464Xlat.java index a57e992499..8ba93f2f26 100644 --- a/service/src/com/android/server/connectivity/Nat464Xlat.java +++ b/service/src/com/android/server/connectivity/Nat464Xlat.java @@ -22,6 +22,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_TEST; import static com.android.net.module.util.CollectionUtils.contains; import android.annotation.NonNull; +import android.annotation.Nullable; import android.net.ConnectivityManager; import android.net.IDnsResolver; import android.net.INetd; @@ -420,7 +421,7 @@ public class Nat464Xlat { * This is necessary because the LinkProperties in mNetwork come from the transport layer, which * has no idea that 464xlat is running on top of it. */ - public void fixupLinkProperties(@NonNull LinkProperties oldLp, @NonNull LinkProperties lp) { + public void fixupLinkProperties(@Nullable LinkProperties oldLp, @NonNull LinkProperties lp) { // This must be done even if clatd is not running, because otherwise shouldStartClat would // never return true. lp.setNat64Prefix(selectNat64Prefix()); @@ -433,6 +434,8 @@ public class Nat464Xlat { } Log.d(TAG, "clatd running, updating NAI for " + mIface); + // oldLp can't be null here since shouldStartClat checks null LinkProperties to start clat. + // Thus, the status won't pass isRunning check if the oldLp is null. for (LinkProperties stacked: oldLp.getStackedLinks()) { if (Objects.equals(mIface, stacked.getInterfaceName())) { lp.addStackedLink(stacked); diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index d2a71354ec..a785a35314 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -7109,6 +7109,15 @@ public class ConnectivityServiceTest { expectNotifyNetworkStatus(onlyCell, MOBILE_IFNAME); reset(mStatsManager); + // Verify change fields other than interfaces does not trigger a notification to NSS. + cellLp.addLinkAddress(new LinkAddress("192.0.2.4/24")); + cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("192.0.2.4"), + MOBILE_IFNAME)); + cellLp.setDnsServers(List.of(InetAddress.getAllByName("8.8.8.8"))); + mCellNetworkAgent.sendLinkProperties(cellLp); + verifyNoMoreInteractions(mStatsManager); + reset(mStatsManager); + // Default network switch should update ifaces. mWiFiNetworkAgent.connect(false); mWiFiNetworkAgent.sendLinkProperties(wifiLp);