From eb69f2ead7a39a75308369dced800aba161bfafc Mon Sep 17 00:00:00 2001 From: Edgar Arriaga Date: Tue, 1 Feb 2022 18:58:34 -0800 Subject: [PATCH 1/3] Fix for compaction bailing out when MAX_RW_COUNT bytes are sent to compaction Compaction uses process_madvise which internally makes use of iovec which imposes certain restrictions on the amount of data that can be sent, previously we had added a constraint of the total vmas sent within a single syscall determined by UIO_MAXIOV. However, in this CL we also added a cap on the amount of bytes sent per syscall up to the maximum supported MAX_RW_COUNT and split the VMA into chunks which end up being sent in different syscalls. Test: Verified ZRAM changes during compaction and added logging to verify that all VMAs where processed Bug: 205658049 Change-Id: Ib478397a1199d31a85606b0a38ab1b673b2333fc --- ...m_android_server_am_CachedAppOptimizer.cpp | 90 ++++++++++++++----- 1 file changed, 68 insertions(+), 22 deletions(-) diff --git a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp index 94bc22a05d7ad..f0271f806e132 100644 --- a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp +++ b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp @@ -57,6 +57,19 @@ using android::base::unique_fd; #define ASYNC_RECEIVED_WHILE_FROZEN (2) #define TXNS_PENDING_WHILE_FROZEN (4) +#define MAX_RW_COUNT (INT_MAX & PAGE_MASK) + +// Defines the maximum amount of VMAs we can send per process_madvise syscall. +// Currently this is set to UIO_MAXIOV which is the maximum segments allowed by +// iovec implementation used by process_madvise syscall +#define MAX_VMAS_PER_COMPACTION UIO_MAXIOV + +// Maximum bytes that we can send per process_madvise syscall once this limit +// is reached we split the remaining VMAs into another syscall. The MAX_RW_COUNT +// limit is imposed by iovec implementation. However, if you want to use a smaller +// limit, it has to be a page aligned value, otherwise, compaction would fail. +#define MAX_BYTES_PER_COMPACTION MAX_RW_COUNT + namespace android { static bool cancelRunningCompaction; @@ -70,12 +83,9 @@ static inline void compactProcessProcfs(int pid, const std::string& compactionTy } // Compacts a set of VMAs for pid using an madviseType accepted by process_madvise syscall -// On success returns the total bytes that where compacted. On failure it returns -// a negative error code from the standard linux error codes. +// Returns the total bytes that where madvised. static int64_t compactMemory(const std::vector& vmas, int pid, int madviseType) { - // UIO_MAXIOV is currently a small value and we might have more addresses - // we do multiple syscalls if we exceed its maximum - static struct iovec vmasToKernel[UIO_MAXIOV]; + static struct iovec vmasToKernel[MAX_VMAS_PER_COMPACTION]; if (vmas.empty()) { return 0; @@ -89,33 +99,64 @@ static int64_t compactMemory(const std::vector& vmas, int pid, int madviseT compactionInProgress = true; cancelRunningCompaction = false; - int64_t totalBytesCompacted = 0; - for (int iBase = 0; iBase < vmas.size(); iBase += UIO_MAXIOV) { - if (CC_UNLIKELY(cancelRunningCompaction)) { - // There could be a significant delay betweenwhen a compaction - // is requested and when it is handled during this time - // our OOM adjust could have improved. + int64_t totalBytesProcessed = 0; + + int64_t vmaOffset = 0; + for (int iVma = 0; iVma < vmas.size();) { + uint64_t bytesSentToCompact = 0; + int iVec = 0; + while (iVec < MAX_VMAS_PER_COMPACTION && iVma < vmas.size()) { + if (CC_UNLIKELY(cancelRunningCompaction)) { + // There could be a significant delay between when a compaction + // is requested and when it is handled during this time our + // OOM adjust could have improved. + break; + } + + uint64_t vmaStart = vmas[iVma].start + vmaOffset; + uint64_t vmaSize = vmas[iVma].end - vmaStart; + if (vmaSize == 0) { + goto next_vma; + } + vmasToKernel[iVec].iov_base = (void*)vmaStart; + if (vmaSize > MAX_BYTES_PER_COMPACTION - bytesSentToCompact) { + // Exceeded the max bytes that could be sent, so clamp + // the end to avoid exceeding limit and issue compaction + vmaSize = MAX_BYTES_PER_COMPACTION - bytesSentToCompact; + } + + vmasToKernel[iVec].iov_len = vmaSize; + bytesSentToCompact += vmaSize; + ++iVec; + if (bytesSentToCompact >= MAX_BYTES_PER_COMPACTION) { + // Ran out of bytes within iovec, dispatch compaction. + vmaOffset += vmaSize; + break; + } + + next_vma: + // Finished current VMA, and have more bytes remaining + vmaOffset = 0; + ++iVma; + } + + if (cancelRunningCompaction) { cancelRunningCompaction = false; break; } - int totalVmasToKernel = std::min(UIO_MAXIOV, (int)(vmas.size() - iBase)); - for (int iVec = 0, iVma = iBase; iVec < totalVmasToKernel; ++iVec, ++iVma) { - vmasToKernel[iVec].iov_base = (void*)vmas[iVma].start; - vmasToKernel[iVec].iov_len = vmas[iVma].end - vmas[iVma].start; - } - auto bytesCompacted = - process_madvise(pidfd, vmasToKernel, totalVmasToKernel, madviseType, 0); - if (CC_UNLIKELY(bytesCompacted == -1)) { + auto bytesProcessed = process_madvise(pidfd, vmasToKernel, iVec, madviseType, 0); + + if (CC_UNLIKELY(bytesProcessed == -1)) { compactionInProgress = false; return -errno; } - totalBytesCompacted += bytesCompacted; + totalBytesProcessed += bytesProcessed; } compactionInProgress = false; - return totalBytesCompacted; + return totalBytesProcessed; } static int getFilePageAdvice(const Vma& vma) { @@ -138,7 +179,12 @@ static int getAnyPageAdvice(const Vma& vma) { } // Perform a full process compaction using process_madvise syscall -// reading all filtering VMAs and filtering pages as specified by pageFilter +// using the madvise behavior defined by vmaToAdviseFunc per VMA. +// +// Currently supported behaviors are MADV_COLD and MADV_PAGEOUT. +// +// Returns the total number of bytes compacted or forwards an +// process_madvise error. static int64_t compactProcess(int pid, VmaToAdviseFunc vmaToAdviseFunc) { ProcMemInfo meminfo(pid); std::vector pageoutVmas, coldVmas; From cbb6b2be373dcd44a3705937745dd3f66196c5a3 Mon Sep 17 00:00:00 2001 From: Edgar Arriaga Date: Mon, 7 Feb 2022 16:46:56 -0800 Subject: [PATCH 2/3] Improve compaction by skipping bad VMAs instead of fully bailing out Previously when a VMA failed compaction for any reason the system would stop trying to compact all the rest of the VMAs and bail out. However, there are multiple reasons that a VMA can fail due to not being reclaimable with -EINVAL, in such instances we will just skip the VMA and keep going with the rest of the VMAs and any other error will still cause compaction to bail out as it would likely be irrecoverable. Test: Manual. Verified that once a VMA errors with -EINVAL, it continues with the rest of the VMAs and other errors bail out. Bug: 205658049 Change-Id: Ifc190e371c4dce0eaa6dbab104aa0b666e06027d --- .../com_android_server_am_CachedAppOptimizer.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp index f0271f806e132..0271277acb7a2 100644 --- a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp +++ b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp @@ -84,6 +84,9 @@ static inline void compactProcessProcfs(int pid, const std::string& compactionTy // Compacts a set of VMAs for pid using an madviseType accepted by process_madvise syscall // Returns the total bytes that where madvised. +// +// If any VMA fails compaction due to -EINVAL it will be skipped and continue. +// However, if it fails for any other reason, it will bail out and forward the error static int64_t compactMemory(const std::vector& vmas, int pid, int madviseType) { static struct iovec vmasToKernel[MAX_VMAS_PER_COMPACTION]; @@ -148,8 +151,15 @@ static int64_t compactMemory(const std::vector& vmas, int pid, int madviseT auto bytesProcessed = process_madvise(pidfd, vmasToKernel, iVec, madviseType, 0); if (CC_UNLIKELY(bytesProcessed == -1)) { - compactionInProgress = false; - return -errno; + if (errno == EINVAL) { + // This error is somewhat common due to an unevictable VMA if this is + // the case silently skip the bad VMA and continue compacting the rest. + continue; + } else { + // Forward irrecoverable errors and bail out compaction + compactionInProgress = false; + return -errno; + } } totalBytesProcessed += bytesProcessed; From 9acf12b37c63b1ae978b0ea491e76b057b971f05 Mon Sep 17 00:00:00 2001 From: Edgar Arriaga Date: Wed, 9 Feb 2022 12:41:20 -0800 Subject: [PATCH 3/3] Improve compaction to abort when system changes to awake state While the screen is off we sometimes issue compactions as they are not disruptive to the user. However, it is possible to wake up and have compactions running or scheduled so this may lead to jank. This patch cancels any pending or running compactions that were scheduled to happen during this non interactive type when the system becomes awake to avoid jank. Test: Manual Bug: 214654755 Change-Id: Ie9b2cbaa7093d63e77f8666d1e6f050ab5610a1d --- .../com/android/server/am/ActivityManagerService.java | 1 + .../java/com/android/server/am/CachedAppOptimizer.java | 10 ++++++++++ .../core/java/com/android/server/am/OomAdjuster.java | 4 ++++ .../jni/com_android_server_am_CachedAppOptimizer.cpp | 1 + 4 files changed, 16 insertions(+) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 330d2ddc0a945..a9e02c675a9ce 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -6357,6 +6357,7 @@ public class ActivityManagerService extends IActivityManager.Stub reportCurWakefulnessUsageEvent(); mActivityTaskManager.onScreenAwakeChanged(isAwake); mOomAdjProfiler.onWakefulnessChanged(wakefulness); + mOomAdjuster.onWakefulnessChanged(wakefulness); } updateOomAdjLocked(OomAdjuster.OOM_ADJ_REASON_UI_VISIBILITY); } diff --git a/services/core/java/com/android/server/am/CachedAppOptimizer.java b/services/core/java/com/android/server/am/CachedAppOptimizer.java index 6bb5aa148e599..3a704f781be95 100644 --- a/services/core/java/com/android/server/am/CachedAppOptimizer.java +++ b/services/core/java/com/android/server/am/CachedAppOptimizer.java @@ -28,6 +28,7 @@ import android.net.Uri; import android.os.Debug; import android.os.Handler; import android.os.Message; +import android.os.PowerManagerInternal; import android.os.Process; import android.os.SystemClock; import android.os.Trace; @@ -1028,6 +1029,15 @@ public final class CachedAppOptimizer { } } + void onWakefulnessChanged(int wakefulness) { + if(wakefulness == PowerManagerInternal.WAKEFULNESS_AWAKE) { + // Remove any pending compaction we may have scheduled to happen while screen was off + Slog.e(TAG_AM, "Cancel pending or running compactions as system is awake"); + mPendingCompactionProcesses.clear(); + cancelCompaction(); + } + } + @GuardedBy({"mService", "mProcLock"}) void onOomAdjustChanged(int oldAdj, int newAdj, ProcessRecord app) { // Cancel any currently executing compactions diff --git a/services/core/java/com/android/server/am/OomAdjuster.java b/services/core/java/com/android/server/am/OomAdjuster.java index 88ca8a5e8356e..b64b31089500d 100644 --- a/services/core/java/com/android/server/am/OomAdjuster.java +++ b/services/core/java/com/android/server/am/OomAdjuster.java @@ -2590,6 +2590,10 @@ public class OomAdjuster { } } + void onWakefulnessChanged(int wakefulness) { + mCachedAppOptimizer.onWakefulnessChanged(wakefulness); + } + /** Applies the computed oomadj, procstate and sched group values and freezes them in set* */ @GuardedBy({"mService", "mProcLock"}) private boolean applyOomAdjLSP(ProcessRecord app, boolean doingAll, long now, diff --git a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp index 0271277acb7a2..636ca4143a33c 100644 --- a/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp +++ b/services/core/jni/com_android_server_am_CachedAppOptimizer.cpp @@ -113,6 +113,7 @@ static int64_t compactMemory(const std::vector& vmas, int pid, int madviseT // There could be a significant delay between when a compaction // is requested and when it is handled during this time our // OOM adjust could have improved. + LOG(DEBUG) << "Cancelled running compaction for " << pid; break; }