Re: [PATCH v2 2/3] selftests: cgroup: refactor proactive reclaim code to reclaim_until()

[Yosry Ahmed]

On Wed, Nov 23, 2022 at 7:16 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> 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.

Roman, any thoughts on this?

>
>
> 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!