On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@xxxxxxxxx> wrote:
>
> On Wed, Nov 23, 2022 at 09:21:31AM +0000, Yosry Ahmed wrote:
> > Refactor the code that drives writing to memory.reclaim (retrying, error
> > handling, etc) from test_memcg_reclaim() to a helper called
> > reclaim_until(), which proactively reclaims from a memcg until its
> > usage reaches a certain value.
> >
> > This will be used in a following patch in another test.
> >
> > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > ---
> > .../selftests/cgroup/test_memcontrol.c | 85 +++++++++++--------
> > 1 file changed, 49 insertions(+), 36 deletions(-)
> >
> > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> > index 8833359556f3..d4182e94945e 100644
> > --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> > @@ -645,6 +645,53 @@ static int test_memcg_max(const char *root)
> > return ret;
>
>
> The code below looks correct, but can be simplified a bit.
> And btw thank you for adding a test!
>
> Reviewed-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> (idk if you want invest your time in further simplication of this code,
> it was this way before this patch, so up to you).
I don't "want" to, but the voices in my head won't shut up until I do so..
Here's a patch that simplifies the code, I inlined it here to avoid
sending a new version. If it looks good to you, it can be squashed
into this patch or merged separately (whatever you and Andrew prefer).
I can also send it in a separate thread if preferred.
>From 18c40d61dac05b33cfc9233b17979b54422ed7c5 Mon Sep 17 00:00:00 2001
From: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
Date: Thu, 24 Nov 2022 02:21:12 +0000
Subject: [PATCH] selftests: cgroup: simplify memcg reclaim code
Simplify the code for the reclaim_until() helper used for memcg reclaim
through memory.reclaim.
Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
---
.../selftests/cgroup/test_memcontrol.c | 65 ++++++++++---------
1 file changed, 33 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c
b/tools/testing/selftests/cgroup/test_memcontrol.c
index bac3b91f1579..2e2bde44a6f7 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -17,6 +17,7 @@
#include <netdb.h>
#include <errno.h>
#include <sys/mman.h>
+#include <limits.h>
#include "../kselftest.h"
#include "cgroup_util.h"
@@ -656,51 +657,51 @@ static int test_memcg_max(const char *root)
return ret;
}
-/* Reclaim from @memcg until usage reaches @goal_usage */
+/*
+ * Reclaim from @memcg until usage reaches @goal_usage by writing to
+ * memory.reclaim.
+ *
+ * This function will return false if the usage is already below the
+ * goal.
+ *
+ * This function assumes that writing to memory.reclaim is the only
+ * source of change in memory.current (no concurrent allocations or
+ * reclaim).
+ *
+ * This function makes sure memory.reclaim is sane. It will return
+ * false if memory.reclaim's error codes do not make sense, even if
+ * the usage goal was satisfied.
+ */
static bool reclaim_until(const char *memcg, long goal_usage)
{
char buf[64];
int retries = 5;
- int err;
+ int err = INT_MAX;
long current, to_reclaim;
- /* Nothing to do here */
- if (cg_read_long(memcg, "memory.current") <= goal_usage)
- return true;
-
while (true) {
current = cg_read_long(memcg, "memory.current");
- to_reclaim = current - goal_usage;
- /*
- * We only keep looping if we get -EAGAIN, which means we could
- * not reclaim the full amount. This means we got -EAGAIN when
- * we actually reclaimed the requested amount, so fail.
- */
- if (to_reclaim <= 0)
- break;
+ /* First iteration*/
+ if (err == INT_MAX) {
+ if (current <= goal_usage)
+ return false;
+ /* Write successful, check reclaimed amount */
+ } else if (!err) {
+ return current <= goal_usage ||
+ values_close(current, goal_usage, 3);
+ /* Unexpected error, or ran out of retries */
+ } else if (err != -EAGAIN || !retries--) {
+ return false;
+ /* EAGAIN -> retry, but check for false negatives */
+ } else if (current <= goal_usage) {
+ return false;
+ }
+ to_reclaim = current - goal_usage;
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, goal_usage, 3) &&
current > goal_usage)
- break;
- return true;
- }
-
- /* The kernel could not reclaim the full amount, try again. */
- if (err == -EAGAIN && retries--)
- continue;
-
- /* We got an unexpected error or ran out of retries. */
- break;
}
- return false;
}
/*
--
2.38.1.584.g0f3c55d4c2-goog
>
> > }
> >
> > +/* Reclaim from @memcg until usage reaches @goal_usage */
> > +static bool reclaim_until(const char *memcg, long goal_usage)
> > +{
> > + char buf[64];
> > + int retries = 5;
> > + int err;
> > + long current, to_reclaim;
> > +
> > + /* Nothing to do here */
> > + if (cg_read_long(memcg, "memory.current") <= goal_usage)
> > + return true;
> > +
> > + while (true) {
> > + current = cg_read_long(memcg, "memory.current");
> > + to_reclaim = current - goal_usage;
> > +
> > + /*
> > + * We only keep looping if we get -EAGAIN, which means we could
> > + * not reclaim the full amount. This means we got -EAGAIN when
> > + * we actually reclaimed the requested amount, so fail.
> > + */
> > + if (to_reclaim <= 0)
> > + break;
> > +
> > + 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, goal_usage, 3) && current > goal_usage)
> > + break;
>
> There are 3 places in this function where memory.current is read and compared
> to goal_usage. I believe only one can be left.
>
> > + return true;
> > + }
> > +
> > + /* The kernel could not reclaim the full amount, try again. */
> > + if (err == -EAGAIN && retries--)
> > + continue;
> > +
> > + /* We got an unexpected error or ran out of retries. */
> > + break;
>
> if (err != -EAGAIN || retries--)
> break;
>
> Thanks!
