From 304de5156188b64947a301ca20f9e2be6b63b4c4 Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 12 Feb 2020 14:55:54 +0800 Subject: [PATCH] [SP17] Wait for stats providers to report stats update From current design, the traffic accounted by stats providers will be updated asynchronously when force polling. When upper layer make two subsequently queries. They will get stale stats upon the first query, and may get newest/stale stats base on the result of race. Thus, wait for a bit of time to allow asynchronous stats update complete to reduce the chance of race. In pratice, it would be finished in ~2ms when testing. Test: systrace.py network Test: atest FrameworksNetTests Bug: 147460444 Change-Id: I22a00fc4049cddf77fd578e25769ae1979f2cc6d --- .../server/net/NetworkStatsService.java | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 415ccb8fe8..1c52110449 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -155,6 +155,8 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; /** * Collect and persist detailed network statistics, and provide this data to @@ -280,8 +282,11 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private final DropBoxNonMonotonicObserver mNonMonotonicObserver = new DropBoxNonMonotonicObserver(); + private static final int MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS = 100; private final RemoteCallbackList mStatsProviderCbList = new RemoteCallbackList<>(); + /** Semaphore used to wait for stats provider to respond to request stats update. */ + private final Semaphore mStatsProviderSem = new Semaphore(0, true); @GuardedBy("mStatsLock") private NetworkStatsRecorder mDevRecorder; @@ -1337,6 +1342,25 @@ 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. + // TODO: request with a valid token. + Trace.traceBegin(TRACE_TAG_NETWORK, "provider.requestStatsUpdate"); + final int registeredCallbackCount = mStatsProviderCbList.getRegisteredCallbackCount(); + mStatsProviderSem.drainPermits(); + invokeForAllStatsProviderCallbacks((cb) -> cb.mProvider.requestStatsUpdate(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); + // TODO: consider marking "untrusted" times in historical stats final long currentTime = mClock.millis(); @@ -1374,10 +1398,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { performSampleLocked(); } - // request asynchronous stats update from all providers for next poll. - // TODO: request with a valid token. - invokeForAllStatsProviderCallbacks((cb) -> cb.mProvider.requestStatsUpdate(0 /* unused */)); - // finally, dispatch updated event to any listeners final Intent updatedIntent = new Intent(ACTION_NETWORK_STATS_UPDATED); updatedIntent.setFlags(Intent.FLAG_RECEIVER_REGISTERED_ONLY); @@ -1798,7 +1818,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub { Objects.requireNonNull(tag, "tag is null"); try { NetworkStatsProviderCallbackImpl callback = new NetworkStatsProviderCallbackImpl( - tag, provider, mAlertObserver, mStatsProviderCbList); + tag, provider, mStatsProviderSem, mAlertObserver, + mStatsProviderCbList); mStatsProviderCbList.register(callback); Log.d(TAG, "registerNetworkStatsProvider from " + callback.mTag + " uid/pid=" + getCallingUid() + "/" + getCallingPid()); @@ -1846,6 +1867,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @NonNull final String mTag; @NonNull private final Object mProviderStatsLock = new Object(); @NonNull final INetworkStatsProvider mProvider; + @NonNull private final Semaphore mSemaphore; @NonNull final INetworkManagementEventObserver mAlertObserver; @NonNull final RemoteCallbackList mStatsProviderCbList; @@ -1857,12 +1879,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub { NetworkStatsProviderCallbackImpl( @NonNull String tag, @NonNull INetworkStatsProvider provider, + @NonNull Semaphore semaphore, @NonNull INetworkManagementEventObserver alertObserver, @NonNull RemoteCallbackList cbList) throws RemoteException { mTag = tag; mProvider = provider; mProvider.asBinder().linkToDeath(this, 0); + mSemaphore = semaphore; mAlertObserver = alertObserver; mStatsProviderCbList = cbList; } @@ -1895,6 +1919,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { if (ifaceStats != null) mIfaceStats.combineAllValues(ifaceStats); if (uidStats != null) mUidStats.combineAllValues(uidStats); } + mSemaphore.release(); } @Override