From 002f02ea496b1d61af6b063e76ccf374beb37427 Mon Sep 17 00:00:00 2001 From: Sandeep Patil Date: Sun, 13 Jan 2019 19:39:46 -0800 Subject: [PATCH 1/4] meminfo: Fix ProcMemInfo ForEachVmaFromFile Caused by passing invalid parameters to getline(3) and the test failure went unnoticed. Bug: 111694435 Test: libmeminfo_test 1 --gtest_filter=TestProcMemInfo.ForEachVmaFromFileTest Change-Id: Ideb39604c58f89237b05d2f7c8edb67c5ae65768 Merged-In: Ideb39604c58f89237b05d2f7c8edb67c5ae65768 Signed-off-by: Sandeep Patil --- libmeminfo/libmeminfo_test.cpp | 5 +++++ libmeminfo/procmeminfo.cpp | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/libmeminfo/libmeminfo_test.cpp b/libmeminfo/libmeminfo_test.cpp index 20ed3bf41..2529bd76c 100644 --- a/libmeminfo/libmeminfo_test.cpp +++ b/libmeminfo/libmeminfo_test.cpp @@ -374,6 +374,9 @@ TEST(TestProcMemInfo, ForEachVmaFromFileTest) { auto collect_vmas = [&](const Vma& v) { vmas.push_back(v); }; ASSERT_TRUE(ForEachVmaFromFile(path, collect_vmas)); + // We should get a total of 6 vmas + ASSERT_EQ(vmas.size(), 6); + // Expect values to be equal to what we have in testdata1/smaps_short // Check for sizes first ASSERT_EQ(vmas[0].usage.vss, 32768); @@ -468,6 +471,8 @@ TEST(TestProcMemInfo, SmapsTest) { auto vmas = proc_mem.Smaps(path); ASSERT_FALSE(vmas.empty()); + // We should get a total of 6 vmas + ASSERT_EQ(vmas.size(), 6); // Expect values to be equal to what we have in testdata1/smaps_short // Check for sizes first diff --git a/libmeminfo/procmeminfo.cpp b/libmeminfo/procmeminfo.cpp index 1f8db1a04..18ae9784a 100644 --- a/libmeminfo/procmeminfo.cpp +++ b/libmeminfo/procmeminfo.cpp @@ -338,8 +338,9 @@ bool ForEachVmaFromFile(const std::string& path, const VmaCallback& callback) { char* line = nullptr; bool parsing_vma = false; ssize_t line_len; + size_t line_alloc = 0; Vma vma; - while ((line_len = getline(&line, 0, fp.get())) > 0) { + while ((line_len = getline(&line, &line_alloc, fp.get())) > 0) { // Make sure the line buffer terminates like a C string for ReadMapFile line[line_len] = '\0'; From 56c414e872ccafe2c40cd6969b08262ee96dd873 Mon Sep 17 00:00:00 2001 From: Sandeep Patil Date: Sat, 12 Jan 2019 21:29:19 -0800 Subject: [PATCH 2/4] meminfo: Remove unnecessary working set stats The Vma and ProcMemInfo objects do not need separate stats objects for storing working set. The Vma either has working set or memory usage information and never both. Bug: 111694435 Test: libmeminfo_test 1 Change-Id: I2df05f7e750bbba4325474633e705d6d68dd2ccb Merged-In: I2df05f7e750bbba4325474633e705d6d68dd2ccb Signed-off-by: Sandeep Patil --- libmeminfo/include/meminfo/meminfo.h | 7 +--- libmeminfo/include/meminfo/procmeminfo.h | 1 - libmeminfo/procmeminfo.cpp | 43 ++++++++---------------- libmeminfo/tools/procmem.cpp | 12 +++---- libmeminfo/tools/procrank.cpp | 2 +- 5 files changed, 22 insertions(+), 43 deletions(-) diff --git a/libmeminfo/include/meminfo/meminfo.h b/libmeminfo/include/meminfo/meminfo.h index 5ee32d45f..2fc78670a 100644 --- a/libmeminfo/include/meminfo/meminfo.h +++ b/libmeminfo/include/meminfo/meminfo.h @@ -72,15 +72,10 @@ struct Vma { : start(s), end(e), offset(off), flags(f), name(n) {} ~Vma() = default; - void clear() { - memset(&usage, 0, sizeof(usage)); - memset(&wss, 0, sizeof(wss)); - } + void clear() { memset(&usage, 0, sizeof(usage)); } // Memory usage of this mapping. MemUsage usage; - // Working set within this mapping. - MemUsage wss; }; } // namespace meminfo diff --git a/libmeminfo/include/meminfo/procmeminfo.h b/libmeminfo/include/meminfo/procmeminfo.h index 0bfd80f2f..c5f8c3c11 100644 --- a/libmeminfo/include/meminfo/procmeminfo.h +++ b/libmeminfo/include/meminfo/procmeminfo.h @@ -82,7 +82,6 @@ class ProcMemInfo final { std::vector maps_; MemUsage usage_; - MemUsage wss_; std::vector swap_offsets_; }; diff --git a/libmeminfo/procmeminfo.cpp b/libmeminfo/procmeminfo.cpp index 18ae9784a..9345bda90 100644 --- a/libmeminfo/procmeminfo.cpp +++ b/libmeminfo/procmeminfo.cpp @@ -157,14 +157,14 @@ const MemUsage& ProcMemInfo::Wss() { if (!get_wss_) { LOG(WARNING) << "Trying to read process working set for " << pid_ << " using invalid object"; - return wss_; + return usage_; } if (maps_.empty() && !ReadMaps(get_wss_)) { LOG(ERROR) << "Failed to get working set for Process " << pid_; } - return wss_; + return usage_; } bool ProcMemInfo::ForEachVma(const VmaCallback& callback) { @@ -228,11 +228,7 @@ bool ProcMemInfo::ReadMaps(bool get_wss) { maps_.clear(); return false; } - if (get_wss) { - add_mem_usage(&wss_, vma.wss); - } else { - add_mem_usage(&usage_, vma.usage); - } + add_mem_usage(&usage_, vma.usage); } return true; @@ -300,31 +296,20 @@ bool ProcMemInfo::ReadVmaStats(int pagemap_fd, Vma& vma, bool get_wss) { // This effectively makes vss = rss for the working set is requested. // The libpagemap implementation returns vss > rss for // working set, which doesn't make sense. - vma.wss.vss += pagesz; - vma.wss.rss += pagesz; - vma.wss.uss += is_private ? pagesz : 0; - vma.wss.pss += pagesz / pg_counts[i]; - if (is_private) { - vma.wss.private_dirty += is_dirty ? pagesz : 0; - vma.wss.private_clean += is_dirty ? 0 : pagesz; - } else { - vma.wss.shared_dirty += is_dirty ? pagesz : 0; - vma.wss.shared_clean += is_dirty ? 0 : pagesz; - } + vma.usage.vss += pagesz; + } + + vma.usage.rss += pagesz; + vma.usage.uss += is_private ? pagesz : 0; + vma.usage.pss += pagesz / pg_counts[i]; + if (is_private) { + vma.usage.private_dirty += is_dirty ? pagesz : 0; + vma.usage.private_clean += is_dirty ? 0 : pagesz; } else { - vma.usage.rss += pagesz; - vma.usage.uss += is_private ? pagesz : 0; - vma.usage.pss += pagesz / pg_counts[i]; - if (is_private) { - vma.usage.private_dirty += is_dirty ? pagesz : 0; - vma.usage.private_clean += is_dirty ? 0 : pagesz; - } else { - vma.usage.shared_dirty += is_dirty ? pagesz : 0; - vma.usage.shared_clean += is_dirty ? 0 : pagesz; - } + vma.usage.shared_dirty += is_dirty ? pagesz : 0; + vma.usage.shared_clean += is_dirty ? 0 : pagesz; } } - return true; } diff --git a/libmeminfo/tools/procmem.cpp b/libmeminfo/tools/procmem.cpp index b9b174db8..47881ed5b 100644 --- a/libmeminfo/tools/procmem.cpp +++ b/libmeminfo/tools/procmem.cpp @@ -98,7 +98,7 @@ static int show(const MemUsage& proc_stats, const std::vector& maps) { std::stringstream ss; print_header(ss); for (auto& vma : maps) { - const MemUsage& vma_stats = show_wss ? vma.wss : vma.usage; + const MemUsage& vma_stats = vma.usage; if (hide_zeroes && vma_stats.rss == 0) { continue; } @@ -116,14 +116,14 @@ static int show(const MemUsage& proc_stats, const std::vector& maps) { int main(int argc, char* argv[]) { int opt; auto pss_sort = [](const Vma& a, const Vma& b) { - uint64_t pss_a = show_wss ? a.wss.pss : a.usage.pss; - uint64_t pss_b = show_wss ? b.wss.pss : b.usage.pss; + uint64_t pss_a = a.usage.pss; + uint64_t pss_b = b.usage.pss; return pss_a > pss_b; }; auto uss_sort = [](const Vma& a, const Vma& b) { - uint64_t uss_a = show_wss ? a.wss.uss : a.usage.uss; - uint64_t uss_b = show_wss ? b.wss.uss : b.usage.uss; + uint64_t uss_a = a.usage.uss; + uint64_t uss_b = b.usage.uss; return uss_a > uss_b; }; @@ -182,7 +182,7 @@ int main(int argc, char* argv[]) { } ProcMemInfo proc(pid, show_wss); - const MemUsage& proc_stats = show_wss ? proc.Wss() : proc.Usage(); + const MemUsage& proc_stats = proc.Usage(); std::vector maps(proc.Maps()); if (sort_func != nullptr) { std::sort(maps.begin(), maps.end(), sort_func); diff --git a/libmeminfo/tools/procrank.cpp b/libmeminfo/tools/procrank.cpp index a75172220..21a684c5c 100644 --- a/libmeminfo/tools/procrank.cpp +++ b/libmeminfo/tools/procrank.cpp @@ -465,7 +465,7 @@ int main(int argc, char* argv[]) { } // Skip processes with no memory mappings - uint64_t vss = show_wss ? proc.Wss().vss : proc.Usage().vss; + uint64_t vss = proc.Usage().vss; if (vss == 0) return true; // collect swap_offset counts from all processes in 1st pass From 8871e7e90f393bce69608ea19112aa15e4914166 Mon Sep 17 00:00:00 2001 From: Sandeep Patil Date: Sun, 13 Jan 2019 16:47:20 -0800 Subject: [PATCH 3/4] meminfo: Add SmapsOrRollupPss Adds SmapsOrRollup parsing methods to only read Pss of the process fomr /proc//{smaps, smaps_rollup}. Bug: 111694435 Test: libmeminfo_test 1 --gtest_filter=TestProcMemInfo.* Change-Id: I31b982ae5ff2bb5b165ea33f6c57755ee34cbbc7 Merged-In: I31b982ae5ff2bb5b165ea33f6c57755ee34cbbc7 Signed-off-by: Sandeep Patil --- libmeminfo/include/meminfo/procmeminfo.h | 10 ++++++ libmeminfo/libmeminfo_test.cpp | 44 ++++++++++++++++++++++++ libmeminfo/procmeminfo.cpp | 23 +++++++++++++ 3 files changed, 77 insertions(+) diff --git a/libmeminfo/include/meminfo/procmeminfo.h b/libmeminfo/include/meminfo/procmeminfo.h index c5f8c3c11..4cce133ae 100644 --- a/libmeminfo/include/meminfo/procmeminfo.h +++ b/libmeminfo/include/meminfo/procmeminfo.h @@ -66,6 +66,11 @@ class ProcMemInfo final { // All other fields of MemUsage are zeroed. bool SmapsOrRollup(bool use_rollup, MemUsage* stats) const; + // Used to parse either of /proc//{smaps, smaps_rollup} and record the process's + // Pss. The 'use_rollup' parameter decides which file is to be tried. + // Returns 'true' on success and the value of Pss in the out parameter. + bool SmapsOrRollupPss(bool use_rollup, uint64_t* pss) const; + const std::vector& SwapOffsets(); ~ProcMemInfo() = default; @@ -94,5 +99,10 @@ bool ForEachVmaFromFile(const std::string& path, const VmaCallback& callback); // or /proc//smaps_rollup bool SmapsOrRollupFromFile(const std::string& path, MemUsage* stats); +// Same as ProcMemInfo::SmapsOrRollupPss but reads the statistics directly +// from a file and returns total Pss in kB. The file MUST be in the same format +// as /proc//smaps or /proc//smaps_rollup +bool SmapsOrRollupPssFromFile(const std::string& path, uint64_t* pss); + } // namespace meminfo } // namespace android diff --git a/libmeminfo/libmeminfo_test.cpp b/libmeminfo/libmeminfo_test.cpp index 2529bd76c..796a7d0fe 100644 --- a/libmeminfo/libmeminfo_test.cpp +++ b/libmeminfo/libmeminfo_test.cpp @@ -365,6 +365,50 @@ VmFlags: rd wr mr mw me ac EXPECT_EQ(stats.swap_pss, 70); } +TEST(TestProcMemInfo, SmapsOrRollupPssRollupTest) { + // This is a made up smaps for the test + std::string smaps = + R"smaps(12c00000-13440000 rw-p 00000000 00:00 0 [anon:dalvik-main space (region space)] +Name: [anon:dalvik-main space (region space)] +Size: 8448 kB +KernelPageSize: 4 kB +MMUPageSize: 4 kB +Rss: 2652 kB +Pss: 2652 kB +Shared_Clean: 840 kB +Shared_Dirty: 40 kB +Private_Clean: 84 kB +Private_Dirty: 2652 kB +Referenced: 2652 kB +Anonymous: 2652 kB +AnonHugePages: 0 kB +ShmemPmdMapped: 0 kB +Shared_Hugetlb: 0 kB +Private_Hugetlb: 0 kB +Swap: 102 kB +SwapPss: 70 kB +Locked: 2652 kB +VmFlags: rd wr mr mw me ac +)smaps"; + + TemporaryFile tf; + ASSERT_TRUE(tf.fd != -1); + ASSERT_TRUE(::android::base::WriteStringToFd(smaps, tf.fd)); + + uint64_t pss; + ASSERT_EQ(SmapsOrRollupPssFromFile(tf.path, &pss), true); + EXPECT_EQ(pss, 2652); +} + +TEST(TestProcMemInfo, SmapsOrRollupPssSmapsTest) { + std::string exec_dir = ::android::base::GetExecutableDirectory(); + std::string path = ::android::base::StringPrintf("%s/testdata1/smaps_short", exec_dir.c_str()); + + uint64_t pss; + ASSERT_EQ(SmapsOrRollupPssFromFile(path, &pss), true); + EXPECT_EQ(pss, 19119); +} + TEST(TestProcMemInfo, ForEachVmaFromFileTest) { std::string exec_dir = ::android::base::GetExecutableDirectory(); std::string path = ::android::base::StringPrintf("%s/testdata1/smaps_short", exec_dir.c_str()); diff --git a/libmeminfo/procmeminfo.cpp b/libmeminfo/procmeminfo.cpp index 9345bda90..f72d46964 100644 --- a/libmeminfo/procmeminfo.cpp +++ b/libmeminfo/procmeminfo.cpp @@ -178,6 +178,12 @@ bool ProcMemInfo::SmapsOrRollup(bool use_rollup, MemUsage* stats) const { return SmapsOrRollupFromFile(path, stats); }; +bool ProcMemInfo::SmapsOrRollupPss(bool use_rollup, uint64_t* pss) const { + std::string path = ::android::base::StringPrintf("/proc/%d/%s", pid_, + use_rollup ? "smaps_rollup" : "smaps"); + return SmapsOrRollupPssFromFile(path, pss); +} + const std::vector& ProcMemInfo::SwapOffsets() { if (get_wss_) { LOG(WARNING) << "Trying to read process swap offsets for " << pid_ @@ -412,5 +418,22 @@ bool SmapsOrRollupFromFile(const std::string& path, MemUsage* stats) { return true; } +bool SmapsOrRollupPssFromFile(const std::string& path, uint64_t* pss) { + auto fp = std::unique_ptr{fopen(path.c_str(), "re"), fclose}; + if (fp == nullptr) { + return false; + } + *pss = 0; + char line[1024]; + while (fgets(line, sizeof(line), fp.get()) != nullptr) { + uint64_t v; + if (sscanf(line, "Pss: %" SCNu64 " kB", &v) == 1) { + *pss += v; + } + } + + return true; +} + } // namespace meminfo } // namespace android From dfd34be42b559a7caf607b9dca771432cf28491e Mon Sep 17 00:00:00 2001 From: Sandeep Patil Date: Sun, 13 Jan 2019 17:39:08 -0800 Subject: [PATCH 4/4] meminfo: Add IsSmapsRollupSupported Api Consolidate the checking of /proc//smaps_rollup support in libmeminfo and do it in a thread safe way. Use the API in ProcMemInfo as well to eliminate the extra parameters passed to SmapsOrRollup* methods. Bug: 111694435 Test: libmeminfo_test 1 --gtest_filter=TestProcMemInfo.IsSmapsSupportedTest Test: Tested with and without the smaps_rollup support in kernel. Change-Id: I992057f06b54569025fa0cdade9618da2675d1de Merged-In: I992057f06b54569025fa0cdade9618da2675d1de Signed-off-by: Sandeep Patil --- libmeminfo/include/meminfo/procmeminfo.h | 12 +++++-- libmeminfo/libmeminfo_test.cpp | 11 ++++++- libmeminfo/procmeminfo.cpp | 40 +++++++++++++++++++----- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/libmeminfo/include/meminfo/procmeminfo.h b/libmeminfo/include/meminfo/procmeminfo.h index 4cce133ae..0b66074aa 100644 --- a/libmeminfo/include/meminfo/procmeminfo.h +++ b/libmeminfo/include/meminfo/procmeminfo.h @@ -64,12 +64,12 @@ class ProcMemInfo final { // private_dirty // SwapPss // All other fields of MemUsage are zeroed. - bool SmapsOrRollup(bool use_rollup, MemUsage* stats) const; + bool SmapsOrRollup(MemUsage* stats) const; // Used to parse either of /proc//{smaps, smaps_rollup} and record the process's - // Pss. The 'use_rollup' parameter decides which file is to be tried. + // Pss. // Returns 'true' on success and the value of Pss in the out parameter. - bool SmapsOrRollupPss(bool use_rollup, uint64_t* pss) const; + bool SmapsOrRollupPss(uint64_t* pss) const; const std::vector& SwapOffsets(); @@ -94,6 +94,12 @@ class ProcMemInfo final { // same format as /proc//smaps. Returns 'false' if the file is malformed. bool ForEachVmaFromFile(const std::string& path, const VmaCallback& callback); +// Returns if the kernel supports /proc//smaps_rollup. Assumes that the +// calling process has access to the /proc//smaps_rollup. +// Returns 'false' if the calling process has no permission to read the file if it exists +// of if the file doesn't exist. +bool IsSmapsRollupSupported(pid_t pid); + // Same as ProcMemInfo::SmapsOrRollup but reads the statistics directly // from a file. The file MUST be in the same format as /proc//smaps // or /proc//smaps_rollup diff --git a/libmeminfo/libmeminfo_test.cpp b/libmeminfo/libmeminfo_test.cpp index 796a7d0fe..e689a2679 100644 --- a/libmeminfo/libmeminfo_test.cpp +++ b/libmeminfo/libmeminfo_test.cpp @@ -284,13 +284,22 @@ TEST(TestProcMemInfo, SwapOffsetsEmpty) { EXPECT_EQ(swap_offsets.size(), 0); } +TEST(TestProcMemInfo, IsSmapsSupportedTest) { + std::string path = ::android::base::StringPrintf("/proc/%d/smaps_rollup", pid); + bool supported = IsSmapsRollupSupported(pid); + EXPECT_EQ(!access(path.c_str(), F_OK | R_OK), supported); + // Second call must return what the first one returned regardless of the pid parameter. + // So, deliberately pass invalid pid. + EXPECT_EQ(supported, IsSmapsRollupSupported(-1)); +} + TEST(TestProcMemInfo, SmapsOrRollupReturn) { // if /proc//smaps_rollup file exists, .SmapsRollup() must return true; // false otherwise std::string path = ::android::base::StringPrintf("/proc/%d/smaps_rollup", pid); ProcMemInfo proc_mem(pid); MemUsage stats; - EXPECT_EQ(!access(path.c_str(), F_OK), proc_mem.SmapsOrRollup(true, &stats)); + EXPECT_EQ(!access(path.c_str(), F_OK), proc_mem.SmapsOrRollup(&stats)); } TEST(TestProcMemInfo, SmapsOrRollupTest) { diff --git a/libmeminfo/procmeminfo.cpp b/libmeminfo/procmeminfo.cpp index f72d46964..347a293a9 100644 --- a/libmeminfo/procmeminfo.cpp +++ b/libmeminfo/procmeminfo.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -172,15 +173,15 @@ bool ProcMemInfo::ForEachVma(const VmaCallback& callback) { return ForEachVmaFromFile(path, callback); } -bool ProcMemInfo::SmapsOrRollup(bool use_rollup, MemUsage* stats) const { - std::string path = ::android::base::StringPrintf("/proc/%d/%s", pid_, - use_rollup ? "smaps_rollup" : "smaps"); +bool ProcMemInfo::SmapsOrRollup(MemUsage* stats) const { + std::string path = ::android::base::StringPrintf( + "/proc/%d/%s", pid_, IsSmapsRollupSupported(pid_) ? "smaps_rollup" : "smaps"); return SmapsOrRollupFromFile(path, stats); -}; +} -bool ProcMemInfo::SmapsOrRollupPss(bool use_rollup, uint64_t* pss) const { - std::string path = ::android::base::StringPrintf("/proc/%d/%s", pid_, - use_rollup ? "smaps_rollup" : "smaps"); +bool ProcMemInfo::SmapsOrRollupPss(uint64_t* pss) const { + std::string path = ::android::base::StringPrintf( + "/proc/%d/%s", pid_, IsSmapsRollupSupported(pid_) ? "smaps_rollup" : "smaps"); return SmapsOrRollupPssFromFile(path, pss); } @@ -374,6 +375,31 @@ bool ForEachVmaFromFile(const std::string& path, const VmaCallback& callback) { return true; } +enum smaps_rollup_support { UNTRIED, SUPPORTED, UNSUPPORTED }; + +static std::atomic g_rollup_support = UNTRIED; + +bool IsSmapsRollupSupported(pid_t pid) { + // Similar to OpenSmapsOrRollup checks from android_os_Debug.cpp, except + // the method only checks if rollup is supported and returns the status + // right away. + enum smaps_rollup_support rollup_support = g_rollup_support.load(std::memory_order_relaxed); + if (rollup_support != UNTRIED) { + return rollup_support == SUPPORTED; + } + std::string rollup_file = ::android::base::StringPrintf("/proc/%d/smaps_rollup", pid); + if (access(rollup_file.c_str(), F_OK | R_OK)) { + // No check for errno = ENOENT necessary here. The caller MUST fallback to + // using /proc//smaps instead anyway. + g_rollup_support.store(UNSUPPORTED, std::memory_order_relaxed); + return false; + } + + g_rollup_support.store(SUPPORTED, std::memory_order_relaxed); + LOG(INFO) << "Using smaps_rollup for pss collection"; + return true; +} + bool SmapsOrRollupFromFile(const std::string& path, MemUsage* stats) { auto fp = std::unique_ptr{fopen(path.c_str(), "re"), fclose}; if (fp == nullptr) {