From d953c53d3b04d772bb1b62ede1c2011641ca82b5 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Sat, 16 Aug 2014 18:17:38 -0700 Subject: Work on issue #16629489: Google (Play?) Services eating through battery There is a bug in how we deal with name overflows combined with resetting the battery stats data. If we do a reset while a wakelock is being actively held that has been put into the overflow bucket, then we can end up reducing the number of known wake locks in the list so when after that it is released we try to release it under its real name rather than the overflow name. This means we need to keep track of which wake locks have been placed in the overflow bucket while they are actively being used, so we can be sure to properly handle it as part of that bucket until it is eventually released. This makes things... somewhat more complicated. So now we have a class to take care of all these details, and also use it for other places where we have the same overflow semantics sync and job stats. Also fix potential deadlock -- BatteryStatsHelper needs to call on to ConnectivityManager to find out of there is telepohny, however we use that class when doing a dump while the battery stats lock is held. To fix this, we check the connectivity state up in the battery stats service before acquiring the lock and propagate that information through to the dump code. Change-Id: Ib452206af5c36f4b0f03cc94d2845d36613d1ba5 --- core/java/android/os/BatteryStats.java | 39 ++- .../android/internal/os/BatteryStatsHelper.java | 18 +- .../com/android/internal/os/BatteryStatsImpl.java | 386 ++++++++++++++------- .../com/android/server/am/BatteryStatsService.java | 4 + .../android/server/power/PowerManagerService.java | 9 +- 5 files changed, 311 insertions(+), 145 deletions(-) diff --git a/core/java/android/os/BatteryStats.java b/core/java/android/os/BatteryStats.java index 7d086e1aa7ef..be46bc798adb 100644 --- a/core/java/android/os/BatteryStats.java +++ b/core/java/android/os/BatteryStats.java @@ -1843,13 +1843,21 @@ public abstract class BatteryStats implements Parcelable { } pw.println(); } - + + /** + * Temporary for settings. + */ + public final void dumpCheckinLocked(Context context, PrintWriter pw, int which, int reqUid) { + dumpCheckinLocked(context, pw, which, reqUid, BatteryStatsHelper.checkWifiOnly(context)); + } + /** * Checkin server version of dump to produce more compact, computer-readable log. * * NOTE: all times are expressed in 'ms'. */ - public final void dumpCheckinLocked(Context context, PrintWriter pw, int which, int reqUid) { + public final void dumpCheckinLocked(Context context, PrintWriter pw, int which, int reqUid, + boolean wifiOnly) { final long rawUptime = SystemClock.uptimeMillis() * 1000; final long rawRealtime = SystemClock.elapsedRealtime() * 1000; final long batteryUptime = getBatteryUptime(rawUptime); @@ -2046,7 +2054,7 @@ public abstract class BatteryStats implements Parcelable { } } - BatteryStatsHelper helper = new BatteryStatsHelper(context, false); + BatteryStatsHelper helper = new BatteryStatsHelper(context, false, wifiOnly); helper.create(this); helper.refreshStats(which, UserHandle.USER_ALL); List sippers = helper.getUsageList(); @@ -2315,9 +2323,17 @@ public abstract class BatteryStats implements Parcelable { printer.print(BatteryStatsHelper.makemAh(power)); } + /** + * Temporary for settings. + */ + public final void dumpLocked(Context context, PrintWriter pw, String prefix, int which, + int reqUid) { + dumpLocked(context, pw, prefix, which, reqUid, BatteryStatsHelper.checkWifiOnly(context)); + } + @SuppressWarnings("unused") public final void dumpLocked(Context context, PrintWriter pw, String prefix, final int which, - int reqUid) { + int reqUid, boolean wifiOnly) { final long rawUptime = SystemClock.uptimeMillis() * 1000; final long rawRealtime = SystemClock.elapsedRealtime() * 1000; final long batteryUptime = getBatteryUptime(rawUptime); @@ -2746,7 +2762,7 @@ public abstract class BatteryStats implements Parcelable { pw.println(); } - BatteryStatsHelper helper = new BatteryStatsHelper(context, false); + BatteryStatsHelper helper = new BatteryStatsHelper(context, false, wifiOnly); helper.create(this); helper.refreshStats(which, UserHandle.USER_ALL); List sippers = helper.getUsageList(); @@ -3723,6 +3739,7 @@ public abstract class BatteryStats implements Parcelable { public static final int DUMP_HISTORY_ONLY = 1<<2; public static final int DUMP_INCLUDE_HISTORY = 1<<3; public static final int DUMP_VERBOSE = 1<<4; + public static final int DUMP_DEVICE_WIFI_ONLY = 1<<5; private void dumpHistoryLocked(PrintWriter pw, int flags, long histStart, boolean checkin) { final HistoryPrinter hprinter = new HistoryPrinter(); @@ -3918,12 +3935,14 @@ public abstract class BatteryStats implements Parcelable { pw.println("Statistics since last charge:"); pw.println(" System starts: " + getStartCount() + ", currently on battery: " + getIsOnBattery()); - dumpLocked(context, pw, "", STATS_SINCE_CHARGED, reqUid); + dumpLocked(context, pw, "", STATS_SINCE_CHARGED, reqUid, + (flags&DUMP_DEVICE_WIFI_ONLY) != 0); pw.println(); } if (!filtering || (flags&DUMP_UNPLUGGED_ONLY) != 0) { pw.println("Statistics since last unplugged:"); - dumpLocked(context, pw, "", STATS_SINCE_UNPLUGGED, reqUid); + dumpLocked(context, pw, "", STATS_SINCE_UNPLUGGED, reqUid, + (flags&DUMP_DEVICE_WIFI_ONLY) != 0); } } @@ -4013,10 +4032,12 @@ public abstract class BatteryStats implements Parcelable { dumpLine(pw, 0 /* uid */, "i" /* category */, CHARGE_TIME_REMAIN_DATA, (Object[])lineArgs); } - dumpCheckinLocked(context, pw, STATS_SINCE_CHARGED, -1); + dumpCheckinLocked(context, pw, STATS_SINCE_CHARGED, -1, + (flags&DUMP_DEVICE_WIFI_ONLY) != 0); } if (!filtering || (flags&DUMP_UNPLUGGED_ONLY) != 0) { - dumpCheckinLocked(context, pw, STATS_SINCE_UNPLUGGED, -1); + dumpCheckinLocked(context, pw, STATS_SINCE_UNPLUGGED, -1, + (flags&DUMP_DEVICE_WIFI_ONLY) != 0); } } } diff --git a/core/java/com/android/internal/os/BatteryStatsHelper.java b/core/java/com/android/internal/os/BatteryStatsHelper.java index 63be2d4d4f6a..ee406bd932a9 100644 --- a/core/java/com/android/internal/os/BatteryStatsHelper.java +++ b/core/java/com/android/internal/os/BatteryStatsHelper.java @@ -74,6 +74,7 @@ public final class BatteryStatsHelper { final private Context mContext; final private boolean mCollectBatteryBroadcast; + final private boolean mWifiOnly; private IBatteryStats mBatteryInfo; private BatteryStats mStats; @@ -123,6 +124,19 @@ public final class BatteryStatsHelper { public BatteryStatsHelper(Context context, boolean collectBatteryBroadcast) { mContext = context; mCollectBatteryBroadcast = collectBatteryBroadcast; + mWifiOnly = checkWifiOnly(context); + } + + public BatteryStatsHelper(Context context, boolean collectBatteryBroadcast, boolean wifiOnly) { + mContext = context; + mCollectBatteryBroadcast = collectBatteryBroadcast; + mWifiOnly = wifiOnly; + } + + public static boolean checkWifiOnly(Context context) { + ConnectivityManager cm = (ConnectivityManager)context.getSystemService( + Context.CONNECTIVITY_SERVICE); + return !cm.isNetworkSupported(ConnectivityManager.TYPE_MOBILE); } public void storeStatsHistoryInFile(String fname) { @@ -870,9 +884,7 @@ public final class BatteryStatsHelper { addBluetoothUsage(); addIdleUsage(); // Not including cellular idle power // Don't compute radio usage if it's a wifi-only device - ConnectivityManager cm = (ConnectivityManager)mContext.getSystemService( - Context.CONNECTIVITY_SERVICE); - if (cm.isNetworkSupported(ConnectivityManager.TYPE_MOBILE)) { + if (!mWifiOnly) { addRadioUsage(); } } diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index a745b209605f..6c9766e4bbc6 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -49,6 +49,7 @@ import android.text.TextUtils; import android.util.ArrayMap; import android.util.Log; import android.util.LogWriter; +import android.util.MutableInt; import android.util.PrintWriterPrinter; import android.util.Printer; import android.util.Slog; @@ -106,8 +107,6 @@ public final class BatteryStatsImpl extends BatteryStats { // in to one common name. private static final int MAX_WAKELOCKS_PER_UID = 50; - private static final String BATCHED_WAKELOCK_NAME = "*overflow*"; - private static int sNumSpeedSteps; private final JournaledFile mFile; @@ -1551,6 +1550,140 @@ public final class BatteryStatsImpl extends BatteryStats { } } + public abstract class OverflowArrayMap { + private static final String OVERFLOW_NAME = "*overflow*"; + + final ArrayMap mMap = new ArrayMap<>(); + T mCurOverflow; + ArrayMap mActiveOverflow; + + public OverflowArrayMap() { + } + + public ArrayMap getMap() { + return mMap; + } + + public void clear() { + mMap.clear(); + mCurOverflow = null; + mActiveOverflow = null; + } + + public void add(String name, T obj) { + mMap.put(name, obj); + if (OVERFLOW_NAME.equals(name)) { + mCurOverflow = obj; + } + } + + public void cleanup() { + if (mActiveOverflow != null) { + if (mActiveOverflow.size() == 0) { + mActiveOverflow = null; + } + } + if (mActiveOverflow == null) { + // There is no currently active overflow, so we should no longer have + // an overflow entry. + if (mMap.containsKey(OVERFLOW_NAME)) { + Slog.wtf(TAG, "Cleaning up with no active overflow, but have overflow entry " + + mMap.get(OVERFLOW_NAME)); + mMap.remove(OVERFLOW_NAME); + } + mCurOverflow = null; + } else { + // There is currently active overflow, so we should still have an overflow entry. + if (mCurOverflow == null || !mMap.containsKey(OVERFLOW_NAME)) { + Slog.wtf(TAG, "Cleaning up with active overflow, but no overflow entry: cur=" + + mCurOverflow + " map=" + mMap.get(OVERFLOW_NAME)); + } + } + } + + public T startObject(String name) { + T obj = mMap.get(name); + if (obj != null) { + return obj; + } + + // No object exists for the given name, but do we currently have it + // running as part of the overflow? + if (mActiveOverflow != null) { + MutableInt over = mActiveOverflow.get(name); + if (over != null) { + // We are already actively counting this name in the overflow object. + obj = mCurOverflow; + if (obj == null) { + // Shouldn't be here, but we'll try to recover. + Slog.wtf(TAG, "Have active overflow " + name + " but null overflow"); + obj = mCurOverflow = instantiateObject(); + mMap.put(OVERFLOW_NAME, obj); + } + over.value++; + return obj; + } + } + + // No object exists for given name nor in the overflow; we need to make + // a new one. + final int N = mMap.size(); + if (N >= MAX_WAKELOCKS_PER_UID) { + // Went over the limit on number of objects to track; this one goes + // in to the overflow. + obj = mCurOverflow; + if (obj == null) { + // Need to start overflow now... + obj = mCurOverflow = instantiateObject(); + mMap.put(OVERFLOW_NAME, obj); + } + if (mActiveOverflow == null) { + mActiveOverflow = new ArrayMap<>(); + } + mActiveOverflow.put(name, new MutableInt(1)); + return obj; + } + + // Normal case where we just need to make a new object. + obj = instantiateObject(); + mMap.put(name, obj); + return obj; + } + + public T stopObject(String name) { + T obj = mMap.get(name); + if (obj != null) { + return obj; + } + + // No object exists for the given name, but do we currently have it + // running as part of the overflow? + if (mActiveOverflow != null) { + MutableInt over = mActiveOverflow.get(name); + if (over != null) { + // We are already actively counting this name in the overflow object. + obj = mCurOverflow; + if (obj != null) { + over.value--; + if (over.value <= 0) { + mActiveOverflow.remove(name); + } + return obj; + } + } + } + + // Huh, they are stopping an active operation but we can't find one! + // That's not good. + Slog.wtf(TAG, "Unable to find object for " + name + " mapsize=" + + mMap.size() + " activeoverflow=" + mActiveOverflow + + " curoverflow=" + mCurOverflow); + return null; + } + + public abstract T instantiateObject(); + } + /* * Get the wakeup reason counter, and create a new one if one * doesn't already exist. @@ -3995,17 +4128,27 @@ public final class BatteryStatsImpl extends BatteryStats { /** * The statistics we have collected for this uid's wake locks. */ - final ArrayMap mWakelockStats = new ArrayMap(); + final OverflowArrayMap mWakelockStats = new OverflowArrayMap() { + @Override public Wakelock instantiateObject() { return new Wakelock(); } + }; /** * The statistics we have collected for this uid's syncs. */ - final ArrayMap mSyncStats = new ArrayMap(); + final OverflowArrayMap mSyncStats = new OverflowArrayMap() { + @Override public StopwatchTimer instantiateObject() { + return new StopwatchTimer(Uid.this, SYNC, null, mOnBatteryTimeBase); + } + }; /** * The statistics we have collected for this uid's jobs. */ - final ArrayMap mJobStats = new ArrayMap(); + final OverflowArrayMap mJobStats = new OverflowArrayMap() { + @Override public StopwatchTimer instantiateObject() { + return new StopwatchTimer(Uid.this, JOB, null, mOnBatteryTimeBase); + } + }; /** * The statistics we have collected for this uid's sensor activations. @@ -4043,17 +4186,17 @@ public final class BatteryStatsImpl extends BatteryStats { @Override public Map getWakelockStats() { - return mWakelockStats; + return mWakelockStats.getMap(); } @Override public Map getSyncStats() { - return mSyncStats; + return mSyncStats.getMap(); } @Override public Map getJobStats() { - return mJobStats; + return mJobStats.getMap(); } @Override @@ -4567,32 +4710,38 @@ public final class BatteryStatsImpl extends BatteryStats { mMobileRadioActiveCount.reset(false); } - for (int iw=mWakelockStats.size()-1; iw>=0; iw--) { - Wakelock wl = mWakelockStats.valueAt(iw); + final ArrayMap wakeStats = mWakelockStats.getMap(); + for (int iw=wakeStats.size()-1; iw>=0; iw--) { + Wakelock wl = wakeStats.valueAt(iw); if (wl.reset()) { - mWakelockStats.removeAt(iw); + wakeStats.removeAt(iw); } else { active = true; } } - for (int is=mSyncStats.size()-1; is>=0; is--) { - StopwatchTimer timer = mSyncStats.valueAt(is); + mWakelockStats.cleanup(); + final ArrayMap syncStats = mSyncStats.getMap(); + for (int is=syncStats.size()-1; is>=0; is--) { + StopwatchTimer timer = syncStats.valueAt(is); if (timer.reset(false)) { - mSyncStats.removeAt(is); + syncStats.removeAt(is); timer.detach(); } else { active = true; } } - for (int ij=mJobStats.size()-1; ij>=0; ij--) { - StopwatchTimer timer = mJobStats.valueAt(ij); + mSyncStats.cleanup(); + final ArrayMap jobStats = mJobStats.getMap(); + for (int ij=jobStats.size()-1; ij>=0; ij--) { + StopwatchTimer timer = jobStats.valueAt(ij); if (timer.reset(false)) { - mJobStats.removeAt(ij); + jobStats.removeAt(ij); timer.detach(); } else { active = true; } } + mJobStats.cleanup(); for (int ise=mSensorStats.size()-1; ise>=0; ise--) { Sensor s = mSensorStats.valueAt(ise); if (s.reset()) { @@ -4687,27 +4836,30 @@ public final class BatteryStatsImpl extends BatteryStats { } void writeToParcelLocked(Parcel out, long elapsedRealtimeUs) { - int NW = mWakelockStats.size(); + final ArrayMap wakeStats = mWakelockStats.getMap(); + int NW = wakeStats.size(); out.writeInt(NW); for (int iw=0; iw syncStats = mSyncStats.getMap(); + int NS = syncStats.size(); out.writeInt(NS); for (int is=0; is jobStats = mJobStats.getMap(); + int NJ = jobStats.size(); out.writeInt(NJ); for (int ij=0; ij