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 1CC18C4345F for ; Fri, 3 May 2024 02:37:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4E7696B008C; Thu, 2 May 2024 22:37:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 496DC6B0093; Thu, 2 May 2024 22:37:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 35E756B0095; Thu, 2 May 2024 22:37:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 134CF6B008C for ; Thu, 2 May 2024 22:37:41 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 8767D16031F for ; Fri, 3 May 2024 02:37:40 +0000 (UTC) X-FDA: 82075523880.18.CFEC11A Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) by imf11.hostedemail.com (Postfix) with ESMTP id A799B40003 for ; Fri, 3 May 2024 02:37:38 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=eFV5mK56; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf11.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.54 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714703858; 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=XOl2gl0CKeCoR9KkGO2oMhcoDMcEE+ph32ZmwgyY4w4=; b=F8adi416StE0CF8mSt5uYh4XsdDQKieFylVnEy53tcEqS5CDvKQ0Oms+lHRFcMScsEk4t/ qkljX8HrAY0QY0WaUUz6BrKUk/nOocsONEwzg8SI3Y9X1LLMzx6rUIxhIGDVaCF2QPGX1z 5nVAR9Y8NnTG7gE6+KK00Q5ejBjKzcY= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714703858; a=rsa-sha256; cv=none; b=aLDcH2W2lsiCYJp/sa51bQUPMD4GT556tAlcar9nK2UAqV+mul26qCFOzJjMSYh46qQ74c 9SKSbkneVdwnoF2S4bfKJ4wOJH8dYYAz6v8fdZdSAOprG0K+in3YPnSB2Mv1iQU/zJw+zg iOXPzFy+ssswjLnJKMI2NdhpRIlrE+Q= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=eFV5mK56; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf11.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.54 as permitted sender) smtp.mailfrom=yosryahmed@google.com Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-51f12ccff5eso1959675e87.1 for ; Thu, 02 May 2024 19:37:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1714703857; x=1715308657; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=XOl2gl0CKeCoR9KkGO2oMhcoDMcEE+ph32ZmwgyY4w4=; b=eFV5mK56bPwV2FjuHwamyD8RUWtNswr1obgLFY3BIw8F5m32omrRlYsZUh+PoxrJYX 1v3WQC6c8rB3Bvx6fgYEwO1+rCBEMvABTzxmuHfVFjaI0h5Su7h3UQxc83AmurMyQ6yw P1n6Z9lH2WFKwKFuDaxZg8VPhO315jdcJmiviYps/PIysd0oxe6shRJRlMWs3p3+EK3O Aj4usD78rJpfXlwTbKWCXRKEQlfFy5pWiK+qlaVmsdAQeo2S7YXZNsqHz44z20Euf8lb mdFAXGf6L0lYYtr05JnOhvcZgsSw/AM+CBb6s6RsJt3MhOzYq4rYCiHZ0m7NbwBEZjGC 5HkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714703857; x=1715308657; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XOl2gl0CKeCoR9KkGO2oMhcoDMcEE+ph32ZmwgyY4w4=; b=sBOmrg/InEjJ8IenmcoqMtsFmenSaIO40hZ5XefE+bw4i5J8Ds8D0HG4q5Jmq3aZea Yzl3TnnxWVtjr9Mq+0Y38vclLTJinvdPXCfxFEyVPyN05ttlaQnGMBHIi8UOrkWgvcrQ AxFsaksJbd3VadF9z5cG1zPZCMpje2ANW4SjdpWXUVlPGClUeGuU6tMfH+s+uEMqGnTU QqBBJ5aUAQ+/A0jjl9YpisC1jI22pf9l+GIproYgficKuXigbR45YMQVCvpHBF3fxfxQ RzjYY7iwSs26T1/KuiF9AsFuS9R65EBYRVXWKMlVFeBiAafIBuwAdmiT3IPkDL7omrCR YZ1g== X-Forwarded-Encrypted: i=1; AJvYcCWTOrgB3OUkkmkBR8qEvrhjdED+gyPKfPBKk5NCTB7h527KEXSFH6kW+V4S50Mz+51xIOtZtgITiggGdVCEC2FBF5Y= X-Gm-Message-State: AOJu0Yy+14aFcVD+l/aPsxNBG9PSSgAVheAGEVfzQ6KaADSZhw4GNlOw N3i7fFh8fZhKa1ena77umxmrg+fgKTbtiEBa3xhIv/JtGPcYDJIShGkOYH3yEPEcGcUt+h5/Wz9 yijWbGbcvyJsu84KjANUpKhATq8glm/Sp7OKQ X-Google-Smtp-Source: AGHT+IGD370JRl9Z5QPqGfGyUZ/21OmfZM6h80E/KeOm6QxBNwhg85RkQYXZlxPIcfX6n8xtGAU09uxxERboVwm1xrw= X-Received: by 2002:ac2:44af:0:b0:51f:53e0:1bc4 with SMTP id c15-20020ac244af000000b0051f53e01bc4mr985926lfm.29.1714703856559; Thu, 02 May 2024 19:37:36 -0700 (PDT) MIME-Version: 1.0 References: <20240502190223.3975970-1-usamaarif642@gmail.com> <20240502190223.3975970-2-usamaarif642@gmail.com> In-Reply-To: <20240502190223.3975970-2-usamaarif642@gmail.com> From: Yosry Ahmed Date: Thu, 2 May 2024 19:36:58 -0700 Message-ID: Subject: Re: [PATCH v2 1/1] selftests: cgroup: add tests to verify the zswap writeback path To: Usama Arif 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: A799B40003 X-Stat-Signature: iyekcrdugdjcxrqx7j6s7uze3b7kdog6 X-Rspam-User: X-HE-Tag: 1714703858-155455 X-HE-Meta: U2FsdGVkX1+Sn+6r+9/MMCElVzIIJiGMawupLUyGGHbLoRyIMXHhOL+E8DGvDeMjuCQV6mIezGBVw66JXR/vLthm+3qEWVIPG5kJKMJ3HT3rMBbH0vOZI6fiMy/lmuWSYVSSZYDJRgGfDe14aw7mDBBLW1xRJXV8cn337dw01V7kKYA1vkdVd6qOPG6RYKPjFU2BkQa0Y65YXiM4GzYNnvW5udJeEoU48o3APPlC7mVxoOdo/Dwv9eS/LBvLRFt9UeAoFSi/Wqdpdtk9UWotPsUfD0F4FpPVqkd7fFe2+HI55fzf3DwD1Hw6UCH71DXXX8FJf63wYBi/OT+OBfWAwPp6IfQsiIxUH1gPvJc4rDBzMrg2i1aXUU9hhXqEbg49jQxUJsSw4M4rsyBmLg66Wd+zvLccdt+xtvATSgyjcip4CI9DdSfrkq+RqTqHXIURzecp758YVSd1ZG/Uq6AU3+Nc4onut+kA3XJ9lcYOgjakVUZb6uaA1uwXAXnoUoBUcCCt2w0yKF60geQ8xSnYnjNG6+1pG2k/PdiT8C65/25bAPW4BTZYyqs1Fikildvc5blC/W7declBZdR74XeG42MU6A/4MCqvOJJpt3gTZU7agqIIpgXhtrB48yjx3MFNm3PC/hYgAnEn6BWkSrSxjtmOfVF/yF70l4R2YpeBpLsYLyVkoxjkDiFSRLBF59sSvxt93iaiUJUGI1CTw7ALIIJ1tfQVsnfPWnEm3OotpLIgVtft7ic/IQCVIJBWa6f5wcSGzO6HwLBCn+h7eA4veph9CWc/xm6di9hwfPJ/8sQNYaDf01P84HWb24w4DEFbskt6EIXVcba9MIt/bBGeuDawnzrv7OoYlAVRnhSx7yVQvtbT6vZNXJPQi2Fsr9XASt2UVPxQwxefbqpHBQLC9DynmjDcC9X4Llvsr7owggy/r4yQdS1YF+nEyuHPTukxxwJ71IYpDCfuMLj3bWr EfPOV1Wg a0sZeYQ3ik7Hy2Xt1LdgbYxSarrlr2UwohvYzP/pyFJYqWCn6DONjd1Ki32RlvOxcNRNQwkhB4VE8OFa5v1dPap5r9KB8Ax3SjfRWouoftUm7RTLbQIhe8qDmFgp7cYQ+90uOyOIXYuapio3P09KsvSzOBWP22ch6nwD/ssL1PAhNWD10pQSFEocxog+ZC0NqSmzewDUkgi62kePfSvxW5qEO9NYAEqXZwkB/RLSC0tnE3EpmxnRZF9UA7w== X-Bogosity: Ham, tests=bogofilter, spamicity=0.428742, 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 Thu, May 2, 2024 at 12:02=E2=80=AFPM 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 spa= ce. > + */ > +static int initiate_writeback(const char *cgroup, void *arg) > +{ > + char *test_group =3D arg; > + size_t memsize =3D MB(4); > + int ret =3D -1; > + bool wb_enabled =3D cg_read_long(test_group, "memory.zswap.writeb= ack"); > + 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. > + > + if (cg_write(test_group, "memory.max", "8M")) > + return ret; Why do we need to set this? > + > + /* Allocate random memory to enusre high zswap memory usage */ typo: ensure > + char *mem =3D (char *)malloc(memsize); > + > + if (!mem) > + return ret; > + for (int i =3D 0; i < memsize; i++) > + mem[i] =3D 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. > + > + /* Try and reclaim allocated memory */ > + if (cg_write(test_group, "memory.reclaim", "4M")) { > + ksft_print_msg("Failed to reclaim all of the requested me= mory\n"); > + goto out; > + } > + > + zswap_usage =3D cg_read_long(test_group, "memory.zswap.current"); > + > + /* zswpin */ > + for (int i =3D 0; i < memsize; i++) { > + if (mem[i] < 0 || mem[i] > 127) { > + ksft_print_msg("invalid memory\n"); > + ret =3D -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. > + > + if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/= 2)) > + goto out; > + > + /* > + * If writeback is enabled, trying to reclaim memory now will tri= gger 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 =3D 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. > + ret =3D 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 =3D KSFT_FAIL; > + char *test_group; > + long zswpwb_before, zswpwb_after; > + > + test_group =3D cg_name(root, > + wb ? "zswap_writeback_enabled_test" : "zswap_writeback_di= sabled_test"); Why do we need different cgroup names? The tests do not run in parallel, do they? > + 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 =3D get_cg_wb_count(test_group); > + if (zswpwb_before < 0) { Should we assert zswpwb_before =3D=3D 0 instead? > + 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 ena= bled */ > + zswpwb_after =3D get_cg_wb_count(test_group); > + if (wb) { > + if (zswpwb_after <=3D zswpwb_before) { If we assert zswpwb_before =3D=3D 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_afte= r <=3D zswpwb_before\n"); > + goto out; > + } > + } else { > + if (zswpwb_after !=3D zswpwb_before) { > + ksft_print_msg("writeback disabled and zswpwb_aft= er !=3D zswpwb_before\n"); > + goto out; > + } > + } > + > + ret =3D 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 mem= ory > * limit in zswap, writeback should affect only the zswapped pages of th= at > @@ -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 >