From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id ABA2BC4345F for ; Fri, 3 May 2024 11:16:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3B3C96B0082; Fri, 3 May 2024 07:16:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 363A46B0083; Fri, 3 May 2024 07:16:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 22BBE6B0088; Fri, 3 May 2024 07:16:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 07DB16B0082 for ; Fri, 3 May 2024 07:16:42 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 51E7040FF3 for ; Fri, 3 May 2024 11:16:41 +0000 (UTC) X-FDA: 82076831802.18.A3EB4BD Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf02.hostedemail.com (Postfix) with ESMTP id 2362380012 for ; Fri, 3 May 2024 11:16:34 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=KnFns9Gi; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714734995; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Eb+xzn111ko7br6Rv+Ab3g9AwNQcxovpEvUU1HG7Y40=; b=I5kuZTg0YlpGntnwStodpVE6GHGjtwa1v+z4SBLDhCQiZQg6L9WH7784uSf6tDREQt4bbV VlrJEVUqYYEjrMwZp4jEfyfGbkBa1lFiOihgBq9t+Aosbo+jDaJz2eh/gkrjCF33n28TJG YmQiWVrqnHK22aqzwFtU3JKDSBNROpY= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=KnFns9Gi; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714734995; a=rsa-sha256; cv=none; b=S8Y/OOpp81jueqoywj8XqTQD9WXF/BZbuzkpdMYEsZ1UO7ysm8l5eaZSnA/ceD7uBgTCv4 8CgVKVhvloxzc4T4XJAzFyBmqZeVkOmuYXPywzRa8CxUXTMqF+U+xOIko55t5CoA5YWAM1 ralrIwCnK/eQlGJvdaoFCZ0wdVeeWKM= Received: by mail-lf1-f52.google.com with SMTP id 2adb3069b0e04-51ab4ee9df8so11060397e87.1 for ; Fri, 03 May 2024 04:16:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714734993; x=1715339793; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Eb+xzn111ko7br6Rv+Ab3g9AwNQcxovpEvUU1HG7Y40=; b=KnFns9GiOfgwfZGyDb1xdgAh7mCO5R7xhk9ykY1040mZJQO5y8cg/aphFESmb1FXJw nFLxHozquFH3+qaXk/zqW0pRu/xU6g8nUyHFXdVTujhI6FnnYI9kw/2o16NIKQ7i5iC+ HyLPwQ/rrifh4yPRZEWLbmMd3W3JS/wKskI0lb9C1cr+Tb2MbxtHQU1OdXm/Eabnswkq lxv+Ro7ObpVgKr9gz6vRpxI8sudqDd7mg6WAf3FscfCTvwFySWDga+GAB2pUH0SQJg8j y5Iua3tF3XnYgu13iR7ga1a6ZGK7KeVUernlWtH5NBZA5gHwdJaa+O8RwPF7NP5UXZU4 UgLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714734993; x=1715339793; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Eb+xzn111ko7br6Rv+Ab3g9AwNQcxovpEvUU1HG7Y40=; b=d2ja1ihL/tKM3cTYnlsEpT6wEHHSrcF1uOdlSnidKgBuSoyWDjWVaBITkAK5SKtXq1 rslK0HScT1uifKSALX8SegAK3b0UEGwklzBnsO5jOy71pj+D8K4FIkl5C9Q3Ynj8ZiTT bIG/Oh2IwYA8RlblHoLq0qPOoQXGcI9lHt0XmYeHFARYYcen124a5vH/RvPvePQZZLTa 6RyseOUwt+dFzlsQ7FXaPT9JVbNoU0w96fj/X80FzjpLeg+zBGVUOLJkcCAYsIJEOsD5 4bNYFKwTB3Hd2Km05apbVX5uPQQJjtcO5GpBFe9H+Qca2q1Kak5hIfKqlbAjWy+Wus+f igpA== X-Forwarded-Encrypted: i=1; AJvYcCVK+QECf+UrN42nnh/S+Uo4spalEalgdSq2R7dlnsoUwsql4Vct6+T5O2azKD6ORTAdGFjspbqnsPEB0D4rjDedPoM= X-Gm-Message-State: AOJu0Yx6/lWDIsQilHGSW7mc9qb7j6fxedkv0gCQGkh978yGtB+9qgPa TlNdzlOl8F8ePf5sUPGPFVsQe5oOJ4enzkzD9sNA2STinsFIet5wWVaDXY5GP5zPkudb X-Google-Smtp-Source: AGHT+IGi0W+YmKQJDwxXSpcwYiXSiYGwe77f+H5HvZp62ZIkGwt7vARw516WD4uKceoJzGMlHweyOg== X-Received: by 2002:a05:6512:715:b0:51f:315c:75e0 with SMTP id b21-20020a056512071500b0051f315c75e0mr1340644lfs.44.1714734992894; Fri, 03 May 2024 04:16:32 -0700 (PDT) Received: from ?IPV6:2a03:83e0:1126:4:eb:d0d0:c7fd:c82c? ([2620:10d:c092:500::4:f792]) by smtp.gmail.com with ESMTPSA id f6-20020a056402160600b005722ce89ae2sm1575017edv.38.2024.05.03.04.16.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 May 2024 04:16:32 -0700 (PDT) Message-ID: <10bf047a-88b8-4502-a7e2-1ae35f8d57ec@gmail.com> Date: Fri, 3 May 2024 12:16:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/1] selftests: cgroup: add tests to verify the zswap writeback path To: Yosry Ahmed Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com References: <20240502190223.3975970-1-usamaarif642@gmail.com> <20240502190223.3975970-2-usamaarif642@gmail.com> Content-Language: en-US From: Usama Arif In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 2362380012 X-Stat-Signature: jnf4s3ciif1n91bjhfuomncuustkwwaa X-HE-Tag: 1714734994-668786 X-HE-Meta: U2FsdGVkX19yIbN3zKcQpoQmFo+lgkXPHVfc9CitQ65zcsdJqfoFFzwQg+aJLP5SmVkwItpqiVGLxKss1zkhacN4FNxfMcSzkO7qrBOeLMNeNIb6c1GtFxweK+7sHRKhS4+hughFNqX1L7Te3QJyd/d7Id72bjsIha8ywaIkpIcsXbys8/6afpIeVpU3hwyKQ0OkYDd0lHOocdVojXjKax6r2KS2M2nu7bTXcemr1CcgOk0RvmC2BYPig3B76nOwCT+EcNmxq3AsVFZcrQjYfVtDw5eN8vcN46X174kJao/RmARyNMxLzhQztfP1GqLgHKNlrDCHHLxOARzbrJqmrDXRGCJSH5fjgnWfiN2Hq8TQzXFNwmjF2wHwpxjA2kC23fSBJZVImG43jGqD6ozBSwfjYDzltO4/7smBLxZT+gfZSbq1Zr4xB3tcHHMNDKEul3hlFHhPViUGK3Ok8iTtUhacrp9xD26zlD6zB/JA7jahzE9AZta6QBRYEAS0+sUQph4NmPhb6wtoz0/U9pFuLRrdHzdc6S3b4dFLWW2kiEDB9kd9hx2i8x5yAgUlNettQla5eHMaF8YQJvRx2PmD574CHFGtiz/e1DmOIVa/Wb4sRu/5nTZ+MVlIcbTN6ig8S/XaNUKzPUCLQHEHbd4N4XCdVFuDo+Ix/nNCYRwP7Z4vbHEcdU++38FS4QyI+dG8H4nDZxZbJKwL86NDxIAIn5uuPXdgCv1GUSMqGqxsZLsin8N7C3QQbAyaJ2P+e/iaogDlSbswYW+lxa10rko8KtWG5wJ4OYjbylFVUVqVwljCF3CcDY6psNmkzx0PXMIg6qBtVi2YBfoqk3CTbqMM4/6QKVFp6Dt8sZYwBf+AK1UtT0wYNZQtMAGvju89EAzNI7wTYsfIhm2a8T9oC1DiY7KLHzPnoV0kS3MRAfX0mfWPFUbzm1DO/rN53bZs0S34ajbfpCD2q4CgIuKjoeZ NMBOj3nQ a8nHFgM7eAqhOAq+x2vj/6Pl+8TxsQzhGQeOde/iU//1lK9W84qGc5ILbM0Ze3V9h4EVqEZoCGDKtVvqazq/cOmpQP8P+Z+JL7hbcqeeNxe7ENdCNWIg0VxX4RWNaW8ONhveJcX0paz9mHNu8GcN99BS/108xQiSMKX8njgcZGqnTl3qNt+gQrS5ecSHiPW/YUrbHvVDYMpG0YqOxTT+OVzq2aPmhMIHTBp567AqlXAv/g3ZXgyNtUF4KX5TGg2bBDbEGSF1luPe0DAA5IMwKYqn9c2SZmtMD8w/vtxEwml89q/pqwRaIL69Uc9BlbeRWmgWw1JN6ONprQpCcRvMTeH6xEsdeNE+8Lxraj9KEMvnT05VZakXYY/zNQXQV76AHy6PovGftyuuPtIexKYPGgeg61eRtLZwIKHhDHxLgmHMEwP5U2PjooF4UWSQLyipq9f2vbHSqzfCC4gofX1AysAph5PqTePGvOsYVwj81vyrwx0GzuZIccSQf1gSCADPPNNCoBXIGdIXvc9yDszdD+ABX7xi1fSqAcNDuemPtv9dgdija+LS51iuZRg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.397381, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 03/05/2024 03:36, Yosry Ahmed wrote: > On Thu, May 2, 2024 at 12:02 PM Usama Arif wrote: >> Initate writeback with the below steps and check using >> memory.stat.zswpwb if zswap writeback occurred: >> 1. Allocate memory. >> 2. Reclaim memory equal to the amount that was allocated in step 1. >> This will move it into zswap. >> 3. Save current zswap usage. >> 4. Move the memory allocated in step 1 back in from zswap. >> 5. Set zswap.max to half the amount that was recorded in step 3. >> 6. Attempt to reclaim memory equal to the amount that was allocated, >> this will either trigger writeback if its enabled, or reclamation >> will fail if writeback is disabled as there isn't enough zswap >> space. >> >> Suggested-by: Nhat Pham >> Signed-off-by: Usama Arif >> --- >> tools/testing/selftests/cgroup/test_zswap.c | 125 +++++++++++++++++++- >> 1 file changed, 124 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c >> index f0e488ed90d8..cd864ab825d0 100644 >> --- a/tools/testing/selftests/cgroup/test_zswap.c >> +++ b/tools/testing/selftests/cgroup/test_zswap.c >> @@ -50,7 +50,7 @@ static int get_zswap_stored_pages(size_t *value) >> return read_int("/sys/kernel/debug/zswap/stored_pages", value); >> } >> >> -static int get_cg_wb_count(const char *cg) >> +static long get_cg_wb_count(const char *cg) >> { >> return cg_read_key_long(cg, "memory.stat", "zswpwb"); >> } >> @@ -248,6 +248,127 @@ static int test_zswapin(const char *root) >> return ret; >> } >> >> +/* >> + * Initate writeback with the following steps: >> + * 1. Allocate memory. >> + * 2. Reclaim memory equal to the amount that was allocated in step 1. >> + This will move it into zswap. >> + * 3. Save current zswap usage. >> + * 4. Move the memory allocated in step 1 back in from zswap. >> + * 5. Set zswap.max to half the amount that was recorded in step 3. >> + * 6. Attempt to reclaim memory equal to the amount that was allocated, >> + this will either trigger writeback if its enabled, or reclamation >> + will fail if writeback is disabled as there isn't enough zswap space. >> + */ >> +static int initiate_writeback(const char *cgroup, void *arg) >> +{ >> + char *test_group = arg; >> + size_t memsize = MB(4); >> + int ret = -1; >> + bool wb_enabled = cg_read_long(test_group, "memory.zswap.writeback"); >> + long zswap_usage; > Nit: Reverse christmas tree (i.e. is usually more aesthetically > pleasing, but it isn't consistently followed but up to you. You can > separate the declaration and initialization of wb_enabled to do that > if you choose to. > Thanks for the review. Will change it in next revision. >> + >> + if (cg_write(test_group, "memory.max", "8M")) >> + return ret; > Why do we need to set this? Not needed, will remove. >> + >> + /* Allocate random memory to enusre high zswap memory usage */ > typo: ensure > >> + char *mem = (char *)malloc(memsize); >> + >> + if (!mem) >> + return ret; >> + for (int i = 0; i < memsize; i++) >> + mem[i] = rand() % 128; > I am curious, what compression ratio do you observe in practice with > this? Is there a risk of pages becoming totally incompressible and > skipping zswap? One way to guarantee the page compressibility is to > add a bunch of zeros. I usually fill half of it with zeros and half of > it with random data. Not sure if this is actually required though. > With the default zswap parameters, zswap.current is 3491645, so about 1.2:1. I had tried a few different zswap parameters and compression was still taking place. I initially tried the method from allocate_bytes, but the compression was too high with all the zeros and zswap was a really small value. Will switch to your method in next revision. >> + >> + /* Try and reclaim allocated memory */ >> + if (cg_write(test_group, "memory.reclaim", "4M")) { >> + ksft_print_msg("Failed to reclaim all of the requested memory\n"); >> + goto out; >> + } >> + >> + zswap_usage = cg_read_long(test_group, "memory.zswap.current"); >> + >> + /* zswpin */ >> + for (int i = 0; i < memsize; i++) { >> + if (mem[i] < 0 || mem[i] > 127) { >> + ksft_print_msg("invalid memory\n"); >> + ret = -1; >> + } >> + } > I understand this correctness check is not strictly needed, but it is > very weak right now. There is a 50% chance that corruption will be > missed because the range we are checking is half the possible values. > > If we want a reliable correctness check, perhaps we should fill the > page with increasing known values instead. Then after zswpin, we can > check that the data is exactly what we filled in the first place. > > Is there any value in using random data here? If there is, we can > store a second copy of the array to compare against instead. So my thought over here was not to do a correctness check for zswap, just to access memory to do zswpin. Switching to the method in the comment above, we can do a correctness check as well, so will add that in next revision. >> + >> + if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2)) >> + goto out; >> + >> + /* >> + * If writeback is enabled, trying to reclaim memory now will trigger a >> + * writeback as zswap.max is half of what was needed when reclaim ran the first time. >> + * If writeback is disabled, memory reclaim will fail as zswap is limited and >> + * it can't writeback to swap. >> + */ >> + ret = cg_write(test_group, "memory.reclaim", "4M"); >> + if (!wb_enabled && ret) > Should we assert that ret is -EBUSY if !wb_enabled instead? Right now > any error code will pass. The test will also pass if reclaim succeeds > while writeback is disabled, which is not correct behavior. I believe its -EAGAIN, but yes will change it. >> + ret = 0; >> +out: >> + free(mem); >> + return ret; >> +} >> + >> +/* Test to verify the zswap writeback path */ >> +static int test_zswap_writeback(const char *root, bool wb) >> +{ >> + int ret = KSFT_FAIL; >> + char *test_group; >> + long zswpwb_before, zswpwb_after; >> + >> + test_group = cg_name(root, >> + wb ? "zswap_writeback_enabled_test" : "zswap_writeback_disabled_test"); > Why do we need different cgroup names? The tests do not run in > parallel, do they? > It makes the tests independent of each other (for e.g. if cg_destroy for any magical reason fails for one of the tests, the cgroup creation of the other test wont be affected). Plus, I found it easier for debugging to examine cgroups after the test was executed. But no strong preference, will change it to one name. >> + if (!test_group) >> + goto out; >> + if (cg_create(test_group)) >> + goto out; >> + if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0")) >> + return ret; >> + >> + zswpwb_before = get_cg_wb_count(test_group); >> + if (zswpwb_before < 0) { > Should we assert zswpwb_before == 0 instead? Sure, will do in next revision. >> + ksft_print_msg("failed to get zswpwb_before\n"); >> + goto out; >> + } >> + >> + if (cg_run(test_group, initiate_writeback, (void *) test_group)) > Nit: initiate_writeback() is not a very descriptive name because it > may or may not trigger writeback. > >> + goto out; >> + >> + /* Verify that zswap writeback occurred only if writeback was enabled */ >> + zswpwb_after = get_cg_wb_count(test_group); >> + if (wb) { >> + if (zswpwb_after <= zswpwb_before) { > If we assert zswpwb_before == 0 above, this can be simplified. Also, I > think a single condition is enough, the message contents can be > inferred by which test failed. > >> + ksft_print_msg("writeback enabled and zswpwb_after <= zswpwb_before\n"); >> + goto out; >> + } >> + } else { >> + if (zswpwb_after != zswpwb_before) { >> + ksft_print_msg("writeback disabled and zswpwb_after != zswpwb_before\n"); >> + goto out; >> + } >> + } >> + >> + ret = KSFT_PASS; >> + >> +out: >> + cg_destroy(test_group); >> + free(test_group); >> + return ret; >> +} >> + >> +static int test_zswap_writeback_enabled(const char *root) >> +{ >> + return test_zswap_writeback(root, true); >> +} >> + >> +static int test_zswap_writeback_disabled(const char *root) >> +{ >> + return test_zswap_writeback(root, false); >> +} >> + >> /* >> * When trying to store a memcg page in zswap, if the memcg hits its memory >> * limit in zswap, writeback should affect only the zswapped pages of that >> @@ -425,6 +546,8 @@ struct zswap_test { >> T(test_zswap_usage), >> T(test_swapin_nozswap), >> T(test_zswapin), >> + T(test_zswap_writeback_enabled), >> + T(test_zswap_writeback_disabled), >> T(test_no_kmem_bypass), >> T(test_no_invasive_cgroup_shrink), >> }; >> -- >> 2.43.0 >>