linux-kernel - Re: [PATCH v2 2/3] selftests: cgroup: refactor proactive reclaim code to reclaim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y37C68kUYakRQ2ZC@P9FQF9L96D.corp.robot.car>
Date:   Wed, 23 Nov 2022 17:03:39 -0800
From:   Roman Gushchin <roman.gushchin@...ux.dev>
To:     Yosry Ahmed <yosryahmed@...gle.com>
Cc:     Shakeel Butt <shakeelb@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...e.com>, Yu Zhao <yuzhao@...gle.com>,
        Muchun Song <songmuchun@...edance.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Vasily Averin <vasily.averin@...ux.dev>,
        Vlastimil Babka <vbabka@...e.cz>,
        Chris Down <chris@...isdown.name>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2 2/3] selftests: cgroup: refactor proactive reclaim
 code to reclaim_until()

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@...gle.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@...ux.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!