You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by "DevinLeamy (via GitHub)" <gi...@apache.org> on 2024/04/15 13:59:14 UTC
[PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
DevinLeamy opened a new pull request, #554:
URL: https://github.com/apache/mesos/pull/554
Introduces
```
cgroups2::memory::min(cgroup) // get the minimum
cgroups2::memory::set_min(cgroup, bytes) // set the minimum
```
to get and set the minimum memory in bytes that are guaranteed to not be reclaimed by the kernel under any conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1565850696
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
+// we only test with multiples of the page size.
Review Comment:
src: https://docs.kernel.org/admin-guide/cgroup-v2.html#memory-interface-files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler closed pull request #554: [cgroups2] Introduced API to set the hard memory minimum for a cgroup.
URL: https://github.com/apache/mesos/pull/554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1566127015
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
+// we only test with multiples of the page size.
+const long PAGE_SIZE = sysconf(_SC_PAGE_SIZE);
Review Comment:
no need to store a constant for this, and let's use os/pagesize.hpp inline for this:
```
Bytes(os::pagesize()) * 5
```
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -325,6 +334,24 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryUsage)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryMinimum)
+{
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ const Bytes bytes = Bytes(5 * PAGE_SIZE);
Review Comment:
see above comment (multiplication can be moved outside the bytes here
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
Review Comment:
might be good to have the test just show what happens directly here:
1. Test a value that is not page size aligned (gets rounded down despite the docs saying rounded up)
2. Test a page size aligned value, comes back the same
##########
src/linux/cgroups2.hpp:
##########
@@ -220,6 +220,19 @@ namespace memory {
// Current memory usage of a cgroup and its descendants in bytes.
Try<Bytes> usage(const std::string& cgroup);
+
+// Set the minimum memory that is guaranteed to not be reclaimed under any
+// conditions. If the value is larger than the parent cgroup's value, the
+// parent's value is the effective value.
Review Comment:
you mentioned that this depends on a configuration or something? would be good to spell out this subtlety
could also be more clear about what effective value means here (i.e. effective seems to suggest that you can actually set it higher but the parent's will effectively limit it before that, as opposed to the value you set getting rounded down to the parent's value)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1566284559
##########
src/linux/cgroups2.cpp:
##########
@@ -786,6 +787,23 @@ Try<Bytes> usage(const string& cgroup)
return Bytes(*contents);
}
+
+Try<Nothing> set_min(const string& cgroup, const Bytes& bytes)
+{
+ return cgroups2::write(cgroup, control::MIN, stringify(bytes.bytes()));
Review Comment:
we already have a write that takes uint64_t?
##########
src/linux/cgroups2.cpp:
##########
@@ -786,6 +787,23 @@ Try<Bytes> usage(const string& cgroup)
return Bytes(*contents);
}
+
+Try<Nothing> set_min(const string& cgroup, const Bytes& bytes)
+{
+ return cgroups2::write(cgroup, control::MIN, stringify(bytes.bytes()));
+}
+
+
+Try<Bytes> min(const string& cgroup)
+{
+ Try<string> contents = cgroups2::read<string>(cgroup, control::MIN);
+ if (contents.isError()) {
+ return Error("Failed to read 'memory.min': " + contents.error());
+ }
+
+ return Bytes::parse(strings::trim(*contents) + "B");
Review Comment:
no need to parse with a B here, can just cgroups::read a uint64_t and directly construct Bytes with it?
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -325,6 +326,52 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryUsage)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryMinimum)
+{
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ const Bytes bytes = Bytes(os::pagesize()) * 5;
+
+ // Does not exist for the root cgroup.
+ EXPECT_ERROR(cgroups2::memory::min(cgroups2::ROOT_CGROUP));
+ EXPECT_ERROR(cgroups2::memory::set_min(cgroups2::ROOT_CGROUP, bytes));
+
+ EXPECT_SOME(cgroups2::memory::set_min(TEST_CGROUP, bytes));
+ EXPECT_SOME_EQ(bytes, cgroups2::memory::min(TEST_CGROUP));
+}
+
+
+// Check that byte amounts written to the memory controller are rounded
+// down to the nearest page size.
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryBytesRounding)
Review Comment:
is the plan to add other memory setters in here, hence not calling this a Minimum test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org