Skip to content

GitLab

T
tangled
Commit 9c40768c authored by Kuo-Hsin Yang's avatar Kuo-Hsin Yang Committed by Commit Bot
Browse files
Options

Remove feature option CrOSUserSpaceLowMemoryNotification 

And remove these dead code path, these dead code paths make it harder to
modify SystemMemoryPressureEvaluator.

BUG=b:149833548
TEST=run chromeos_unittests

Change-Id: I594009d05b39be43332708fd973e1a02031879a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2421189Reviewed-by: default avatarBrian Geffon <bgeffon@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Commit-Queue: Kuo-Hsin Yang <vovoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811801}
parent da99e30e
Hide whitespace changes
Inline Side-by-side
Showing with 33 additions and 217 deletions
chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc
View file @ 9c40768c
......@@ -36,6 +36,7 @@
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/memory/pressure/pressure.h"
#include "chromeos/memory/pressure/system_memory_pressure_evaluator.h"
#include "components/arc/arc_service_manager.h"
#include "components/arc/arc_util.h"
......@@ -234,14 +235,14 @@ int TabManagerDelegate::MemoryStat::LowMemoryMarginKB() {
// memory back to the margin.
int TabManagerDelegate::MemoryStat::TargetMemoryToFreeKB() {
uint64_t available_mem_mb;
auto* monitor = chromeos::memory::SystemMemoryPressureEvaluator::Get();
if (monitor) {
available_mem_mb = monitor->GetAvailableMemoryKB();
if (chromeos::memory::SystemMemoryPressureEvaluator::Get()) {
available_mem_mb = chromeos::memory::pressure::GetAvailableMemoryKB();
} else {
// When TabManager::DiscardTab(LifecycleUnitDiscardReason::EXTERNAL) is
// called by a test or an extension, TabManagerDelegate might be used
// without chromeos SystemMemoryPressureEvaluator, e.g. the browser test
// DiscardTabsWithMinimizedWindow.
// DiscardTabsWithMinimizedWindow. Set available to 0 to force discarding a
// tab to pass the test.
LOG(WARNING) << "SystemMemoryPressureEvaluator is not available";
available_mem_mb = 0;
}
......
chromeos/memory/pressure/system_memory_pressure_evaluator.cc
View file @ 9c40768c
......@@ -30,9 +30,6 @@
namespace chromeos {
namespace memory {
const base::Feature kCrOSUserSpaceLowMemoryNotification{
"CrOSUserSpaceLowMemoryNotification", base::FEATURE_ENABLED_BY_DEFAULT};
namespace {
// Pointer to the SystemMemoryPressureEvaluator used by TabManagerDelegate for
// chromeos to need to call into ScheduleEarlyCheck.
......@@ -49,11 +46,6 @@ constexpr base::TimeDelta kModerateMemoryPressureCooldownTime =
// memory pressure notifications in chromeos.
constexpr char kMarginMemFile[] = "/sys/kernel/mm/chromeos-low_mem/margin";
// The available memory file contains the available memory as determined
// by the kernel.
constexpr char kAvailableMemFile[] =
"/sys/kernel/mm/chromeos-low_mem/available";
// Converts an available memory value in MB to a memory pressure level.
base::MemoryPressureListener::MemoryPressureLevel
GetMemoryPressureLevelFromAvailable(int available_mb,
......@@ -67,76 +59,20 @@ GetMemoryPressureLevelFromAvailable(int available_mb,
return base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE;
}
uint64_t ReadAvailableMemoryMB(int available_fd) {
// Read the available memory.
char buf[32] = {};
// kernfs/file.c:
// "Once poll/select indicates that the value has changed, you
// need to close and re-open the file, or seek to 0 and read again.
ssize_t bytes_read = HANDLE_EINTR(pread(available_fd, buf, sizeof(buf), 0));
PCHECK(bytes_read != -1);
std::string mem_str(buf, bytes_read);
uint64_t available = std::numeric_limits<uint64_t>::max();
CHECK(base::StringToUint64(
base::TrimWhitespaceASCII(mem_str, base::TrimPositions::TRIM_ALL),
&available));
return available;
}
// This function will wait until the /sys/kernel/mm/chromeos-low_mem/available
// file becomes readable and then read the latest value. This file will only
// become readable once the available memory cross through one of the margin
// values specified in /sys/kernel/mm/chromeos-low_mem/margin, for more
// details see https://crrev.com/c/536336.
bool WaitForMemoryPressureChanges(int available_fd) {
base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
base::BlockingType::WILL_BLOCK);
pollfd pfd = {available_fd, POLLPRI | POLLERR, 0};
int res = HANDLE_EINTR(poll(&pfd, 1, -1)); // Wait indefinitely.
PCHECK(res != -1);
if (pfd.revents != (POLLPRI | POLLERR)) {
// If we didn't receive POLLPRI | POLLERR it means we likely received
// POLLNVAL because the fd has been closed we will only log an error in
// other situations.
LOG_IF(ERROR, pfd.revents != POLLNVAL)
<< "WaitForMemoryPressureChanges received unexpected revents: "
<< pfd.revents;
// We no longer want to wait for a kernel notification if the fd has been
// closed.
return false;
}
return true;
}
} // namespace
SystemMemoryPressureEvaluator::SystemMemoryPressureEvaluator(
std::unique_ptr<util::MemoryPressureVoter> voter)
: SystemMemoryPressureEvaluator(
kMarginMemFile,
kAvailableMemFile,
base::BindRepeating(&WaitForMemoryPressureChanges),
/*disable_timer_for_testing*/ false,
base::FeatureList::IsEnabled(
chromeos::memory::kCrOSUserSpaceLowMemoryNotification),
std::move(voter)) {}
SystemMemoryPressureEvaluator::SystemMemoryPressureEvaluator(
const std::string& margin_file,
const std::string& available_file,
base::RepeatingCallback<bool(int)> kernel_waiting_callback,
bool disable_timer_for_testing,
bool is_user_space_notify,
std::unique_ptr<util::MemoryPressureVoter> voter)
: util::SystemMemoryPressureEvaluator(std::move(voter)),
is_user_space_notify_(is_user_space_notify),
weak_ptr_factory_(this) {
DCHECK(g_system_evaluator == nullptr);
g_system_evaluator = this;
......@@ -151,19 +87,7 @@ SystemMemoryPressureEvaluator::SystemMemoryPressureEvaluator(
critical_pressure_threshold_mb_ = margin_parts[0];
moderate_pressure_threshold_mb_ = margin_parts[1];
if (is_user_space_notify_) {
chromeos::memory::pressure::UpdateMemoryParameters();
}
if (!is_user_space_notify_) {
kernel_waiting_callback_ = base::BindRepeating(
std::move(kernel_waiting_callback), available_mem_file_.get());
available_mem_file_ =
base::ScopedFD(HANDLE_EINTR(open(available_file.c_str(), O_RDONLY)));
CHECK(available_mem_file_.is_valid());
ScheduleWaitForKernelNotification();
}
chromeos::memory::pressure::UpdateMemoryParameters();
if (!disable_timer_for_testing) {
// We will check the memory pressure and report the metric
......@@ -217,16 +141,6 @@ std::vector<int> SystemMemoryPressureEvaluator::GetMarginFileParts(
return margin_values;
}
uint64_t SystemMemoryPressureEvaluator::GetAvailableMemoryKB() {
if (is_user_space_notify_) {
return chromeos::memory::pressure::GetAvailableMemoryKB();
} else {
const uint64_t available_mem_mb =
ReadAvailableMemoryMB(available_mem_file_.get());
return available_mem_mb * 1024;
}
}
bool SystemMemoryPressureEvaluator::SupportsKernelNotifications() {
// Unfortunately at the moment the only way to determine if the chromeos
// kernel supports polling on the available file is to observe two values
......@@ -235,15 +149,20 @@ bool SystemMemoryPressureEvaluator::SupportsKernelNotifications() {
return SystemMemoryPressureEvaluator::GetMarginFileParts().size() >= 2;
}
// CheckMemoryPressure will get the current memory pressure level by reading
// the available file.
// CheckMemoryPressure will get the current memory pressure level by checking
// the available memory.
void SystemMemoryPressureEvaluator::CheckMemoryPressure() {
uint64_t mem_avail_mb =
chromeos::memory::pressure::GetAvailableMemoryKB() / 1024;
CheckMemoryPressureImpl(mem_avail_mb);
}
void SystemMemoryPressureEvaluator::CheckMemoryPressureImpl(
uint64_t mem_avail_mb) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
auto old_vote = current_vote();
uint64_t mem_avail_mb = GetAvailableMemoryKB() / 1024;
SetCurrentVote(GetMemoryPressureLevelFromAvailable(
mem_avail_mb, moderate_pressure_threshold_mb_,
critical_pressure_threshold_mb_));
......@@ -279,22 +198,6 @@ void SystemMemoryPressureEvaluator::CheckMemoryPressure() {
SendCurrentVote(notify);
}
void SystemMemoryPressureEvaluator::HandleKernelNotification(bool result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// If WaitForKernelNotification returned false then the FD has been closed and
// we just exit without waiting again.
if (!result) {
return;
}
CheckMemoryPressure();
// Now we need to schedule back our blocking task to wait for more
// kernel notifications.
ScheduleWaitForKernelNotification();
}
void SystemMemoryPressureEvaluator::CheckMemoryPressureAndRecordStatistics() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
......@@ -319,16 +222,5 @@ void SystemMemoryPressureEvaluator::ScheduleEarlyCheck() {
weak_ptr_factory_.GetWeakPtr()));
}
void SystemMemoryPressureEvaluator::ScheduleWaitForKernelNotification() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE,
{base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
base::BindOnce(kernel_waiting_callback_),
base::BindOnce(&SystemMemoryPressureEvaluator::HandleKernelNotification,
weak_ptr_factory_.GetWeakPtr()));
}
} // namespace memory
} // namespace chromeos
chromeos/memory/pressure/system_memory_pressure_evaluator.h
View file @ 9c40768c
......@@ -22,9 +22,6 @@
namespace chromeos {
namespace memory {
// A feature which controls user space low memory notification.
extern const base::Feature kCrOSUserSpaceLowMemoryNotification;
////////////////////////////////////////////////////////////////////////////////
// SystemMemoryPressureEvaluator
//
......@@ -50,9 +47,6 @@ class COMPONENT_EXPORT(CHROMEOS_MEMORY) SystemMemoryPressureEvaluator
// is moderate memory pressure level.
static std::vector<int> GetMarginFileParts();
// GetAvailableMemoryKB returns the available memory in KiB.
uint64_t GetAvailableMemoryKB();
// SupportsKernelNotifications will return true if the kernel supports and is
// configured for notifications on memory availability changes.
static bool SupportsKernelNotifications();
......@@ -79,19 +73,17 @@ class COMPONENT_EXPORT(CHROMEOS_MEMORY) SystemMemoryPressureEvaluator
// This constructor is only used for testing.
SystemMemoryPressureEvaluator(
const std::string& margin_file,
const std::string& available_file,
base::RepeatingCallback<bool(int)> kernel_waiting_callback,
bool disable_timer_for_testing,
bool is_user_space_notify,
std::unique_ptr<util::MemoryPressureVoter> voter);
static std::vector<int> GetMarginFileParts(const std::string& margin_file);
void CheckMemoryPressure();
// Split CheckMemoryPressure and CheckMemoryPressureImpl for testing.
void CheckMemoryPressureImpl(uint64_t mem_avail_mb);
private:
void HandleKernelNotification(bool result);
void ScheduleWaitForKernelNotification();
void CheckMemoryPressureAndRecordStatistics();
int moderate_pressure_threshold_mb_ = 0;
int critical_pressure_threshold_mb_ = 0;
......@@ -104,22 +96,10 @@ class COMPONENT_EXPORT(CHROMEOS_MEMORY) SystemMemoryPressureEvaluator
// Memory.PressureLevel metric.
base::TimeTicks last_pressure_level_report_;
// File descriptor used to read and poll(2) available memory from sysfs,
// In /sys/kernel/mm/chromeos-low_mem/available.
base::ScopedFD available_mem_file_;
// A timer to check the memory pressure and to report an UMA metric
// periodically.
base::RepeatingTimer checking_timer_;
// Kernel waiting callback which is responsible for blocking on the
// available file until it receives a kernel notification, this is
// configurable to make testing easier.
base::RepeatingCallback<bool()> kernel_waiting_callback_;
// User space low memory notification mode.
const bool is_user_space_notify_;
SEQUENCE_CHECKER(sequence_checker_);
base::WeakPtrFactory<SystemMemoryPressureEvaluator> weak_ptr_factory_;
......
chromeos/memory/pressure/system_memory_pressure_evaluator_unittest.cc
View file @ 9c40768c
......@@ -27,27 +27,6 @@ namespace memory {
namespace {
// Since it would be very hard to mock sysfs instead we will send in our own
// implementation of WaitForKernelNotification which instead will block on a
// pipe that we can trigger for the test to cause a mock kernel notification.
bool WaitForMockKernelNotification(int pipe_read_fd, int available_fd) {
base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
base::BlockingType::WILL_BLOCK);
// We just use a pipe to block our kernel notification thread until we have
// a fake kernel notification.
char buf = 0;
int res = HANDLE_EINTR(read(pipe_read_fd, &buf, sizeof(buf)));
// Fail if we encounter any error.
return res > 0;
}
void TriggerKernelNotification(int pipe_write_fd) {
char buf = '1';
HANDLE_EINTR(write(pipe_write_fd, &buf, sizeof(buf)));
}
// Processes OnMemoryPressure calls by just storing the sequence of events so we
// can validate that we received the expected pressure levels as the test runs.
void OnMemoryPressure(
......@@ -56,37 +35,26 @@ void OnMemoryPressure(
history->push_back(level);
}
void RunLoopRunWithTimeout(base::TimeDelta timeout) {
base::RunLoop run_loop;
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(FROM_HERE,
run_loop.QuitClosure(),
timeout);
run_loop.Run();
}
} // namespace
class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator {
public:
TestSystemMemoryPressureEvaluator(
const std::string& mock_margin_file,
const std::string& mock_available_file,
base::RepeatingCallback<bool(int)> kernel_waiting_callback,
bool disable_timer_for_testing,
bool is_user_space_notify,
std::unique_ptr<util::MemoryPressureVoter> voter)
: SystemMemoryPressureEvaluator(mock_margin_file,
mock_available_file,
std::move(kernel_waiting_callback),
disable_timer_for_testing,
is_user_space_notify,
std::move(voter)) {}
static std::vector<int> GetMarginFileParts(const std::string& file) {
return SystemMemoryPressureEvaluator::GetMarginFileParts(file);
}
void CheckMemoryPressureImpl(uint64_t mem_avail_mb) {
SystemMemoryPressureEvaluator::CheckMemoryPressureImpl(mem_avail_mb);
}
~TestSystemMemoryPressureEvaluator() override = default;
private:
......@@ -145,16 +113,11 @@ TEST(ChromeOSSystemMemoryPressureEvaluatorTest, CheckMemoryPressure) {
ASSERT_TRUE(tmp_dir.CreateUniqueTempDir());
base::FilePath margin_file = tmp_dir.GetPath().Append("margin");
base::FilePath available_file = tmp_dir.GetPath().Append("available");
// Set the margin values to 500 (critical) and 1000 (moderate).
const std::string kMarginContents = "500 1000";
ASSERT_TRUE(base::WriteFile(margin_file, kMarginContents));
// Write the initial available contents.
const std::string kInitialAvailableContents = "1500";
ASSERT_TRUE(base::WriteFile(available_file, kInitialAvailableContents));
base::test::TaskEnvironment task_environment(
base::test::TaskEnvironment::MainThreadType::UI);
......@@ -170,21 +133,8 @@ TEST(ChromeOSSystemMemoryPressureEvaluatorTest, CheckMemoryPressure) {
util::MultiSourceMemoryPressureMonitor monitor;
monitor.ResetSystemEvaluatorForTesting();
// We use a pipe to notify our blocked kernel notification thread that there
// is a kernel notification we need to use a simple blocking syscall and
// read(2)/write(2) will work.
int fds[2] = {};
ASSERT_EQ(0, HANDLE_EINTR(pipe(fds)));
// Make sure the pipe FDs get closed.
base::ScopedFD write_end(fds[1]);
base::ScopedFD read_end(fds[0]);
auto evaluator = std::make_unique<TestSystemMemoryPressureEvaluator>(
margin_file.value(), available_file.value(),
// Bind the read end to WaitForMockKernelNotification.
base::BindRepeating(&WaitForMockKernelNotification, read_end.get()),
/*disable_timer_for_testing=*/true, /*is_user_space_notify*/ false,
margin_file.value(), /*disable_timer_for_testing=*/true,
monitor.CreateVoter());
// Validate that our margin levels are as expected after being parsed from our
......@@ -197,39 +147,32 @@ TEST(ChromeOSSystemMemoryPressureEvaluatorTest, CheckMemoryPressure) {
evaluator->current_vote());
// Moderate Pressure.
ASSERT_TRUE(base::WriteFile(available_file, "900"));
TriggerKernelNotification(write_end.get());
// TODO(bgeffon): Use RunLoop::QuitClosure() instead of relying on "spin for
// 1 second".
RunLoopRunWithTimeout(base::TimeDelta::FromSeconds(1));
evaluator->CheckMemoryPressureImpl(900);
base::RunLoop().RunUntilIdle();
ASSERT_EQ(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE,
evaluator->current_vote());
// Critical Pressure.
ASSERT_TRUE(base::WriteFile(available_file, "450"));
TriggerKernelNotification(write_end.get());
RunLoopRunWithTimeout(base::TimeDelta::FromSeconds(1));
evaluator->CheckMemoryPressureImpl(450);
base::RunLoop().RunUntilIdle();
ASSERT_EQ(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL,
evaluator->current_vote());
// Moderate Pressure.
ASSERT_TRUE(base::WriteFile(available_file, "550"));
TriggerKernelNotification(write_end.get());
RunLoopRunWithTimeout(base::TimeDelta::FromSeconds(1));
evaluator->CheckMemoryPressureImpl(550);
base::RunLoop().RunUntilIdle();
ASSERT_EQ(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE,
evaluator->current_vote());
// No pressure, note: this will not cause any event.
ASSERT_TRUE(base::WriteFile(available_file, "1150"));
TriggerKernelNotification(write_end.get());
RunLoopRunWithTimeout(base::TimeDelta::FromSeconds(1));
evaluator->CheckMemoryPressureImpl(1150);
base::RunLoop().RunUntilIdle();
ASSERT_EQ(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE,
evaluator->current_vote());
// Back into moderate.
ASSERT_TRUE(base::WriteFile(available_file, "950"));
TriggerKernelNotification(write_end.get());
RunLoopRunWithTimeout(base::TimeDelta::FromSeconds(1));
evaluator->CheckMemoryPressureImpl(950);
base::RunLoop().RunUntilIdle();
ASSERT_EQ(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE,
evaluator->current_vote());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment