From c55c7389d4c5b0542a727fb56607e88b3e4d6e65 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Mon, 7 Jun 2021 07:33:47 -0700 Subject: [PATCH] Hold wakelock earlier. Start holding a wakelock from the instant we decide to start running a job instead of waiting until the JobService is bound. This ensures that: 1. The CPU doesn't go to suspend between when we tell controllers that the job is starting, when we start the service, and when we successfully bind to the service 2. Execution timing is accurate and we don't accidentally count CPU sleep time as execution time. Bug: 189985233 Test: atest frameworks/base/services/tests/mockingservicestests/src/com/android/server/job Test: atest frameworks/base/services/tests/servicestests/src/com/android/server/job Test: atest CtsJobSchedulerTestCases Change-Id: I95afa1e8d78448b8c8ee31ce5d9b68821318a23a --- .../server/job/JobConcurrencyManager.java | 54 ++++++++++++------- .../server/job/JobSchedulerService.java | 14 ++++- .../android/server/job/JobServiceContext.java | 42 ++++----------- .../job/controllers/TimeController.java | 28 +++------- .../server/job/JobSchedulerServiceTest.java | 5 -- 5 files changed, 61 insertions(+), 82 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java index 5a13a849a352c..143c0f1ffdf5f 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java @@ -804,28 +804,42 @@ class JobConcurrencyManager { @WorkType final int workType) { final List controllers = mService.mControllers; final int numControllers = controllers.size(); - for (int ic = 0; ic < numControllers; ic++) { - controllers.get(ic).prepareForExecutionLocked(jobStatus); - } - final PackageStats packageStats = - getPkgStatsLocked(jobStatus.getSourceUserId(), jobStatus.getSourcePackageName()); - packageStats.adjustStagedCount(false, jobStatus.shouldTreatAsExpeditedJob()); - if (!worker.executeRunnableJob(jobStatus, workType)) { - Slog.e(TAG, "Error executing " + jobStatus); - mWorkCountTracker.onStagedJobFailed(workType); + final PowerManager.WakeLock wl = + mPowerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, jobStatus.getTag()); + wl.setWorkSource(mService.deriveWorkSource( + jobStatus.getSourceUid(), jobStatus.getSourcePackageName())); + wl.setReferenceCounted(false); + // Since the quota controller will start counting from the time prepareForExecutionLocked() + // is called, hold a wakelock to make sure the CPU doesn't suspend between that call and + // when the service actually starts. + wl.acquire(); + try { for (int ic = 0; ic < numControllers; ic++) { - controllers.get(ic).unprepareFromExecutionLocked(jobStatus); + controllers.get(ic).prepareForExecutionLocked(jobStatus); } - } else { - mRunningJobs.add(jobStatus); - mWorkCountTracker.onJobStarted(workType); - packageStats.adjustRunningCount(true, jobStatus.shouldTreatAsExpeditedJob()); - mActivePkgStats.add( - jobStatus.getSourceUserId(), jobStatus.getSourcePackageName(), packageStats); - } - final List pendingJobs = mService.mPendingJobs; - if (pendingJobs.remove(jobStatus)) { - mService.mJobPackageTracker.noteNonpending(jobStatus); + final PackageStats packageStats = getPkgStatsLocked( + jobStatus.getSourceUserId(), jobStatus.getSourcePackageName()); + packageStats.adjustStagedCount(false, jobStatus.shouldTreatAsExpeditedJob()); + if (!worker.executeRunnableJob(jobStatus, workType)) { + Slog.e(TAG, "Error executing " + jobStatus); + mWorkCountTracker.onStagedJobFailed(workType); + for (int ic = 0; ic < numControllers; ic++) { + controllers.get(ic).unprepareFromExecutionLocked(jobStatus); + } + } else { + mRunningJobs.add(jobStatus); + mWorkCountTracker.onJobStarted(workType); + packageStats.adjustRunningCount(true, jobStatus.shouldTreatAsExpeditedJob()); + mActivePkgStats.add( + jobStatus.getSourceUserId(), jobStatus.getSourcePackageName(), + packageStats); + } + final List pendingJobs = mService.mPendingJobs; + if (pendingJobs.remove(jobStatus)) { + mService.mJobPackageTracker.noteNonpending(jobStatus); + } + } finally { + wl.release(); } } diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index b70df84bede71..848118d08c7ef 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -917,8 +917,18 @@ public class JobSchedulerService extends com.android.server.SystemService return mConstants; } - public boolean isChainedAttributionEnabled() { - return WorkSource.isChainedBatteryAttributionEnabled(getContext()); + @NonNull + public WorkSource deriveWorkSource(int sourceUid, @Nullable String sourcePackageName) { + if (WorkSource.isChainedBatteryAttributionEnabled(getContext())) { + WorkSource ws = new WorkSource(); + ws.createWorkChain() + .addNode(sourceUid, sourcePackageName) + .addNode(Process.SYSTEM_UID, "JobScheduler"); + return ws; + } else { + return sourcePackageName == null + ? new WorkSource(sourceUid) : new WorkSource(sourceUid, sourcePackageName); + } } @Nullable diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java b/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java index 3fa1c12ce762f..2ebd1ade13123 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java @@ -42,7 +42,6 @@ import android.os.Message; import android.os.PowerManager; import android.os.RemoteException; import android.os.UserHandle; -import android.os.WorkSource; import android.util.EventLog; import android.util.IndentingPrintWriter; import android.util.Slog; @@ -109,6 +108,7 @@ public final class JobServiceContext implements ServiceConnection { private final Object mLock; private final IBatteryStats mBatteryStats; private final JobPackageTracker mJobPackageTracker; + private final PowerManager mPowerManager; private PowerManager.WakeLock mWakeLock; // Execution state. @@ -205,6 +205,7 @@ public final class JobServiceContext implements ServiceConnection { mCallbackHandler = new JobServiceHandler(looper); mJobConcurrencyManager = concurrencyManager; mCompletedListener = service; + mPowerManager = mContext.getSystemService(PowerManager.class); mAvailable = true; mVerb = VERB_FINISHED; mPreferredUid = NO_PREFERRED_UID; @@ -271,6 +272,12 @@ public final class JobServiceContext implements ServiceConnection { // it was inflated from disk with not-yet-coherent delay/deadline bounds. job.clearPersistedUtcTimes(); + mWakeLock = mPowerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, job.getTag()); + mWakeLock.setWorkSource( + mService.deriveWorkSource(job.getSourceUid(), job.getSourcePackageName())); + mWakeLock.setReferenceCounted(false); + mWakeLock.acquire(); + mVerb = VERB_BINDING; scheduleOpTimeOutLocked(); final Intent intent = new Intent().setComponent(job.getServiceComponent()); @@ -306,6 +313,7 @@ public final class JobServiceContext implements ServiceConnection { mRunningCallback = null; mParams = null; mExecutionStartTimeElapsed = 0L; + mWakeLock.release(); mVerb = VERB_FINISHED; removeOpTimeOutLocked(); return false; @@ -495,42 +503,10 @@ public final class JobServiceContext implements ServiceConnection { return; } this.service = IJobService.Stub.asInterface(service); - final PowerManager pm = - (PowerManager) mContext.getSystemService(Context.POWER_SERVICE); - PowerManager.WakeLock wl = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, - runningJob.getTag()); - wl.setWorkSource(deriveWorkSource(runningJob)); - wl.setReferenceCounted(false); - wl.acquire(); - - // We use a new wakelock instance per job. In rare cases there is a race between - // teardown following job completion/cancellation and new job service spin-up - // such that if we simply assign mWakeLock to be the new instance, we orphan - // the currently-live lock instead of cleanly replacing it. Watch for this and - // explicitly fast-forward the release if we're in that situation. - if (mWakeLock != null) { - Slog.w(TAG, "Bound new job " + runningJob + " but live wakelock " + mWakeLock - + " tag=" + mWakeLock.getTag()); - mWakeLock.release(); - } - mWakeLock = wl; doServiceBoundLocked(); } } - private WorkSource deriveWorkSource(JobStatus runningJob) { - final int jobUid = runningJob.getSourceUid(); - if (WorkSource.isChainedBatteryAttributionEnabled(mContext)) { - WorkSource workSource = new WorkSource(); - workSource.createWorkChain() - .addNode(jobUid, null) - .addNode(android.os.Process.SYSTEM_UID, "JobScheduler"); - return workSource; - } else { - return new WorkSource(jobUid); - } - } - /** If the client service crashes we reschedule this job and clean up. */ @Override public void onServiceDisconnected(ComponentName name) { diff --git a/apex/jobscheduler/service/java/com/android/server/job/controllers/TimeController.java b/apex/jobscheduler/service/java/com/android/server/job/controllers/TimeController.java index e8ebfb53fde8a..98e37a13b0290 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/controllers/TimeController.java +++ b/apex/jobscheduler/service/java/com/android/server/job/controllers/TimeController.java @@ -18,11 +18,9 @@ package com.android.server.job.controllers; import static com.android.server.job.JobSchedulerService.sElapsedRealtimeClock; -import android.annotation.Nullable; import android.app.AlarmManager; import android.app.AlarmManager.OnAlarmListener; import android.content.Context; -import android.os.Process; import android.os.UserHandle; import android.os.WorkSource; import android.util.IndentingPrintWriter; @@ -62,8 +60,6 @@ public final class TimeController extends StateController { private long mNextDelayExpiredElapsedMillis; private volatile long mLastFiredDelayExpiredElapsedMillis; - private final boolean mChainedAttributionEnabled; - private AlarmManager mAlarmService = null; /** List of tracked jobs, sorted asc. by deadline */ private final List mTrackedJobs = new LinkedList<>(); @@ -73,7 +69,6 @@ public final class TimeController extends StateController { mNextJobExpiredElapsedMillis = Long.MAX_VALUE; mNextDelayExpiredElapsedMillis = Long.MAX_VALUE; - mChainedAttributionEnabled = mService.isChainedAttributionEnabled(); } /** @@ -117,7 +112,8 @@ public final class TimeController extends StateController { it.add(job); job.setTrackingController(JobStatus.TRACKING_TIME); - WorkSource ws = deriveWorkSource(job.getSourceUid(), job.getSourcePackageName()); + WorkSource ws = + mService.deriveWorkSource(job.getSourceUid(), job.getSourcePackageName()); // Only update alarms if the job would be ready with the relevant timing constraint // satisfied. @@ -165,7 +161,7 @@ public final class TimeController extends StateController { } else if (wouldBeReadyWithConstraintLocked(job, JobStatus.CONSTRAINT_DEADLINE)) { // This job's deadline is earlier than the current set alarm. Update the alarm. setDeadlineExpiredAlarmLocked(job.getLatestRunTimeElapsed(), - deriveWorkSource(job.getSourceUid(), job.getSourcePackageName())); + mService.deriveWorkSource(job.getSourceUid(), job.getSourcePackageName())); } } if (job.hasTimingDelayConstraint() @@ -177,7 +173,7 @@ public final class TimeController extends StateController { && wouldBeReadyWithConstraintLocked(job, JobStatus.CONSTRAINT_TIMING_DELAY)) { // This job's delay is earlier than the current set alarm. Update the alarm. setDelayExpiredAlarmLocked(job.getEarliestRunTime(), - deriveWorkSource(job.getSourceUid(), job.getSourcePackageName())); + mService.deriveWorkSource(job.getSourceUid(), job.getSourcePackageName())); } } } @@ -248,7 +244,7 @@ public final class TimeController extends StateController { } } setDeadlineExpiredAlarmLocked(nextExpiryTime, - deriveWorkSource(nextExpiryUid, nextExpiryPackageName)); + mService.deriveWorkSource(nextExpiryUid, nextExpiryPackageName)); } } @@ -312,19 +308,7 @@ public final class TimeController extends StateController { mStateChangedListener.onControllerStateChanged(); } setDelayExpiredAlarmLocked(nextDelayTime, - deriveWorkSource(nextDelayUid, nextDelayPackageName)); - } - } - - private WorkSource deriveWorkSource(int uid, @Nullable String packageName) { - if (mChainedAttributionEnabled) { - WorkSource ws = new WorkSource(); - ws.createWorkChain() - .addNode(uid, packageName) - .addNode(Process.SYSTEM_UID, "JobScheduler"); - return ws; - } else { - return packageName == null ? new WorkSource(uid) : new WorkSource(uid, packageName); + mService.deriveWorkSource(nextDelayUid, nextDelayPackageName)); } } diff --git a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java index f2bb47bfb8ad1..1136cac8f0555 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java @@ -90,11 +90,6 @@ public class JobSchedulerServiceTest { super(context); mAppStateTracker = mock(AppStateTrackerImpl.class); } - - @Override - public boolean isChainedAttributionEnabled() { - return false; - } } @Before