diff mbox series

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

Message ID 20221123092132.2521764-3-yosryahmed@google.com
State New
Headers
Series mm: memcg: fix protection of reclaim target memcg |

Commit Message

Yosry Ahmed Nov. 23, 2022, 9:21 a.m. UTC 
  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@google.com>
---
 .../selftests/cgroup/test_memcontrol.c        | 85 +++++++++++--------
 1 file changed, 49 insertions(+), 36 deletions(-)

Comments

Roman Gushchin Nov. 24, 2022, 1:03 a.m. UTC | #1 
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@google.com>
> ---
>  .../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@linux.dev>
(idk if you want invest your time in further simplication of this code,
it was this way before this patch, so up to you).

>  }
>  
> +/* 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!
Yosry Ahmed Nov. 24, 2022, 3:16 a.m. UTC | #2 
On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@linux.dev> 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@google.com>
> > ---
> >  .../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@linux.dev>
> (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@google.com>
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@google.com>
---
 .../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!
Yosry Ahmed Nov. 29, 2022, 7:42 p.m. UTC | #3 
On Wed, Nov 23, 2022 at 7:16 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@linux.dev> 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@google.com>
> > > ---
> > >  .../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@linux.dev>
> > (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@google.com>
> 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@google.com>
> ---
>  .../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!
Roman Gushchin Nov. 30, 2022, 5:19 p.m. UTC | #4 
On Tue, Nov 29, 2022 at 11:42:31AM -0800, Yosry Ahmed wrote:
> On Wed, Nov 23, 2022 at 7:16 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@linux.dev> 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@google.com>
> > > > ---
> > > >  .../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@linux.dev>
> > > (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@google.com>
> > 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@google.com>
> > ---
> >  .../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;
> > -

Hi Yosry!

Thank you for working on this!
I feel like it's still way more complex than it can be.
How about something like this? (completely untested, treat is
as a pseudo-code).


{
	...
	bool ret = false;

	for (retries = 5; retries > 0; retries--) {
		current = cg_read_long(memcg, "memory.current");

		if (current <= goal) // replace with values_close?
			break;

		to_reclaim = current - goal_usage;
		snprintf(buf, sizeof(buf), "%ld", to_reclaim);
		err = cg_write(memcg, "memory.reclaim", buf);
		if (!err)
			ret = true;
		else if (err != -AGAIN)
			break;
	}

	return ret;
}
Yosry Ahmed Nov. 30, 2022, 6:25 p.m. UTC | #5 
On Wed, Nov 30, 2022 at 9:20 AM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>
> On Tue, Nov 29, 2022 at 11:42:31AM -0800, Yosry Ahmed wrote:
> > On Wed, Nov 23, 2022 at 7:16 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@linux.dev> 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@google.com>
> > > > > ---
> > > > >  .../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@linux.dev>
> > > > (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@google.com>
> > > 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@google.com>
> > > ---
> > >  .../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;
> > > -
>
> Hi Yosry!
>
> Thank you for working on this!
> I feel like it's still way more complex than it can be.
> How about something like this? (completely untested, treat is
> as a pseudo-code).

Thanks Roman!

This looks much simpler, and it nicely and subtly catches the false
negative case (where we return -EAGAIN but have actually reclaimed the
requested amount), but I think it doesn't catch the false positive
case (where memory.reclaim returns 0 but hasn't reclaimed enough
memory). In this case I think we will just keep retrying and ignore
the false positive?

Maybe with the following added check?

>
>
> {
>         ...
>         bool ret = false;
>
>         for (retries = 5; retries > 0; retries--) {
>                 current = cg_read_long(memcg, "memory.current");
>
>                 if (current <= goal) // replace with values_close?
>                         break;
                   else if (ret) { // false positive?
                            ret = false;
                            break;
                    }
>
>                 to_reclaim = current - goal_usage;
>                 snprintf(buf, sizeof(buf), "%ld", to_reclaim);
>                 err = cg_write(memcg, "memory.reclaim", buf);
>                 if (!err)
>                         ret = true;
>                 else if (err != -AGAIN)
>                         break;
>         }
>
>         return ret;
> }

Also, please let me know if you prefer that I send this cleanup in the
same thread like the above, in a completely separate patch that
depends on this series, or have it squashed into this patch in a v3.

Thanks again!
Yosry Ahmed Dec. 2, 2022, 3:19 a.m. UTC | #6 
On Wed, Nov 30, 2022 at 10:25 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Wed, Nov 30, 2022 at 9:20 AM Roman Gushchin <roman.gushchin@linux.dev> wrote:
> >
> > On Tue, Nov 29, 2022 at 11:42:31AM -0800, Yosry Ahmed wrote:
> > > On Wed, Nov 23, 2022 at 7:16 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > > >
> > > > On Wed, Nov 23, 2022 at 5:03 PM Roman Gushchin <roman.gushchin@linux.dev> 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@google.com>
> > > > > > ---
> > > > > >  .../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@linux.dev>
> > > > > (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@google.com>
> > > > 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@google.com>
> > > > ---
> > > >  .../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;
> > > > -
> >
> > Hi Yosry!
> >
> > Thank you for working on this!
> > I feel like it's still way more complex than it can be.
> > How about something like this? (completely untested, treat is
> > as a pseudo-code).
>
> Thanks Roman!
>
> This looks much simpler, and it nicely and subtly catches the false
> negative case (where we return -EAGAIN but have actually reclaimed the
> requested amount), but I think it doesn't catch the false positive
> case (where memory.reclaim returns 0 but hasn't reclaimed enough
> memory). In this case I think we will just keep retrying and ignore
> the false positive?
>
> Maybe with the following added check?
>
> >
> >
> > {
> >         ...
> >         bool ret = false;
> >
> >         for (retries = 5; retries > 0; retries--) {
> >                 current = cg_read_long(memcg, "memory.current");
> >
> >                 if (current <= goal) // replace with values_close?
> >                         break;
>                    else if (ret) { // false positive?
>                             ret = false;
>                             break;
>                     }
> >
> >                 to_reclaim = current - goal_usage;
> >                 snprintf(buf, sizeof(buf), "%ld", to_reclaim);
> >                 err = cg_write(memcg, "memory.reclaim", buf);
> >                 if (!err)
> >                         ret = true;
> >                 else if (err != -AGAIN)
> >                         break;
> >         }
> >
> >         return ret;
> > }
>
> Also, please let me know if you prefer that I send this cleanup in the
> same thread like the above, in a completely separate patch that
> depends on this series, or have it squashed into this patch in a v3.
>
> Thanks again!

I realized I missed a few folks in the CC of this version anyway. Sent
v3 with the suggested refactoring (+ the missing check for false
positives) squashed into this patch. Also included your review tags on
patches 1 & 3 (patch 2 was almost rewritten according to your
suggestions, so I dropped the review tag and added a suggested tag):

https://lore.kernel.org/lkml/20221202031512.1365483-1-yosryahmed@google.com/

Thanks!
diff mbox series

Patch

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;
 }
 
+/* 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;
+			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;
+}
+
 /*
  * This test checks that memory.reclaim reclaims the given
  * amount of memory (from both anon and file, if possible).
@@ -653,8 +700,7 @@  static int test_memcg_reclaim(const char *root)
 {
 	int ret = KSFT_FAIL, fd, retries;
 	char *memcg;
-	long current, expected_usage, to_reclaim;
-	char buf[64];
+	long current, expected_usage;
 
 	memcg = cg_name(root, "memcg_test");
 	if (!memcg)
@@ -705,41 +751,8 @@  static int test_memcg_reclaim(const char *root)
 	 * Reclaim until current reaches 30M, this makes sure we hit both anon
 	 * and file if swap is enabled.
 	 */
-	retries = 5;
-	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 && retries--)
-			continue;
-
-		/* We got an unexpected error or ran out of retries. */
+	if (!reclaim_until(memcg, MB(30)))
 		goto cleanup;
-	}
 
 	ret = KSFT_PASS;
 cleanup: