From c5ae4b9e4d00fc5f8fca1bd0e95a26f5033a65c8 Mon Sep 17 00:00:00 2001 From: Winson Chiu Date: Thu, 12 May 2022 03:12:27 +0000 Subject: [PATCH 1/2] Avoid taking PMS snapshot lock when snapshot is still valid Maintains 2 integers, one for snapshot current version and one for snapshot pending version, where the former is updated to the latter whenever a snapshot is rebuilt. Calling threads can compare the volatile integers to see if the snapshot is valid without taking mSnapshotLock. This avoids blocking parallel threads using the snapshot. Snapshot rebuilds are guaranteed to be accurate to the point in time when one is requested, not the time that the snapshot is returned, and this is enforced by caching the most recent pending version before rebuilding the snapshot. The thread that rebuilds will continue with its result regardless of whether the snapshot was invalidated in between pending version read and rebuild finish, and update the current version to the pending version read. If an invalidation came in from the background, this committed version will no longer be equivalent to the latest pending version in the field, and so when the rebuild thread releases, future/waiting threads will still see the snapshot as invalid and cause a further rebuild. This ensures changes are never lost and the snapshot will always eventually be brought up to date. Bug: 232282785 Test: presubmit Change-Id: I99656d573c7cec88069ec2eb35a1c7be48c782e0 --- .../java/com/android/server/pm/Computer.java | 5 +- .../com/android/server/pm/ComputerEngine.java | 13 ++- .../com/android/server/pm/ComputerLocked.java | 2 +- .../server/pm/PackageManagerService.java | 96 +++++++++---------- .../com/android/server/pm/ThreadComputer.java | 52 ---------- 5 files changed, 61 insertions(+), 107 deletions(-) delete mode 100644 services/core/java/com/android/server/pm/ThreadComputer.java diff --git a/services/core/java/com/android/server/pm/Computer.java b/services/core/java/com/android/server/pm/Computer.java index db48a1f630992..eb635500580a9 100644 --- a/services/core/java/com/android/server/pm/Computer.java +++ b/services/core/java/com/android/server/pm/Computer.java @@ -94,12 +94,13 @@ import java.util.Set; @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE) public interface Computer extends PackageDataSnapshot { + int getVersion(); + /** * Administrative statistics: record that the snapshot has been used. Every call * to use() increments the usage counter. */ - default void use() { - } + Computer use(); /** * Fetch the snapshot usage counter. * @return The number of times this snapshot was used. diff --git a/services/core/java/com/android/server/pm/ComputerEngine.java b/services/core/java/com/android/server/pm/ComputerEngine.java index bf9f4fa8a2114..f3c41af05da37 100644 --- a/services/core/java/com/android/server/pm/ComputerEngine.java +++ b/services/core/java/com/android/server/pm/ComputerEngine.java @@ -367,6 +367,8 @@ public class ComputerEngine implements Computer { return (v1 > v2) ? -1 : ((v1 < v2) ? 1 : 0); }; + private final int mVersion; + // The administrative use counter. private int mUsed = 0; @@ -424,7 +426,8 @@ public class ComputerEngine implements Computer { return mLocalAndroidApplication; } - ComputerEngine(PackageManagerService.Snapshot args) { + ComputerEngine(PackageManagerService.Snapshot args, int version) { + mVersion = version; mSettings = new Settings(args.settings); mIsolatedOwners = args.isolatedOwners; mPackages = args.packages; @@ -464,11 +467,17 @@ public class ComputerEngine implements Computer { mService = args.service; } + @Override + public int getVersion() { + return mVersion; + } + /** * Record that the snapshot was used. */ - public final void use() { + public final Computer use() { mUsed++; + return this; } /** diff --git a/services/core/java/com/android/server/pm/ComputerLocked.java b/services/core/java/com/android/server/pm/ComputerLocked.java index af196d51331f1..37070dbe7131f 100644 --- a/services/core/java/com/android/server/pm/ComputerLocked.java +++ b/services/core/java/com/android/server/pm/ComputerLocked.java @@ -30,7 +30,7 @@ import com.android.internal.annotations.VisibleForTesting; public final class ComputerLocked extends ComputerEngine { ComputerLocked(PackageManagerService.Snapshot args) { - super(args); + super(args, -1); } protected ComponentName resolveComponentName() { diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 2cef35fcf0f60..37fa4d1c02a63 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -271,8 +271,8 @@ import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; /** @@ -1038,22 +1038,15 @@ public class PackageManagerService implements PackageSender, TestUtilityService // times during the PackageManagerService constructor but it should not be modified thereafter. private ComputerLocked mLiveComputer; - // A lock-free cache for frequently called functions. - private volatile Computer mSnapshotComputer; + private static final AtomicReference sSnapshot = new AtomicReference<>(); - // If true, the snapshot is invalid (stale). The attribute is static since it may be - // set from outside classes. The attribute may be set to true anywhere, although it - // should only be set true while holding mLock. However, the attribute id guaranteed - // to be set false only while mLock and mSnapshotLock are both held. - private static final AtomicBoolean sSnapshotInvalid = new AtomicBoolean(true); - - static final ThreadLocal sThreadComputer = - ThreadLocal.withInitial(ThreadComputer::new); + // If this differs from Computer#getVersion, the snapshot is invalid (stale). + private static final AtomicInteger sSnapshotPendingVersion = new AtomicInteger(1); /** - * This lock is used to make reads from {@link #sSnapshotInvalid} and - * {@link #mSnapshotComputer} atomic inside {@code snapshotComputer()}. This lock is - * not meant to be used outside that method. This lock must be taken before + * This lock is used to make reads from {@link #sSnapshotPendingVersion} and + * {@link #sSnapshot} atomic inside {@code snapshotComputer()} when the versions mismatch. + * This lock is not meant to be used outside that method. This lock must be taken before * {@link #mLock} is taken. */ private final Object mSnapshotLock = new Object(); @@ -1077,48 +1070,53 @@ public class PackageManagerService implements PackageSender, TestUtilityService // yet invalidated the snapshot. Always give the thread the live computer. return mLiveComputer; } - synchronized (mSnapshotLock) { - // This synchronization block serializes access to the snapshot computer and - // to the code that samples mSnapshotInvalid. - Computer c = mSnapshotComputer; - if (sSnapshotInvalid.getAndSet(false) || (c == null)) { - // The snapshot is invalid if it is marked as invalid or if it is null. If it - // is null, then it is currently being rebuilt by rebuildSnapshot(). - synchronized (mLock) { - // Rebuild the snapshot if it is invalid. Note that the snapshot might be - // invalidated as it is rebuilt. However, the snapshot is still - // self-consistent (the lock is being held) and is current as of the time - // this function is entered. - rebuildSnapshot(); - // Guaranteed to be non-null. mSnapshotComputer is only be set to null - // temporarily in rebuildSnapshot(), which is guarded by mLock(). Since - // the mLock is held in this block and since rebuildSnapshot() is - // complete, the attribute can not now be null. - c = mSnapshotComputer; - } + var oldSnapshot = sSnapshot.get(); + var pendingVersion = sSnapshotPendingVersion.get(); + + if (oldSnapshot != null && oldSnapshot.getVersion() == pendingVersion) { + return oldSnapshot.use(); + } + + synchronized (mSnapshotLock) { + // Re-capture pending version in case a new invalidation occurred since last check + var rebuildSnapshot = sSnapshot.get(); + var rebuildVersion = sSnapshotPendingVersion.get(); + + // Check the versions again while the lock is held, in case the rebuild time caused + // multiple threads to wait on the snapshot lock. When the first thread finishes + // a rebuild, the snapshot is now valid and the other waiting threads can use it + // without kicking off their own rebuilds. + if (rebuildSnapshot != null && rebuildSnapshot.getVersion() == rebuildVersion) { + return rebuildSnapshot.use(); + } + + synchronized (mLock) { + // Fetch version one last time to ensure that the rebuilt snapshot matches + // the latest invalidation, which could have come in between entering the + // SnapshotLock and mLock sync blocks. + rebuildVersion = sSnapshotPendingVersion.get(); + + // Build the snapshot for this version + var newSnapshot = rebuildSnapshot(rebuildSnapshot, rebuildVersion); + sSnapshot.set(newSnapshot); + return newSnapshot.use(); } - c.use(); - return c; } } - /** - * Rebuild the cached computer. mSnapshotComputer is temporarily set to null to block other - * threads from using the invalid computer until it is rebuilt. - */ @GuardedBy({ "mLock", "mSnapshotLock"}) - private void rebuildSnapshot() { - final long now = SystemClock.currentTimeMicro(); - final int hits = mSnapshotComputer == null ? -1 : mSnapshotComputer.getUsed(); - mSnapshotComputer = null; - final Snapshot args = new Snapshot(Snapshot.SNAPPED); - mSnapshotComputer = new ComputerEngine(args); - final long done = SystemClock.currentTimeMicro(); + private Computer rebuildSnapshot(@Nullable Computer oldSnapshot, int newVersion) { + var now = SystemClock.currentTimeMicro(); + var hits = oldSnapshot == null ? -1 : oldSnapshot.getUsed(); + var args = new Snapshot(Snapshot.SNAPPED); + var newSnapshot = new ComputerEngine(args, newVersion); + var done = SystemClock.currentTimeMicro(); if (mSnapshotStatistics != null) { mSnapshotStatistics.rebuild(now, done, hits); } + return newSnapshot; } /** @@ -1138,7 +1136,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService if (TRACE_SNAPSHOTS) { Log.i(TAG, "snapshot: onChange(" + what + ")"); } - sSnapshotInvalid.set(true); + sSnapshotPendingVersion.incrementAndGet(); } /** @@ -1665,7 +1663,6 @@ public class PackageManagerService implements PackageSender, TestUtilityService mRequiredSdkSandboxPackage = testParams.requiredSdkSandboxPackage; mLiveComputer = createLiveComputer(); - mSnapshotComputer = null; mSnapshotStatistics = null; mPackages.putAll(testParams.packages); @@ -1855,9 +1852,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService // cached computer is the same as the live computer until the end of the // constructor, at which time the invalidation method updates it. mSnapshotStatistics = new SnapshotStatistics(); - sSnapshotInvalid.set(true); + sSnapshotPendingVersion.incrementAndGet(); mLiveComputer = createLiveComputer(); - mSnapshotComputer = null; registerObservers(true); } diff --git a/services/core/java/com/android/server/pm/ThreadComputer.java b/services/core/java/com/android/server/pm/ThreadComputer.java deleted file mode 100644 index f603e635f75db..0000000000000 --- a/services/core/java/com/android/server/pm/ThreadComputer.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright (C) 2021 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.pm; - -/** - * This class records the Computer being used by a thread and the Computer's reference - * count. There is a thread-local copy of this class. - */ -public final class ThreadComputer implements AutoCloseable { - Computer mComputer = null; - int mRefCount = 0; - - void acquire(Computer c) { - if (mRefCount != 0 && mComputer != c) { - throw new RuntimeException("computer mismatch, count = " + mRefCount); - } - mComputer = c; - mRefCount++; - } - - void acquire() { - if (mRefCount == 0 || mComputer == null) { - throw new RuntimeException("computer acquire on empty ref count"); - } - mRefCount++; - } - - void release() { - if (--mRefCount == 0) { - mComputer = null; - } - } - - @Override - public void close() { - release(); - } -} From 2a5f1edc4165ace418dd71c01ce978fe9c7283d3 Mon Sep 17 00:00:00 2001 From: Winson Chiu Date: Thu, 12 May 2022 15:43:51 +0000 Subject: [PATCH 2/2] Fix PMS recordInitialState on retry Need to recapture the initial state so that the retry on the implementation lambda works as expected. Otherwise it will compare a stale, always broken state on the retry. Also adds a safety check in the commitStateMutation method to see if the calling thread already has mLock. If it does, the state change check can be skipped since no snapshot changes can come in while the lock is held. Bug: 232061163 Test: atest android.packageinstaller.install.cts.SessionTest#setAppCategory Change-Id: I78cd6a32782ee64a705ba5faa6a72a7bf8a95f03 --- .../server/pm/PackageManagerService.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 37fa4d1c02a63..96f00414c494e 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -5384,10 +5384,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService @Override public void setApplicationCategoryHint(String packageName, int categoryHint, String callerPackageName) { - final PackageStateMutator.InitialState initialState = recordInitialState(); - - final FunctionalUtils.ThrowingFunction - implementation = computer -> { + final FunctionalUtils.ThrowingBiFunction implementation = (initialState, computer) -> { if (computer.getInstantAppPackageName(Binder.getCallingUid()) != null) { throw new SecurityException( "Instant applications don't have access to this method"); @@ -5415,12 +5413,13 @@ public class PackageManagerService implements PackageSender, TestUtilityService } }; - PackageStateMutator.Result result = implementation.apply(snapshotComputer()); + PackageStateMutator.Result result = + implementation.apply(recordInitialState(), snapshotComputer()); if (result != null && result.isStateChanged() && !result.isSpecificPackageNull()) { // TODO: Specific return value of what state changed? // The installer on record might have changed, retry with lock synchronized (mPackageStateWriteLock) { - result = implementation.apply(snapshotComputer()); + result = implementation.apply(recordInitialState(), snapshotComputer()); } } @@ -7152,9 +7151,19 @@ public class PackageManagerService implements PackageSender, TestUtilityService public PackageStateMutator.Result commitPackageStateMutation( @Nullable PackageStateMutator.InitialState initialState, @NonNull String packageName, @NonNull Consumer consumer) { + PackageStateMutator.Result result = null; + if (Thread.holdsLock(mPackageStateWriteLock)) { + // If the thread is already holding the lock, this is likely a retry based on a prior + // failure, and re-calculating whether a state change occurred can be skipped. + result = PackageStateMutator.Result.SUCCESS; + } synchronized (mPackageStateWriteLock) { - final PackageStateMutator.Result result = mPackageStateMutator.generateResult( - initialState, mChangedPackagesTracker.getSequenceNumber()); + if (result == null) { + // If the thread wasn't previously holding, this is a first-try commit and so a + // state change may have happened. + result = mPackageStateMutator.generateResult( + initialState, mChangedPackagesTracker.getSequenceNumber()); + } if (result != PackageStateMutator.Result.SUCCESS) { return result; }