This patch series adds a memory.reclaim proactive reclaim interface.
The rationale behind the interface and how it works are in the first
patch.
---
Changes in V3:
- Fix cg_write() (in patch 2) to properly return -1 if open() fails
and not fail if len == errno.
- Remove debug printf() in patch 3.
Changes in V2:
- Add the interface to root as well.
- Added a selftest.
- Documented the interface as a nested-keyed interface, which makes
adding optional arguments in the future easier (see doc updates in the
first patch).
- Modified the commit message to reflect changes and add a timeout
argument as a suggested possible extension
- Return -EAGAIN if the kernel fails to reclaim the full requested
amount.
---
Shakeel Butt (1):
memcg: introduce per-memcg reclaim interface
Yosry Ahmed (3):
selftests: cgroup: return the errno of write() in cg_write() on
failure
selftests: cgroup: fix alloc_anon_noexit() instantly freeing memory
selftests: cgroup: add a selftest for memory.reclaim
Documentation/admin-guide/cgroup-v2.rst | 21 +++++
mm/memcontrol.c | 37 ++++++++
tools/testing/selftests/cgroup/cgroup_util.c | 32 ++++---
.../selftests/cgroup/test_memcontrol.c | 93 ++++++++++++++++++-
4 files changed, 166 insertions(+), 17 deletions(-)
--
2.35.1.1178.g4f1659d476-goog
Currently, cg_write() returns 0 on success and -1 on failure. Modify it
to return the errno of write() syscall when write() fails.
Signed-off-by: Yosry Ahmed <[email protected]>
---
tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++---------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
index dbaa7aabbb4a..3b6bb09985fa 100644
--- a/tools/testing/selftests/cgroup/cgroup_util.c
+++ b/tools/testing/selftests/cgroup/cgroup_util.c
@@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len)
return len;
}
-static ssize_t write_text(const char *path, char *buf, ssize_t len)
+/*
+ * Returns:
+ * success -> 0
+ * open() failure -> -1
+ * write() failure -> errno
+ */
+static int write_text(const char *path, char *buf, ssize_t len)
{
- int fd;
+ int fd, ret;
fd = open(path, O_WRONLY | O_APPEND);
if (fd < 0)
return fd;
- len = write(fd, buf, len);
- if (len < 0) {
- close(fd);
- return len;
- }
-
+ ret = write(fd, buf, len) < 0 ? errno : 0;
close(fd);
-
- return len;
+ return ret;
}
char *cg_name(const char *root, const char *name)
@@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control)
return cnt;
}
+/*
+ * Returns:
+ * success -> 0
+ * open() failure -> -1
+ * write() failure -> errno
+ */
int cg_write(const char *cgroup, const char *control, char *buf)
{
char path[PATH_MAX];
ssize_t len = strlen(buf);
snprintf(path, sizeof(path), "%s/%s", cgroup, control);
-
- if (write_text(path, buf, len) == len)
- return 0;
-
- return -1;
+ return write_text(path, buf, len);
}
int cg_find_unified_root(char *root, size_t len)
--
2.35.1.1178.g4f1659d476-goog
Add a new test for memory.reclaim that verifies that the interface
correctly reclaims memory as intended, from both anon and file pages.
Signed-off-by: Yosry Ahmed <[email protected]>
---
.../selftests/cgroup/test_memcontrol.c | 85 +++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index f2ffb3a30194..75ae441c01c3 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -760,6 +760,90 @@ static int test_memcg_max(const char *root)
return ret;
}
+/*
+ * This test checks that memory.reclaim reclaims the given
+ * amount of memory (from both anon and file).
+ */
+static int test_memcg_reclaim(const char *root)
+{
+ int ret = KSFT_FAIL, fd;
+ char *memcg;
+ long current, to_reclaim;
+ char buf[64];
+
+ memcg = cg_name(root, "memcg_test");
+ if (!memcg)
+ goto cleanup;
+
+ if (cg_create(memcg))
+ goto cleanup;
+
+ current = cg_read_long(memcg, "memory.current");
+ if (current != 0)
+ goto cleanup;
+
+ cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(50));
+ sleep(1);
+
+ fd = get_temp_fd();
+ if (fd < 0)
+ goto cleanup;
+
+ cg_run_nowait(memcg, alloc_pagecache_50M_noexit, (void *)(long)fd);
+ sleep(1);
+
+ current = cg_read_long(memcg, "memory.current");
+ if (!values_close(current, MB(100), 10))
+ goto cleanup;
+
+ /*
+ * Reclaim until current reaches 30M, make sure to reclaim over 50M to
+ * hit both anon and file.
+ */
+ while (true) {
+ int err;
+
+ current = cg_read_long(memcg, "memory.current");
+ to_reclaim = current - MB(30);
+
+ /*
+ * We only keep looping if we get EAGAIN, which means we could
+ * not reclaim the full amount.
+ */
+ if (to_reclaim <= 0)
+ goto cleanup;
+
+
+ snprintf(buf, sizeof(buf), "%ld", to_reclaim);
+ err = cg_write(memcg, "memory.reclaim", buf);
+ if (!err) {
+ /*
+ * If writing succeeds, then the written amount should have been
+ * fully reclaimed (and maybe more).
+ */
+ current = cg_read_long(memcg, "memory.current");
+ if (!values_close(current, MB(30), 3) && current > MB(30))
+ goto cleanup;
+ break;
+ }
+
+ /* The kernel could not reclaim the full amount, try again. */
+ if (err == EAGAIN)
+ continue;
+
+ /* We got an unexpected error. */
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+cleanup:
+ cg_destroy(memcg);
+ free(memcg);
+ close(fd);
+
+ return ret;
+}
+
static int alloc_anon_50M_check_swap(const char *cgroup, void *arg)
{
long mem_max = (long)arg;
@@ -1263,6 +1347,7 @@ struct memcg_test {
T(test_memcg_high),
T(test_memcg_high_sync),
T(test_memcg_max),
+ T(test_memcg_reclaim),
T(test_memcg_oom_events),
T(test_memcg_swap_max),
T(test_memcg_sock),
--
2.35.1.1178.g4f1659d476-goog
Currently, alloc_anon_noexit() calls alloc_anon() which instantly frees
the allocated memory. alloc_anon_noexit() is usually used with
cg_run_nowait() to run a process in the background that allocates
memory. It makes sense for the background process to keep the memory
allocated and not instantly free it (otherwise there is no point of
running it in the background).
Signed-off-by: Yosry Ahmed <[email protected]>
---
tools/testing/selftests/cgroup/test_memcontrol.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 36ccf2322e21..f2ffb3a30194 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -211,13 +211,17 @@ static int alloc_pagecache_50M_noexit(const char *cgroup, void *arg)
static int alloc_anon_noexit(const char *cgroup, void *arg)
{
int ppid = getppid();
+ size_t size = (unsigned long)arg;
+ char *buf, *ptr;
- if (alloc_anon(cgroup, arg))
- return -1;
+ buf = malloc(size);
+ for (ptr = buf; ptr < buf + size; ptr += PAGE_SIZE)
+ *ptr = 0;
while (getppid() == ppid)
sleep(1);
+ free(buf);
return 0;
}
--
2.35.1.1178.g4f1659d476-goog
From: Shakeel Butt <[email protected]>
Introduce a memcg interface to trigger memory reclaim on a memory cgroup.
Use case: Proactive Reclaim
---------------------------
A userspace proactive reclaimer can continuously probe the memcg to
reclaim a small amount of memory. This gives more accurate and
up-to-date workingset estimation as the LRUs are continuously
sorted and can potentially provide more deterministic memory
overcommit behavior. The memory overcommit controller can provide
more proactive response to the changing behavior of the running
applications instead of being reactive.
A userspace reclaimer's purpose in this case is not a complete replacement
for kswapd or direct reclaim, it is to proactively identify memory savings
opportunities and reclaim some amount of cold pages set by the policy
to free up the memory for more demanding jobs or scheduling new jobs.
A user space proactive reclaimer is used in Google data centers.
Additionally, Meta's TMO paper recently referenced a very similar
interface used for user space proactive reclaim:
https://dl.acm.org/doi/pdf/10.1145/3503222.3507731
Benefits of a user space reclaimer:
-----------------------------------
1) More flexible on who should be charged for the cpu of the memory
reclaim. For proactive reclaim, it makes more sense to be centralized.
2) More flexible on dedicating the resources (like cpu). The memory
overcommit controller can balance the cost between the cpu usage and
the memory reclaimed.
3) Provides a way to the applications to keep their LRUs sorted, so,
under memory pressure better reclaim candidates are selected. This also
gives more accurate and uptodate notion of working set for an
application.
Why memory.high is not enough?
------------------------------
- memory.high can be used to trigger reclaim in a memcg and can
potentially be used for proactive reclaim.
However there is a big downside in using memory.high. It can potentially
introduce high reclaim stalls in the target application as the
allocations from the processes or the threads of the application can hit
the temporary memory.high limit.
- Userspace proactive reclaimers usually use feedback loops to decide
how much memory to proactively reclaim from a workload. The metrics
used for this are usually either refaults or PSI, and these metrics
will become messy if the application gets throttled by hitting the
high limit.
- memory.high is a stateful interface, if the userspace proactive
reclaimer crashes for any reason while triggering reclaim it can leave
the application in a bad state.
- If a workload is rapidly expanding, setting memory.high to proactively
reclaim memory can result in actually reclaiming more memory than
intended.
The benefits of such interface and shortcomings of existing interface
were further discussed in this RFC thread:
https://lore.kernel.org/linux-mm/[email protected]/
Interface:
----------
Introducing a very simple memcg interface 'echo 10M > memory.reclaim' to
trigger reclaim in the target memory cgroup.
The interface is introduced as a nested-keyed file to allow for future
optional arguments to be easily added to configure the behavior of
reclaim.
Possible Extensions:
--------------------
- This interface can be extended with an additional parameter or flags
to allow specifying one or more types of memory to reclaim from (e.g.
file, anon, ..).
- The interface can also be extended with a node mask to reclaim from
specific nodes. This has use cases for reclaim-based demotion in memory
tiering systens.
- A similar per-node interface can also be added to support proactive
reclaim and reclaim-based demotion in systems without memcg.
- Add a timeout parameter to make it easier for user space to call the
interface without worrying about being blocked for an undefined amount
of time.
For now, let's keep things simple by adding the basic functionality.
[[email protected]: refreshed to current master, updated commit
message based on recent discussions and use cases]
Signed-off-by: Shakeel Butt <[email protected]>
Signed-off-by: Yosry Ahmed <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
Acked-by: Michal Hocko <[email protected]>
Acked-by: Wei Xu <[email protected]>
---
Documentation/admin-guide/cgroup-v2.rst | 21 ++++++++++++++
mm/memcontrol.c | 37 +++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 69d7a6983f78..19bcd73cad03 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1208,6 +1208,27 @@ PAGE_SIZE multiple when read back.
high limit is used and monitored properly, this limit's
utility is limited to providing the final safety net.
+ memory.reclaim
+ A write-only nested-keyed file which exists for all cgroups.
+
+ This is a simple interface to trigger memory reclaim in the
+ target cgroup.
+
+ This file accepts a single key, the number of bytes to reclaim.
+ No nested keys are currently supported.
+
+ Example::
+
+ echo "1G" > memory.reclaim
+
+ The interface can be later extended with nested keys to
+ configure the reclaim behavior. For example, specify the
+ type of memory to reclaim from (anon, file, ..).
+
+ Please note that the kernel can over or under reclaim from
+ the target cgroup. If less bytes are reclaimed than the
+ specified amount, -EAGAIN is returned.
+
memory.oom.group
A read-write single value file which exists on non-root
cgroups. The default value is "0".
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 725f76723220..2b214b66d333 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6355,6 +6355,38 @@ static ssize_t memory_oom_group_write(struct kernfs_open_file *of,
return nbytes;
}
+static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
+ size_t nbytes, loff_t off)
+{
+ struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+ unsigned int nr_retries = MAX_RECLAIM_RETRIES;
+ unsigned long nr_to_reclaim, nr_reclaimed = 0;
+ int err;
+
+ buf = strstrip(buf);
+ err = page_counter_memparse(buf, "", &nr_to_reclaim);
+ if (err)
+ return err;
+
+ while (nr_reclaimed < nr_to_reclaim) {
+ unsigned long reclaimed;
+
+ if (signal_pending(current))
+ break;
+
+ reclaimed = try_to_free_mem_cgroup_pages(memcg,
+ nr_to_reclaim - nr_reclaimed,
+ GFP_KERNEL, true);
+
+ if (!reclaimed && !nr_retries--)
+ break;
+
+ nr_reclaimed += reclaimed;
+ }
+
+ return nr_reclaimed < nr_to_reclaim ? -EAGAIN : nbytes;
+}
+
static struct cftype memory_files[] = {
{
.name = "current",
@@ -6413,6 +6445,11 @@ static struct cftype memory_files[] = {
.seq_show = memory_oom_group_show,
.write = memory_oom_group_write,
},
+ {
+ .name = "reclaim",
+ .flags = CFTYPE_NS_DELEGATABLE,
+ .write = memory_reclaim,
+ },
{ } /* terminate */
};
--
2.35.1.1178.g4f1659d476-goog
On Fri 08-04-22 09:43:03, Dan Schatzberg wrote:
> On Fri, Apr 08, 2022 at 04:57:40AM +0000, Yosry Ahmed wrote:
> > +static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> > + size_t nbytes, loff_t off)
> > +{
> > + struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > + unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> > + unsigned long nr_to_reclaim, nr_reclaimed = 0;
> > + int err;
> > +
> > + buf = strstrip(buf);
> > + err = page_counter_memparse(buf, "", &nr_to_reclaim);
>
> Is there a reason not to support "max"? Empty string seems odd to me
> here.
I have to say I have missed the special meaning of the empty string here
and I agree this would indeed really weird. Does cgroup core even call
here? cgroup_file_write seems to drop !nbytes input.
Regarding "max" as a possible input. I am not really sure to be honest.
I can imagine that it could be legit to simply reclaim all the charges
(e.g. before removing the memcg) which should be achieveable by
reclaiming the reported consumption. Or what exactly should be the
semantic?
--
Michal Hocko
SUSE Labs
On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <[email protected]> wrote:
>
> Currently, cg_write() returns 0 on success and -1 on failure. Modify it
> to return the errno of write() syscall when write() fails.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
> ---
> tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++---------
> 1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
> index dbaa7aabbb4a..3b6bb09985fa 100644
> --- a/tools/testing/selftests/cgroup/cgroup_util.c
> +++ b/tools/testing/selftests/cgroup/cgroup_util.c
> @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len)
> return len;
> }
>
> -static ssize_t write_text(const char *path, char *buf, ssize_t len)
> +/*
> + * Returns:
> + * success -> 0
> + * open() failure -> -1
> + * write() failure -> errno
> + */
> +static int write_text(const char *path, char *buf, ssize_t len)
> {
> - int fd;
> + int fd, ret;
>
> fd = open(path, O_WRONLY | O_APPEND);
> if (fd < 0)
> return fd;
>
> - len = write(fd, buf, len);
> - if (len < 0) {
> - close(fd);
> - return len;
> - }
> -
> + ret = write(fd, buf, len) < 0 ? errno : 0;
> close(fd);
> -
> - return len;
> + return ret;
> }
>
> char *cg_name(const char *root, const char *name)
> @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control)
> return cnt;
> }
>
> +/*
> + * Returns:
> + * success -> 0
> + * open() failure -> -1
> + * write() failure -> errno
> + */
> int cg_write(const char *cgroup, const char *control, char *buf)
> {
> char path[PATH_MAX];
> ssize_t len = strlen(buf);
>
> snprintf(path, sizeof(path), "%s/%s", cgroup, control);
> -
> - if (write_text(path, buf, len) == len)
> - return 0;
> -
> - return -1;
> + return write_text(path, buf, len);
> }
I have changed this in v4 to a cleaner implementation that either
returns 0 on success or -errno on failure. I also made sure to check
that the full buffer was being written, and updated cg_read() as well
for the interface to be consistent.
Will send out once the discussion on patch 1 in v3 reaches a consensus.
>
> int cg_find_unified_root(char *root, size_t len)
> --
> 2.35.1.1178.g4f1659d476-goog
>
On Fri, Apr 8, 2022 at 7:55 AM Dan Schatzberg <[email protected]> wrote:
>
> On Fri, Apr 08, 2022 at 04:11:05PM +0200, Michal Hocko wrote:
> > Regarding "max" as a possible input. I am not really sure to be honest.
> > I can imagine that it could be legit to simply reclaim all the charges
> > (e.g. before removing the memcg) which should be achieveable by
> > reclaiming the reported consumption. Or what exactly should be the
> > semantic?
>
> Yeah, it just allows you to avoid reading memory.current to just
> reclaim everything if you can specify "max" - you're still protected
> by nretries to eventually bail out. Mostly, though I just feel like
> supporting "max" makes memory.reclaim semetric with a lot of the
> cgroup memory control files which tend to support "max".
One possible approach here is to have force_empty behavior when we
write "max" to memory.reclaim. From Google's perspective we don't have
a preference, but it seems to me like logical behavior. We can do this
either by directly calling mem_cgroup_force_empty() or just draining
stock and lrus in memory_reclaim().
This actually brings up another interesting point. Do you think we
should drain lrus if try_to_free_mem_cgroup_pages() fails to reclaim
the request amount? We can do this after the first call or before the
last one. It could introduce more evictable pages for
try_to_free_mem_cgroup_pages() to free.
On Fri, Apr 08, 2022 at 04:57:42AM +0000, Yosry Ahmed wrote:
> Currently, alloc_anon_noexit() calls alloc_anon() which instantly frees
> the allocated memory. alloc_anon_noexit() is usually used with
> cg_run_nowait() to run a process in the background that allocates
> memory. It makes sense for the background process to keep the memory
> allocated and not instantly free it (otherwise there is no point of
> running it in the background).
>
> Signed-off-by: Yosry Ahmed <[email protected]>
Acked-by: Roman Gushchin <[email protected]>
On Fri, Apr 08, 2022 at 04:57:40AM +0000, Yosry Ahmed wrote:
> From: Shakeel Butt <[email protected]>
>
> Introduce a memcg interface to trigger memory reclaim on a memory cgroup.
>
> Use case: Proactive Reclaim
> ---------------------------
>
> A userspace proactive reclaimer can continuously probe the memcg to
> reclaim a small amount of memory. This gives more accurate and
> up-to-date workingset estimation as the LRUs are continuously
> sorted and can potentially provide more deterministic memory
> overcommit behavior. The memory overcommit controller can provide
> more proactive response to the changing behavior of the running
> applications instead of being reactive.
>
> A userspace reclaimer's purpose in this case is not a complete replacement
> for kswapd or direct reclaim, it is to proactively identify memory savings
> opportunities and reclaim some amount of cold pages set by the policy
> to free up the memory for more demanding jobs or scheduling new jobs.
>
> A user space proactive reclaimer is used in Google data centers.
> Additionally, Meta's TMO paper recently referenced a very similar
> interface used for user space proactive reclaim:
> https://dl.acm.org/doi/pdf/10.1145/3503222.3507731
>
> Benefits of a user space reclaimer:
> -----------------------------------
>
> 1) More flexible on who should be charged for the cpu of the memory
> reclaim. For proactive reclaim, it makes more sense to be centralized.
>
> 2) More flexible on dedicating the resources (like cpu). The memory
> overcommit controller can balance the cost between the cpu usage and
> the memory reclaimed.
>
> 3) Provides a way to the applications to keep their LRUs sorted, so,
> under memory pressure better reclaim candidates are selected. This also
> gives more accurate and uptodate notion of working set for an
> application.
>
> Why memory.high is not enough?
> ------------------------------
>
> - memory.high can be used to trigger reclaim in a memcg and can
> potentially be used for proactive reclaim.
> However there is a big downside in using memory.high. It can potentially
> introduce high reclaim stalls in the target application as the
> allocations from the processes or the threads of the application can hit
> the temporary memory.high limit.
>
> - Userspace proactive reclaimers usually use feedback loops to decide
> how much memory to proactively reclaim from a workload. The metrics
> used for this are usually either refaults or PSI, and these metrics
> will become messy if the application gets throttled by hitting the
> high limit.
>
> - memory.high is a stateful interface, if the userspace proactive
> reclaimer crashes for any reason while triggering reclaim it can leave
> the application in a bad state.
>
> - If a workload is rapidly expanding, setting memory.high to proactively
> reclaim memory can result in actually reclaiming more memory than
> intended.
>
> The benefits of such interface and shortcomings of existing interface
> were further discussed in this RFC thread:
> https://lore.kernel.org/linux-mm/[email protected]/
>
> Interface:
> ----------
>
> Introducing a very simple memcg interface 'echo 10M > memory.reclaim' to
> trigger reclaim in the target memory cgroup.
>
> The interface is introduced as a nested-keyed file to allow for future
> optional arguments to be easily added to configure the behavior of
> reclaim.
>
> Possible Extensions:
> --------------------
>
> - This interface can be extended with an additional parameter or flags
> to allow specifying one or more types of memory to reclaim from (e.g.
> file, anon, ..).
>
> - The interface can also be extended with a node mask to reclaim from
> specific nodes. This has use cases for reclaim-based demotion in memory
> tiering systens.
>
> - A similar per-node interface can also be added to support proactive
> reclaim and reclaim-based demotion in systems without memcg.
>
> - Add a timeout parameter to make it easier for user space to call the
> interface without worrying about being blocked for an undefined amount
> of time.
>
> For now, let's keep things simple by adding the basic functionality.
>
> [[email protected]: refreshed to current master, updated commit
> message based on recent discussions and use cases]
> Signed-off-by: Shakeel Butt <[email protected]>
> Signed-off-by: Yosry Ahmed <[email protected]>
> Acked-by: Johannes Weiner <[email protected]>
> Acked-by: Michal Hocko <[email protected]>
> Acked-by: Wei Xu <[email protected]>
Acked-by: Roman Gushchin <[email protected]>
Thank you for incorporating all the feedback!
Nice work!
On Fri, Apr 08, 2022 at 04:57:41AM +0000, Yosry Ahmed wrote:
> Currently, cg_write() returns 0 on success and -1 on failure. Modify it
> to return the errno of write() syscall when write() fails.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
> ---
> tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++---------
> 1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
> index dbaa7aabbb4a..3b6bb09985fa 100644
> --- a/tools/testing/selftests/cgroup/cgroup_util.c
> +++ b/tools/testing/selftests/cgroup/cgroup_util.c
> @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len)
> return len;
> }
>
> -static ssize_t write_text(const char *path, char *buf, ssize_t len)
> +/*
> + * Returns:
> + * success -> 0
> + * open() failure -> -1
Is there a reason why we can't return errno in this case as well?
It would be more straightforward.
Otherwise the change looks good to me.
On Fri, Apr 08, 2022 at 04:57:40AM +0000, Yosry Ahmed wrote:
> +static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> + size_t nbytes, loff_t off)
> +{
> + struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> + unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> + unsigned long nr_to_reclaim, nr_reclaimed = 0;
> + int err;
> +
> + buf = strstrip(buf);
> + err = page_counter_memparse(buf, "", &nr_to_reclaim);
Is there a reason not to support "max"? Empty string seems odd to me
here.
> + if (err)
> + return err;
> +
> + while (nr_reclaimed < nr_to_reclaim) {
> + unsigned long reclaimed;
> +
> + if (signal_pending(current))
> + break;
I think this should be `return -EINTR;`
> +
> + reclaimed = try_to_free_mem_cgroup_pages(memcg,
> + nr_to_reclaim - nr_reclaimed,
> + GFP_KERNEL, true);
> +
> + if (!reclaimed && !nr_retries--)
> + break;
Here you can just `return -EAGAIN;`
> +
> + nr_reclaimed += reclaimed;
> + }
> +
> + return nr_reclaimed < nr_to_reclaim ? -EAGAIN : nbytes;
Then this can just be `return nbytes;`
I'm very much in favor of this new interface. Thanks for working on
it!
On Fri, Apr 8, 2022 at 6:44 PM Roman Gushchin <[email protected]> wrote:
>
> On Fri, Apr 08, 2022 at 06:21:55PM -0700, Yosry Ahmed wrote:
> > On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <[email protected]> wrote:
> > >
> > > Currently, cg_write() returns 0 on success and -1 on failure. Modify it
> > > to return the errno of write() syscall when write() fails.
> > >
> > > Signed-off-by: Yosry Ahmed <[email protected]>
> > > ---
> > > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++---------
> > > 1 file changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
> > > index dbaa7aabbb4a..3b6bb09985fa 100644
> > > --- a/tools/testing/selftests/cgroup/cgroup_util.c
> > > +++ b/tools/testing/selftests/cgroup/cgroup_util.c
> > > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len)
> > > return len;
> > > }
> > >
> > > -static ssize_t write_text(const char *path, char *buf, ssize_t len)
> > > +/*
> > > + * Returns:
> > > + * success -> 0
> > > + * open() failure -> -1
> > > + * write() failure -> errno
> > > + */
> > > +static int write_text(const char *path, char *buf, ssize_t len)
> > > {
> > > - int fd;
> > > + int fd, ret;
> > >
> > > fd = open(path, O_WRONLY | O_APPEND);
> > > if (fd < 0)
> > > return fd;
> > >
> > > - len = write(fd, buf, len);
> > > - if (len < 0) {
> > > - close(fd);
> > > - return len;
> > > - }
> > > -
> > > + ret = write(fd, buf, len) < 0 ? errno : 0;
> > > close(fd);
> > > -
> > > - return len;
> > > + return ret;
> > > }
> > >
> > > char *cg_name(const char *root, const char *name)
> > > @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control)
> > > return cnt;
> > > }
> > >
> > > +/*
> > > + * Returns:
> > > + * success -> 0
> > > + * open() failure -> -1
> > > + * write() failure -> errno
> > > + */
> > > int cg_write(const char *cgroup, const char *control, char *buf)
> > > {
> > > char path[PATH_MAX];
> > > ssize_t len = strlen(buf);
> > >
> > > snprintf(path, sizeof(path), "%s/%s", cgroup, control);
> > > -
> > > - if (write_text(path, buf, len) == len)
> > > - return 0;
> > > -
> > > - return -1;
> > > + return write_text(path, buf, len);
> > > }
> >
> > I have changed this in v4 to a cleaner implementation that either
> > returns 0 on success or -errno on failure. I also made sure to check
> > that the full buffer was being written, and updated cg_read() as well
> > for the interface to be consistent.
> >
> > Will send out once the discussion on patch 1 in v3 reaches a consensus.
>
> Ok, sounds good, please feel free to add my
> Acked-by: Roman Gushchin <[email protected]>
> for the whole series. It looks really nice and ready for merging upstream to me.
>
> Thanks!
Thanks a lot for reviewing it! I would appreciate it if you still took
a look at the v4 version of this patch when I send it (probably early
next week) as it's significantly different and includes changes to
cg_read() and read_text() as well.
On Fri, Apr 8, 2022 at 6:43 AM Dan Schatzberg <[email protected]> wrote:
>
> On Fri, Apr 08, 2022 at 04:57:40AM +0000, Yosry Ahmed wrote:
> > +static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> > + size_t nbytes, loff_t off)
> > +{
> > + struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > + unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> > + unsigned long nr_to_reclaim, nr_reclaimed = 0;
> > + int err;
> > +
> > + buf = strstrip(buf);
> > + err = page_counter_memparse(buf, "", &nr_to_reclaim);
>
> Is there a reason not to support "max"? Empty string seems odd to me
> here.
We can certainly support "max" to reclaim as much as we can with
MAX_RECLAIM_RETRIES, if there are no objections from the maintainers.
>
> > + if (err)
> > + return err;
> > +
> > + while (nr_reclaimed < nr_to_reclaim) {
> > + unsigned long reclaimed;
> > +
> > + if (signal_pending(current))
> > + break;
>
> I think this should be `return -EINTR;`
Yes this makes more sense. I think this was modeled after the if block
in memory_high_write(), but maybe it makes sense there to just report
success as the new high limit was set anyway.
Will change it in the next version.
>
> > +
> > + reclaimed = try_to_free_mem_cgroup_pages(memcg,
> > + nr_to_reclaim - nr_reclaimed,
> > + GFP_KERNEL, true);
> > +
> > + if (!reclaimed && !nr_retries--)
> > + break;
>
> Here you can just `return -EAGAIN;`
Will do.
>
> > +
> > + nr_reclaimed += reclaimed;
> > + }
> > +
> > + return nr_reclaimed < nr_to_reclaim ? -EAGAIN : nbytes;
>
> Then this can just be `return nbytes;`
Will do.
>
> I'm very much in favor of this new interface. Thanks for working on
> it!
Thanks so much for reviewing it!
On Fri, Apr 08, 2022 at 04:11:05PM +0200, Michal Hocko wrote:
> Regarding "max" as a possible input. I am not really sure to be honest.
> I can imagine that it could be legit to simply reclaim all the charges
> (e.g. before removing the memcg) which should be achieveable by
> reclaiming the reported consumption. Or what exactly should be the
> semantic?
Yeah, it just allows you to avoid reading memory.current to just
reclaim everything if you can specify "max" - you're still protected
by nretries to eventually bail out. Mostly, though I just feel like
supporting "max" makes memory.reclaim semetric with a lot of the
cgroup memory control files which tend to support "max".
On Fri, Apr 08, 2022 at 04:57:43AM +0000, Yosry Ahmed wrote:
> Add a new test for memory.reclaim that verifies that the interface
> correctly reclaims memory as intended, from both anon and file pages.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
Acked-by: Roman Gushchin <[email protected]>
Thank you for adding a test!
On Fri, Apr 08, 2022 at 06:21:55PM -0700, Yosry Ahmed wrote:
> On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <[email protected]> wrote:
> >
> > Currently, cg_write() returns 0 on success and -1 on failure. Modify it
> > to return the errno of write() syscall when write() fails.
> >
> > Signed-off-by: Yosry Ahmed <[email protected]>
> > ---
> > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++---------
> > 1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
> > index dbaa7aabbb4a..3b6bb09985fa 100644
> > --- a/tools/testing/selftests/cgroup/cgroup_util.c
> > +++ b/tools/testing/selftests/cgroup/cgroup_util.c
> > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len)
> > return len;
> > }
> >
> > -static ssize_t write_text(const char *path, char *buf, ssize_t len)
> > +/*
> > + * Returns:
> > + * success -> 0
> > + * open() failure -> -1
> > + * write() failure -> errno
> > + */
> > +static int write_text(const char *path, char *buf, ssize_t len)
> > {
> > - int fd;
> > + int fd, ret;
> >
> > fd = open(path, O_WRONLY | O_APPEND);
> > if (fd < 0)
> > return fd;
> >
> > - len = write(fd, buf, len);
> > - if (len < 0) {
> > - close(fd);
> > - return len;
> > - }
> > -
> > + ret = write(fd, buf, len) < 0 ? errno : 0;
> > close(fd);
> > -
> > - return len;
> > + return ret;
> > }
> >
> > char *cg_name(const char *root, const char *name)
> > @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control)
> > return cnt;
> > }
> >
> > +/*
> > + * Returns:
> > + * success -> 0
> > + * open() failure -> -1
> > + * write() failure -> errno
> > + */
> > int cg_write(const char *cgroup, const char *control, char *buf)
> > {
> > char path[PATH_MAX];
> > ssize_t len = strlen(buf);
> >
> > snprintf(path, sizeof(path), "%s/%s", cgroup, control);
> > -
> > - if (write_text(path, buf, len) == len)
> > - return 0;
> > -
> > - return -1;
> > + return write_text(path, buf, len);
> > }
>
> I have changed this in v4 to a cleaner implementation that either
> returns 0 on success or -errno on failure. I also made sure to check
> that the full buffer was being written, and updated cg_read() as well
> for the interface to be consistent.
>
> Will send out once the discussion on patch 1 in v3 reaches a consensus.
Ok, sounds good, please feel free to add my
Acked-by: Roman Gushchin <[email protected]>
for the whole series. It looks really nice and ready for merging upstream to me.
Thanks!
On Fri 08-04-22 10:55:56, Dan Schatzberg wrote:
> On Fri, Apr 08, 2022 at 04:11:05PM +0200, Michal Hocko wrote:
> > Regarding "max" as a possible input. I am not really sure to be honest.
> > I can imagine that it could be legit to simply reclaim all the charges
> > (e.g. before removing the memcg) which should be achieveable by
> > reclaiming the reported consumption. Or what exactly should be the
> > semantic?
>
> Yeah, it just allows you to avoid reading memory.current to just
> reclaim everything if you can specify "max"
The same could be achieved by requesting a really high number (-1Ul)
> - you're still protected
> by nretries to eventually bail out.
The number of retries is an implementation detail and nobody should
really rely on that. Bail out on signal can be still used so yeah
getting a large input or whatever alias of that should be just fine.
> Mostly, though I just feel like
> supporting "max" makes memory.reclaim semetric with a lot of the
> cgroup memory control files which tend to support "max".
max is used for limits now and this doesn't have a semantic of one.
But I have to say I do not really feel strongly about this.
--
Michal Hocko
SUSE Labs
On Fri, Apr 8, 2022 at 1:08 PM Yosry Ahmed <[email protected]> wrote:
>
> On Fri, Apr 8, 2022 at 7:55 AM Dan Schatzberg <[email protected]> wrote:
> >
> > On Fri, Apr 08, 2022 at 04:11:05PM +0200, Michal Hocko wrote:
> > > Regarding "max" as a possible input. I am not really sure to be honest.
> > > I can imagine that it could be legit to simply reclaim all the charges
> > > (e.g. before removing the memcg) which should be achieveable by
> > > reclaiming the reported consumption. Or what exactly should be the
> > > semantic?
> >
> > Yeah, it just allows you to avoid reading memory.current to just
> > reclaim everything if you can specify "max" - you're still protected
> > by nretries to eventually bail out. Mostly, though I just feel like
> > supporting "max" makes memory.reclaim semetric with a lot of the
> > cgroup memory control files which tend to support "max".
>
> One possible approach here is to have force_empty behavior when we
> write "max" to memory.reclaim. From Google's perspective we don't have
> a preference, but it seems to me like logical behavior. We can do this
> either by directly calling mem_cgroup_force_empty() or just draining
> stock and lrus in memory_reclaim().
>
> This actually brings up another interesting point. Do you think we
> should drain lrus if try_to_free_mem_cgroup_pages() fails to reclaim
> the request amount? We can do this after the first call or before the
> last one. It could introduce more evictable pages for
> try_to_free_mem_cgroup_pages() to free.
Hey Michal, any thoughts on this? I am looking for feedback on this
before I send out v4.
On Thu 14-04-22 10:25:29, Yosry Ahmed wrote:
> On Fri, Apr 8, 2022 at 1:08 PM Yosry Ahmed <[email protected]> wrote:
> >
> > On Fri, Apr 8, 2022 at 7:55 AM Dan Schatzberg <[email protected]> wrote:
> > >
> > > On Fri, Apr 08, 2022 at 04:11:05PM +0200, Michal Hocko wrote:
> > > > Regarding "max" as a possible input. I am not really sure to be honest.
> > > > I can imagine that it could be legit to simply reclaim all the charges
> > > > (e.g. before removing the memcg) which should be achieveable by
> > > > reclaiming the reported consumption. Or what exactly should be the
> > > > semantic?
> > >
> > > Yeah, it just allows you to avoid reading memory.current to just
> > > reclaim everything if you can specify "max" - you're still protected
> > > by nretries to eventually bail out. Mostly, though I just feel like
> > > supporting "max" makes memory.reclaim semetric with a lot of the
> > > cgroup memory control files which tend to support "max".
> >
> > One possible approach here is to have force_empty behavior when we
> > write "max" to memory.reclaim. From Google's perspective we don't have
> > a preference, but it seems to me like logical behavior. We can do this
> > either by directly calling mem_cgroup_force_empty() or just draining
> > stock and lrus in memory_reclaim().
> >
> > This actually brings up another interesting point. Do you think we
> > should drain lrus if try_to_free_mem_cgroup_pages() fails to reclaim
> > the request amount? We can do this after the first call or before the
> > last one. It could introduce more evictable pages for
> > try_to_free_mem_cgroup_pages() to free.
>
> Hey Michal, any thoughts on this? I am looking for feedback on this
> before I send out v4.
As I've already said I do not have strong preferences for the "max"
value to be accepted by the interface. Maybe you can add a support for
max in a separate patch so the discussion would not delay the rest of
the work.
For the LRU draining I do not see any problem for that to be added. The
overhead of the operation would increase, especially on larger machines,
which could be a concern. So the real question is whether not doing so
is a big problem. Our force_empty implementation optimistically drains
pcp caches but please note that this is not really guranteeing anything
as charges can happen at any time.
--
Michal Hocko
SUSE Labs