2023-09-12 15:53:24

by Domenico Cerasuolo

Subject: [PATCH] selftests: cgroup: fix test_zswap error path and meaningless check

Replace destruction paths with simple returns before the test cgroup is
created, there is nothing to free or destroy at that point.

Remove pointless check, stored_pages is a size_t and cannot be < 0.

Fixes: a549f9f31561 ("selftests: cgroup: add test_zswap with no kmem bypass test")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Domenico Cerasuolo <[email protected]>
---
tools/testing/selftests/cgroup/test_zswap.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index 49def87a909b..5257106776d5 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -178,10 +178,10 @@ static int test_no_kmem_bypass(const char *root)

/* Set up test memcg */
if (cg_write(root, "cgroup.subtree_control", "+memory"))
- goto out;
+ return KSFT_FAIL;
test_group = cg_name(root, "kmem_bypass_test");
if (!test_group)
- goto out;
+ return KSFT_FAIL;

/* Spawn memcg child and wait for it to allocate */
set_min_free_kb(min_free_kb_low);
@@ -208,8 +208,6 @@ static int test_no_kmem_bypass(const char *root)
free(trigger_allocation);
if (get_zswap_stored_pages(&stored_pages))
break;
- if (stored_pages < 0)
- break;
/* If memory was pushed to zswap, verify it belongs to memcg */
if (stored_pages > stored_pages_threshold) {
int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped ");
--
2.34.1

2023-09-13 00:57:46

by Andrew Morton

Subject: Re: [PATCH] selftests: cgroup: fix test_zswap error path and meaningless check

On Tue, 12 Sep 2023 10:38:00 +0200 Domenico Cerasuolo <[email protected]> wrote:

> Replace destruction paths with simple returns before the test cgroup is
> created, there is nothing to free or destroy at that point.
>
> Remove pointless check, stored_pages is a size_t and cannot be < 0.
>
> ...
>
> @@ -208,8 +208,6 @@ static int test_no_kmem_bypass(const char *root)
> free(trigger_allocation);
> if (get_zswap_stored_pages(&stored_pages))
> break;
> - if (stored_pages < 0)
> - break;
> /* If memory was pushed to zswap, verify it belongs to memcg */
> if (stored_pages > stored_pages_threshold) {
> int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped ");

stored_pages will be set to -1 on error. It would be better to cast
stored_pages to ssize_t in the check, rather than simply ignoring
errors?

2023-09-13 13:23:50

by Domenico Cerasuolo

Subject: Re: [PATCH] selftests: cgroup: fix test_zswap error path and meaningless check

On Tue, Sep 12, 2023 at 7:18 PM Andrew Morton <[email protected]> wrote:
>
> On Tue, 12 Sep 2023 10:38:00 +0200 Domenico Cerasuolo <[email protected]> wrote:
>
> > Replace destruction paths with simple returns before the test cgroup is
> > created, there is nothing to free or destroy at that point.
> >
> > Remove pointless check, stored_pages is a size_t and cannot be < 0.
> >
> > ...
> >
> > @@ -208,8 +208,6 @@ static int test_no_kmem_bypass(const char *root)
> > free(trigger_allocation);
> > if (get_zswap_stored_pages(&stored_pages))
> > break;
> > - if (stored_pages < 0)
> > - break;
> > /* If memory was pushed to zswap, verify it belongs to memcg */
> > if (stored_pages > stored_pages_threshold) {
> > int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped ");
>
> stored_pages will be set to -1 on error. It would be better to cast
> stored_pages to ssize_t in the check, rather than simply ignoring
> errors?

I'm not sure I understood, isn't it safe to just rely on the return value of
get_zswap_stored_pages a few lines above? In that case we break the loop and
the value of stored_pages is not used.