From 54adbb2bd72a887bb1a5a83ce228b828895c49e9 Mon Sep 17 00:00:00 2001 From: Cody Northrop Date: Mon, 23 Oct 2023 15:39:38 +0000 Subject: EGL: Close Multifile Blobcache files after mapping When loading entries from disk, we don't need to keep the files open after mapping their contents. Per mmap documentation: After the mmap() call has returned, the file descriptor, fd, can be closed immediately without invalidating the mapping. https://man7.org/linux/man-pages/man2/mmap.2.html This will prevent consuming excessive file descriptors, which are a limited resource. Added new test that ensures file descriptors do not remain open. Test: libEGL_test, EGL_test, restricted_trace_perf.py Bug: b/286809755 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5f8117acd45348704629a8aa7bd2169a5ad6a547) Merged-In: I6317fdbce340a8e7cbf3020ad41386cf9915dd2d Change-Id: I6317fdbce340a8e7cbf3020ad41386cf9915dd2d --- opengl/libs/EGL/MultifileBlobCache.cpp | 16 ++++++--- opengl/libs/EGL/MultifileBlobCache_test.cpp | 54 +++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/opengl/libs/EGL/MultifileBlobCache.cpp b/opengl/libs/EGL/MultifileBlobCache.cpp index 7ffdac7ea7..ed3c616b92 100644 --- a/opengl/libs/EGL/MultifileBlobCache.cpp +++ b/opengl/libs/EGL/MultifileBlobCache.cpp @@ -48,9 +48,8 @@ namespace { void freeHotCacheEntry(android::MultifileHotCache& entry) { if (entry.entryFd != -1) { // If we have an fd, then this entry was added to hot cache via INIT or GET - // We need to unmap and close the entry + // We need to unmap the entry munmap(entry.entryBuffer, entry.entrySize); - close(entry.entryFd); } else { // Otherwise, this was added to hot cache during SET, so it was never mapped // and fd was only on the deferred thread. @@ -143,6 +142,7 @@ MultifileBlobCache::MultifileBlobCache(size_t maxKeySize, size_t maxValueSize, s if (result != sizeof(MultifileHeader)) { ALOGE("Error reading MultifileHeader from cache entry (%s): %s", fullPath.c_str(), std::strerror(errno)); + close(fd); return; } @@ -152,6 +152,7 @@ MultifileBlobCache::MultifileBlobCache(size_t maxKeySize, size_t maxValueSize, s if (remove(fullPath.c_str()) != 0) { ALOGE("Error removing %s: %s", fullPath.c_str(), std::strerror(errno)); } + close(fd); continue; } @@ -161,6 +162,10 @@ MultifileBlobCache::MultifileBlobCache(size_t maxKeySize, size_t maxValueSize, s // Memory map the file uint8_t* mappedEntry = reinterpret_cast( mmap(nullptr, fileSize, PROT_READ, MAP_PRIVATE, fd, 0)); + + // We can close the file now and the mmap will remain + close(fd); + if (mappedEntry == MAP_FAILED) { ALOGE("Failed to mmap cacheEntry, error: %s", std::strerror(errno)); return; @@ -206,13 +211,11 @@ MultifileBlobCache::MultifileBlobCache(size_t maxKeySize, size_t maxValueSize, s if (!addToHotCache(entryHash, fd, mappedEntry, fileSize)) { ALOGE("INIT Failed to add %u to hot cache", entryHash); munmap(mappedEntry, fileSize); - close(fd); return; } } else { // If we're not keeping it in hot cache, unmap it now munmap(mappedEntry, fileSize); - close(fd); } } closedir(dir); @@ -401,9 +404,12 @@ EGLsizeiANDROID MultifileBlobCache::get(const void* key, EGLsizeiANDROID keySize // Memory map the file cacheEntry = reinterpret_cast(mmap(nullptr, fileSize, PROT_READ, MAP_PRIVATE, fd, 0)); + + // We can close the file now and the mmap will remain + close(fd); + if (cacheEntry == MAP_FAILED) { ALOGE("Failed to mmap cacheEntry, error: %s", std::strerror(errno)); - close(fd); return 0; } diff --git a/opengl/libs/EGL/MultifileBlobCache_test.cpp b/opengl/libs/EGL/MultifileBlobCache_test.cpp index dbee13bb2e..1639be6480 100644 --- a/opengl/libs/EGL/MultifileBlobCache_test.cpp +++ b/opengl/libs/EGL/MultifileBlobCache_test.cpp @@ -42,6 +42,8 @@ protected: virtual void TearDown() { mMBC.reset(); } + int getFileDescriptorCount(); + std::unique_ptr mTempFile; std::unique_ptr mMBC; }; @@ -216,4 +218,56 @@ TEST_F(MultifileBlobCacheTest, CacheMinKeyAndValueSizeSucceeds) { ASSERT_EQ('y', buf[0]); } +int MultifileBlobCacheTest::getFileDescriptorCount() { + DIR* directory = opendir("/proc/self/fd"); + + int fileCount = 0; + struct dirent* entry; + while ((entry = readdir(directory)) != NULL) { + fileCount++; + // printf("File: %s\n", entry->d_name); + } + + closedir(directory); + return fileCount; +} + +TEST_F(MultifileBlobCacheTest, EnsureFileDescriptorsClosed) { + // Populate the cache with a bunch of entries + size_t kLargeNumberOfEntries = 1024; + for (int i = 0; i < kLargeNumberOfEntries; i++) { + // printf("Caching: %i", i); + + // Use the index as the key and value + mMBC->set(&i, sizeof(i), &i, sizeof(i)); + + int result = 0; + ASSERT_EQ(sizeof(i), mMBC->get(&i, sizeof(i), &result, sizeof(result))); + ASSERT_EQ(i, result); + } + + // Ensure we don't have a bunch of open fds + ASSERT_LT(getFileDescriptorCount(), kLargeNumberOfEntries / 2); + + // Close the cache so everything writes out + mMBC->finish(); + mMBC.reset(); + + // Now open it again and ensure we still don't have a bunch of open fds + mMBC.reset( + new MultifileBlobCache(kMaxKeySize, kMaxValueSize, kMaxTotalSize, &mTempFile->path[0])); + + // Check after initialization + ASSERT_LT(getFileDescriptorCount(), kLargeNumberOfEntries / 2); + + for (int i = 0; i < kLargeNumberOfEntries; i++) { + int result = 0; + ASSERT_EQ(sizeof(i), mMBC->get(&i, sizeof(i), &result, sizeof(result))); + ASSERT_EQ(i, result); + } + + // And again after we've actually used it + ASSERT_LT(getFileDescriptorCount(), kLargeNumberOfEntries / 2); +} + } // namespace android -- cgit v1.2.3-59-g8ed1b