[PATCH] selftests: cgroup: simplify memcg reclaim code

[Yosry Ahmed]

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!