From e3be94d65cc345b3b6b01191bf54dd5316a423d2 Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 6 Oct 2020 16:04:31 +0800 Subject: [PATCH 1/3] Remove unused getTetherStats Before Android R, getTetherStats were used to collect tethering iface stats of all clients who extends ITetheringStatsProvider, which typically contains NetdTetheringStatsProvider and OffloadController. However, this always returns 0 since Android R, because: 1. OffloadController implemented NetworkStatsProvider and no longer reports stats through getTetherStats. 2. NetdTetheringStatsProvider always return 0 by design since non-offloaded iface tethering stats are already included in native iface stats. This change is a no-op refactoring to remove unused getTetherStats and update test which is not pratical. Fix for offloaded tethering stats will be in follow-up CLs. Test: atest FrameworksNetTests TetheringTests Bug: 162292214 Change-Id: Icd0717c5c2807ae3bd98626b897e4b148f142815 --- .../server/net/NetworkStatsService.java | 63 +++---------------- 1 file changed, 10 insertions(+), 53 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 12c24d4186..96e3c5163f 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -1083,13 +1083,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub { if (nativeIfaceStats == -1) { return nativeIfaceStats; } else { - // When tethering offload is in use, nativeIfaceStats does not contain usage from - // offload, add it back here. - // When tethering offload is not in use, nativeIfaceStats contains tethering usage. - // this does not cause double-counting of tethering traffic, because - // NetdTetheringStatsProvider returns zero NetworkStats - // when called with STATS_PER_IFACE. - return nativeIfaceStats + getTetherStats(iface, type); + // TODO: When tethering offload is in use, nativeIfaceStats does not contain usage from + // offload, add it back here. + return nativeIfaceStats; } } @@ -1100,42 +1096,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { return nativeTotalStats; } else { // Refer to comment in getIfaceStats - return nativeTotalStats + getTetherStats(IFACE_ALL, type); - } - } - - private long getTetherStats(String iface, int type) { - final NetworkStats tetherSnapshot; - final long token = Binder.clearCallingIdentity(); - try { - tetherSnapshot = getNetworkStatsTethering(STATS_PER_IFACE); - } catch (RemoteException e) { - Slog.w(TAG, "Error get TetherStats: " + e); - return 0; - } finally { - Binder.restoreCallingIdentity(token); - } - HashSet limitIfaces; - if (iface == IFACE_ALL) { - limitIfaces = null; - } else { - limitIfaces = new HashSet(); - limitIfaces.add(iface); - } - NetworkStats.Entry entry = tetherSnapshot.getTotal(null, limitIfaces); - if (LOGD) Slog.d(TAG, "TetherStats: iface=" + iface + " type=" + type + - " entry=" + entry); - switch (type) { - case 0: // TYPE_RX_BYTES - return entry.rxBytes; - case 1: // TYPE_RX_PACKETS - return entry.rxPackets; - case 2: // TYPE_TX_BYTES - return entry.txBytes; - case 3: // TYPE_TX_PACKETS - return entry.txPackets; - default: - return 0; + return nativeTotalStats; } } @@ -1429,14 +1390,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final NetworkStats devSnapshot = readNetworkStatsSummaryDev(); Trace.traceEnd(TRACE_TAG_NETWORK); - // Tethering snapshot for dev and xt stats. Counts per-interface data from tethering stats - // providers that isn't already counted by dev and XT stats. - Trace.traceBegin(TRACE_TAG_NETWORK, "snapshotTether"); - final NetworkStats tetherSnapshot = getNetworkStatsTethering(STATS_PER_IFACE); - Trace.traceEnd(TRACE_TAG_NETWORK); - xtSnapshot.combineAllValues(tetherSnapshot); - devSnapshot.combineAllValues(tetherSnapshot); - // Snapshot for dev/xt stats from all custom stats providers. Counts per-interface data // from stats providers that isn't already counted by dev and XT stats. Trace.traceBegin(TRACE_TAG_NETWORK, "snapshotStatsProvider"); @@ -1931,9 +1884,13 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } /** - * Return snapshot of current tethering statistics. Will return empty - * {@link NetworkStats} if any problems are encountered. + * Return snapshot of current non-offloaded tethering statistics. Will return empty + * {@link NetworkStats} if any problems are encountered, or queried by {@code STATS_PER_IFACE} + * since it is already included by {@link #nativeGetIfaceStat}. + * See {@code OffloadTetheringStatsProvider} for offloaded tethering stats. */ + // TODO: Remove this by implementing {@link NetworkStatsProvider} for non-offloaded + // tethering stats. private NetworkStats getNetworkStatsTethering(int how) throws RemoteException { try { return mNetworkManager.getNetworkStatsTethering(how); From ea2363294f9d653430acb945c7ee1870de351028 Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 6 Oct 2020 11:59:56 +0800 Subject: [PATCH 2/3] Return offloaded traffic when querying from TrafficStats API TrafficStats API are being used for querying realtime network statistics for years. However, on certain devices, some network traffic are produced by hardware components and not be able to seen by kernel counters. Thus, include statistics for those missing network traffic is necessary. Note that the included statistics might be stale since polling newest stats from hardware might impact system health and not suitable for TrafficStats API use cases. Test: atest FrameworksNetTests TetheringTests Bug: 16229221 Change-Id: I6741c41cb5145ca8748f9b083b9c15e7e2735681 --- core/java/android/net/TrafficStats.java | 20 +++-- .../server/net/NetworkStatsService.java | 87 +++++++++++++------ 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/core/java/android/net/TrafficStats.java b/core/java/android/net/TrafficStats.java index e7bba69dbb..7a79ceb14a 100644 --- a/core/java/android/net/TrafficStats.java +++ b/core/java/android/net/TrafficStats.java @@ -976,11 +976,17 @@ public class TrafficStats { } } - // NOTE: keep these in sync with android_net_TrafficStats.cpp - private static final int TYPE_RX_BYTES = 0; - private static final int TYPE_RX_PACKETS = 1; - private static final int TYPE_TX_BYTES = 2; - private static final int TYPE_TX_PACKETS = 3; - private static final int TYPE_TCP_RX_PACKETS = 4; - private static final int TYPE_TCP_TX_PACKETS = 5; + // NOTE: keep these in sync with {@code com_android_server_net_NetworkStatsService.cpp}. + /** {@hide} */ + public static final int TYPE_RX_BYTES = 0; + /** {@hide} */ + public static final int TYPE_RX_PACKETS = 1; + /** {@hide} */ + public static final int TYPE_TX_BYTES = 2; + /** {@hide} */ + public static final int TYPE_TX_PACKETS = 3; + /** {@hide} */ + public static final int TYPE_TCP_RX_PACKETS = 4; + /** {@hide} */ + public static final int TYPE_TCP_TX_PACKETS = 5; } diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 96e3c5163f..bfd0211f18 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -1083,9 +1083,11 @@ public class NetworkStatsService extends INetworkStatsService.Stub { if (nativeIfaceStats == -1) { return nativeIfaceStats; } else { - // TODO: When tethering offload is in use, nativeIfaceStats does not contain usage from - // offload, add it back here. - return nativeIfaceStats; + // When tethering offload is in use, nativeIfaceStats does not contain usage from + // offload, add it back here. Note that the included statistics might be stale + // since polling newest stats from hardware might impact system health and not + // suitable for TrafficStats API use cases. + return nativeIfaceStats + getProviderIfaceStats(iface, type); } } @@ -1096,7 +1098,31 @@ public class NetworkStatsService extends INetworkStatsService.Stub { return nativeTotalStats; } else { // Refer to comment in getIfaceStats - return nativeTotalStats; + return nativeTotalStats + getProviderIfaceStats(IFACE_ALL, type); + } + } + + private long getProviderIfaceStats(@Nullable String iface, int type) { + final NetworkStats providerSnapshot = getNetworkStatsFromProviders(STATS_PER_IFACE); + final HashSet limitIfaces; + if (iface == IFACE_ALL) { + limitIfaces = null; + } else { + limitIfaces = new HashSet<>(); + limitIfaces.add(iface); + } + final NetworkStats.Entry entry = providerSnapshot.getTotal(null, limitIfaces); + switch (type) { + case TrafficStats.TYPE_RX_BYTES: + return entry.rxBytes; + case TrafficStats.TYPE_RX_PACKETS: + return entry.rxPackets; + case TrafficStats.TYPE_TX_BYTES: + return entry.txBytes; + case TrafficStats.TYPE_TX_PACKETS: + return entry.txPackets; + default: + return 0; } } @@ -1464,29 +1490,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final boolean persistUid = (flags & FLAG_PERSIST_UID) != 0; final boolean persistForce = (flags & FLAG_PERSIST_FORCE) != 0; - // Request asynchronous stats update from all providers for next poll. And wait a bit of - // time to allow providers report-in given that normally binder call should be fast. Note - // that size of list might be changed because addition/removing at the same time. For - // addition, the stats of the missed provider can only be collected in next poll; - // for removal, wait might take up to MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS - // once that happened. - // TODO: request with a valid token. - Trace.traceBegin(TRACE_TAG_NETWORK, "provider.requestStatsUpdate"); - final int registeredCallbackCount = mStatsProviderCbList.size(); - mStatsProviderSem.drainPermits(); - invokeForAllStatsProviderCallbacks( - (cb) -> cb.mProvider.onRequestStatsUpdate(0 /* unused */)); - try { - mStatsProviderSem.tryAcquire(registeredCallbackCount, - MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - // Strictly speaking it's possible a provider happened to deliver between the timeout - // and the log, and that doesn't matter too much as this is just a debug log. - Log.d(TAG, "requestStatsUpdate - providers responded " - + mStatsProviderSem.availablePermits() - + "/" + registeredCallbackCount + " : " + e); - } - Trace.traceEnd(TRACE_TAG_NETWORK); + performPollFromProvidersLocked(); // TODO: consider marking "untrusted" times in historical stats final long currentTime = mClock.millis(); @@ -1531,6 +1535,33 @@ public class NetworkStatsService extends INetworkStatsService.Stub { Trace.traceEnd(TRACE_TAG_NETWORK); } + @GuardedBy("mStatsLock") + private void performPollFromProvidersLocked() { + // Request asynchronous stats update from all providers for next poll. And wait a bit of + // time to allow providers report-in given that normally binder call should be fast. Note + // that size of list might be changed because addition/removing at the same time. For + // addition, the stats of the missed provider can only be collected in next poll; + // for removal, wait might take up to MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS + // once that happened. + // TODO: request with a valid token. + Trace.traceBegin(TRACE_TAG_NETWORK, "provider.requestStatsUpdate"); + final int registeredCallbackCount = mStatsProviderCbList.size(); + mStatsProviderSem.drainPermits(); + invokeForAllStatsProviderCallbacks( + (cb) -> cb.mProvider.onRequestStatsUpdate(0 /* unused */)); + try { + mStatsProviderSem.tryAcquire(registeredCallbackCount, + MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + // Strictly speaking it's possible a provider happened to deliver between the timeout + // and the log, and that doesn't matter too much as this is just a debug log. + Log.d(TAG, "requestStatsUpdate - providers responded " + + mStatsProviderSem.availablePermits() + + "/" + registeredCallbackCount + " : " + e); + } + Trace.traceEnd(TRACE_TAG_NETWORK); + } + /** * Sample recent statistics summary into {@link EventLog}. */ @@ -2183,6 +2214,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } + // TODO: Remove unused definitions after removing JNI that fills these variables. + // See {@code com_android_server_net_NetworkStatsService.cpp}. private static int TYPE_RX_BYTES; private static int TYPE_RX_PACKETS; private static int TYPE_TX_BYTES; From a545c35fa4fa81dd9920b33f6036aa34e8023f68 Mon Sep 17 00:00:00 2001 From: junyulai Date: Fri, 6 Nov 2020 19:19:31 +0800 Subject: [PATCH 3/3] Remove unused variables From aosp/537809, variables that used to fetch realtime stats are defined in NetworkStatsService. These varialbles are filled by JNI in boot up stage in order to keep definitions sync with native layer. However, there is still a copy in TrafficStats.java, and this copy cannot be filled in boot-up stage since it is in app process. Besides, making a binder call to fetch these constants from service is considered an overkill. Thus, since there is no caller to these variables and callers should use definitions in TrafficStats, remove these variables. Test: atest FrameworksNetTests Bug: 16229221 Change-Id: I6a48d4dbb1b824cfc6c4a47395b2a76aa28cf5c9 --- .../android/server/net/NetworkStatsService.java | 9 --------- ...com_android_server_net_NetworkStatsService.cpp | 15 --------------- 2 files changed, 24 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index bfd0211f18..81a6641de8 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -2214,15 +2214,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - // TODO: Remove unused definitions after removing JNI that fills these variables. - // See {@code com_android_server_net_NetworkStatsService.cpp}. - private static int TYPE_RX_BYTES; - private static int TYPE_RX_PACKETS; - private static int TYPE_TX_BYTES; - private static int TYPE_TX_PACKETS; - private static int TYPE_TCP_RX_PACKETS; - private static int TYPE_TCP_TX_PACKETS; - private static native long nativeGetTotalStat(int type, boolean useBpfStats); private static native long nativeGetIfaceStat(String iface, int type, boolean useBpfStats); private static native long nativeGetUidStat(int uid, int type, boolean useBpfStats); diff --git a/services/core/jni/com_android_server_net_NetworkStatsService.cpp b/services/core/jni/com_android_server_net_NetworkStatsService.cpp index 0275f3ea32..10b248a70e 100644 --- a/services/core/jni/com_android_server_net_NetworkStatsService.cpp +++ b/services/core/jni/com_android_server_net_NetworkStatsService.cpp @@ -215,21 +215,6 @@ static const JNINativeMethod gMethods[] = { }; int register_android_server_net_NetworkStatsService(JNIEnv* env) { - jclass netStatsService = env->FindClass("com/android/server/net/NetworkStatsService"); - jfieldID rxBytesId = env->GetStaticFieldID(netStatsService, "TYPE_RX_BYTES", "I"); - jfieldID rxPacketsId = env->GetStaticFieldID(netStatsService, "TYPE_RX_PACKETS", "I"); - jfieldID txBytesId = env->GetStaticFieldID(netStatsService, "TYPE_TX_BYTES", "I"); - jfieldID txPacketsId = env->GetStaticFieldID(netStatsService, "TYPE_TX_PACKETS", "I"); - jfieldID tcpRxPacketsId = env->GetStaticFieldID(netStatsService, "TYPE_TCP_RX_PACKETS", "I"); - jfieldID tcpTxPacketsId = env->GetStaticFieldID(netStatsService, "TYPE_TCP_TX_PACKETS", "I"); - - env->SetStaticIntField(netStatsService, rxBytesId, RX_BYTES); - env->SetStaticIntField(netStatsService, rxPacketsId, RX_PACKETS); - env->SetStaticIntField(netStatsService, txBytesId, TX_BYTES); - env->SetStaticIntField(netStatsService, txPacketsId, TX_PACKETS); - env->SetStaticIntField(netStatsService, tcpRxPacketsId, TCP_RX_PACKETS); - env->SetStaticIntField(netStatsService, tcpTxPacketsId, TCP_TX_PACKETS); - return jniRegisterNativeMethods(env, "com/android/server/net/NetworkStatsService", gMethods, NELEM(gMethods)); }