| Message ID | 20221123092132.2521764-3-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memcg: fix protection of reclaim target memcg | expand |
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!
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!
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!
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; }
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!
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 --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:
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(-)