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 A686EC46CD2 for ; Tue, 30 Jan 2024 18:31:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 17ADE6B0075; Tue, 30 Jan 2024 13:31:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 12AE36B0096; Tue, 30 Jan 2024 13:31:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 019B06B0099; Tue, 30 Jan 2024 13:31:38 -0500 (EST) 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 E62FA6B0096 for ; Tue, 30 Jan 2024 13:31:38 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 8C5081207A9 for ; Tue, 30 Jan 2024 18:31:38 +0000 (UTC) X-FDA: 81736820676.08.A4E4ADD Received: from mail-io1-f48.google.com (mail-io1-f48.google.com [209.85.166.48]) by imf05.hostedemail.com (Postfix) with ESMTP id D42A910000F for ; Tue, 30 Jan 2024 18:31:36 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=a6DJLQl1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.48 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706639496; a=rsa-sha256; cv=none; b=juGY+LrwcA/a9Bzrvjlj182cVnMStnaKzL2jJMoR6yolSp5HFbhIzY3RZTkus5aYICeDHD 859IHMIMyjTFbaaVGtdI9PF9/rEneRETzHrtkUMi6t/ATKsnPrGB3lpfiHxDXK/uBg+J0n 5utoNU0MhD2ngPVSb/ySrNE8w19fxvs= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=a6DJLQl1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.48 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706639496; 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=w4BTRkkumRWS7a29JYn1bsYyn8iQweTdqX0+5Jq8L/I=; b=FDPM6J7FAGDVqrV4xU8FhY8g1QwOTavfk4lfVTSjOTclrY9I3WGa/QOBbuRQjwviVXU16A IPR1t/fnzRlVGagCgWYPxjPlVGmgDF3zEXz9pYrNrkMLYfudSpc/fTp44j2O+HTpkYFeow ZJeEJ5C8juZ1IxBTR0Jkpg6w2OPW+jA= Received: by mail-io1-f48.google.com with SMTP id ca18e2360f4ac-7bfeacc32d2so114535739f.2 for ; Tue, 30 Jan 2024 10:31:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1706639496; x=1707244296; 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=w4BTRkkumRWS7a29JYn1bsYyn8iQweTdqX0+5Jq8L/I=; b=a6DJLQl1wIqhGfD5FTra0trw5P0kIGj31kPxQnwH9WCFzP09wjVJO19Wytn4Elarzn eftXhjNcYF4xjUVvAEBPTZn9wlQOO4lTk3KybdVkOdpc3Biq39JQW6sYDxBasgSNvVr0 LzqrbFDnlMDLhVU6wYCT3KzowKYj6nuKg2r/FQrmX7XGPmrtaQKGWMnUBF4ShBrYF8GE RmpCRDlr1w/EFTjF8hE2pHTZ9AogI/YsVJfiPAnA2arqO/cipKRQbvd2aNnoLDK4aq6h Od2LP+LePVbYG2l2BZr0AO9anQTV5kCwCtnZfuo7L4OMZeJi1qnTBw9mETXhC+Zskutz Eqcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706639496; x=1707244296; 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=w4BTRkkumRWS7a29JYn1bsYyn8iQweTdqX0+5Jq8L/I=; b=c7Cg7g+gsQg+CyE6qPPnWYlf10ZHBFDdnMHFM3r8Nh7oaM+qBkkyn+BKtuJxJGYmiH urRdaloT3MNZ7QeR8tqG0h+kZqIOd+a60sySuY9zIdRK08ELkVeZ5HSNRHyVG4VQGcDA th/jKw4I0wbI2jfe2lSny0EsevwEy/Ya7U54AMk6J+tZO1g/A7EAohuZ3TYW7c5fe3ML 6ul9OxOakon07ZcBEihiBNXphdRMwH6NEvxuSNGpb+FCIju7mEdapSmSW560nWPip2+g 5OmTgygTDKYz9MtVZ+Lb0g7ZBgxIZzPNM/mq14BVKT2l6BtXM/GQkrKtXA5dGZdUlk5G btfg== X-Gm-Message-State: AOJu0Yww8OPcUx6Hw4LAPvFp/G3VXlIb3Nd00SBfZkNUhNHxa9R7mSdR L4KxOJ1aZX/6Yj7LnRXJRruCo3tArW+Otmkb7VQqdPvrOKxEvssh+jsKTeeIVyn7aVWA7OJ8f04 TW+ZfR30p4cePlAkIoAq06rN4ebE= X-Google-Smtp-Source: AGHT+IGhQVQ3vc88KQYAMP/He0t9hm5odiWA9YrIXE2nGn/Cmcw+/qEXsVaCQBpT8FK5MgL1cc2PvU8T/ZJi5gRZ+sc= X-Received: by 2002:a05:6602:2f14:b0:7be:d961:6b0a with SMTP id q20-20020a0566022f1400b007bed9616b0amr14660719iow.18.1706639495901; Tue, 30 Jan 2024 10:31:35 -0800 (PST) MIME-Version: 1.0 References: <20240129224542.162599-1-nphamcs@gmail.com> <20240129224542.162599-4-nphamcs@gmail.com> In-Reply-To: From: Nhat Pham Date: Tue, 30 Jan 2024 10:31:24 -0800 Message-ID: Subject: Re: [PATCH 3/3] selftests: add test for zswapin To: Yosry Ahmed Cc: akpm@linux-foundation.org, shuah@kernel.org, hannes@cmpxchg.org, tj@kernel.org, lizefan.x@bytedance.com, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-kselftest@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: D42A910000F X-Stat-Signature: jyys968bhd1ayzt4x8daiu59omfk5mb5 X-HE-Tag: 1706639496-349963 X-HE-Meta: U2FsdGVkX18jZ8u9p8tDIXbIygtvk+sbElAKT6HiHHz1xq82bcJIFUBI5i/Zw6GdDPbQ8eE9eBYhKM2vBtPpbrC4505EYzQqVUmkCMJxQvs+7glcGyhNv1Tc+YL/bNs5hxFfHJzgxoboBl6TIieYemG1un04dpXi+MYyFSrBg1VTmbd3TAo4rI2ne+Jg/tFa+R0QJOwqh3f9tRx7qRigkimlHoLPOF2mLvAa6DPx8cyKOhozmVPRm/tDFosbej4ySaNJl33Uj0YZIQNhVpmHgWFs79q+H8aDw/GCtrKgtMnEkA6J0QRPwGaYA90zSFpwqJtse9sp76GG5PVNPmH9DL/3BnOEUdeBROpH3Uztwgazn5mfZCNZmbOQtaWFt25RMGu+m6xvha8BW2HWzHC0vzlRUDPg/wDH0f+jVnionHCvprGlqosf+b4iuxxKP6KdkD3c2DBtCQaJAAgRwBp8q0SNtFSiF8/SpGxiwM9LHQf+KvVhrKcDSEQW9uLz1PzyT/KYbh+nahxMRaZeryTVfTZhwYN/JKEUCtGejggtZnNuNphepMySOiLrlhYDraVNLbsVzSpTc2gV2KLTqySVOAfehA0WO48VQtLu2kA1WMy9noFFtz1G6mXRRAjF9nRKC+er17mnuptXc01We83MIa6i4cdZgZNgRPF1IuDzags8EPgTsHqLgQQYiE9dULZmvg7ExoV3rRfrgUcbuOm/pSxbnmOk/EgfKsTUSHHcc1xVOGGSdFXF/e3yAfmS+CSgYj9uIkX/y2jBQqzNE0cTXqQJZJ1TRMP2PKl6Zne/NsVMThgXB4wMEG8o98lmCeBzzDhiC6TYWRSxiAwT8mnQIrVhwEyITFTN6Wfft2aDjlUBS3wKnYSB5YWRWKJuNR17p/ZKb4JTwT//sE0VSZLiNbqSf2grLl/r0yxPNmC7ecpWOPeO12zzaieztcg9T1Ay1Br/9d8lPNiyKejsKC1 0L+58Y6N oeQWwNecH748LGMbyfTjdbHMu1xoSJQe9CbHvzszzhp/A8r/AyLVB47dVsGjRCOMQd8GKwYIz7tAMUBMLIhxHVfvPEOeSqlwZjnECUWj8xPD/rSu53wpD8jt2JJMytR8PssrUrGylFKMuuXXujNMcl5x2r+RdN2c7EG9FbV7pvtOyg15CFXl/Ger1zi6t1v1yUFzBeI3hzxwxwdVf6TZ4Oh9GemxfTIh5qOsKXk5eMhcwSXheouBr7ywSvgFoOgaU7yIfmT4lRjRyhKI55nCnG8jDuDScodR/BQEhqtK8OHSJTbiSwc97EF0OfgpUm+rUXNlhJ6GLrSN9CLwFU/+G6Csb2/qGAnU/1LWbuavHBtx7LzUJpqRUol97DQanoxCKVbyxUeYdVSNX/l2u3v5EOBnyYvx4O6QuzUuHUaoeChjKzssAlYMHCx2oRMLcW+06xPQRoCZOjKXP17k= X-Bogosity: Ham, tests=bogofilter, spamicity=0.418490, 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 Mon, Jan 29, 2024 at 5:24=E2=80=AFPM Yosry Ahmed = wrote: > > On Mon, Jan 29, 2024 at 02:45:42PM -0800, Nhat Pham wrote: > > We recently encountered a kernel crash on the zswapin path in our > > internal kernel, which went undetected because of a lack of test > > coverage for this path. Add a selftest to cover this code path, > > allocating more memories than the cgroup limit to trigger > > s/memories/memory > > > swapout/zswapout, then reading the pages back in memories several times= . > > > > Also add a variant of this test that runs with zswap disabled, to verif= y > > swapin correctness as well. > > > > Suggested-by: Rik van Riel > > Signed-off-by: Nhat Pham > > --- > > tools/testing/selftests/cgroup/test_zswap.c | 67 ++++++++++++++++++++- > > 1 file changed, 65 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testin= g/selftests/cgroup/test_zswap.c > > index 32ce975b21d1..86231c86dc89 100644 > > --- a/tools/testing/selftests/cgroup/test_zswap.c > > +++ b/tools/testing/selftests/cgroup/test_zswap.c > > @@ -60,17 +60,39 @@ static long get_zswpout(const char *cgroup) > > return cg_read_key_long(cgroup, "memory.stat", "zswpout "); > > } > > > > -static int allocate_bytes(const char *cgroup, void *arg) > > +static int allocate_bytes_and_read(const char *cgroup, void *arg, bool= read) > > { > > size_t size =3D (size_t)arg; > > char *mem =3D (char *)malloc(size); > > + int ret =3D 0; > > > > if (!mem) > > return -1; > > for (int i =3D 0; i < size; i +=3D 4095) > > mem[i] =3D 'a'; > > + > > + if (read) { > > + /* cycle through the allocated memory to (z)swap in and o= ut pages */ > > + for (int t =3D 0; t < 5; t++) { > > What benefit does the iteration serve here? I would guess one iteration > is enough to swap everything in at least once, no? There might be data races etc. that might not appear in one iteration. Running multiple iterations increases the probability of these bugs cropping up. Admittedly, the same effect could, perhaps, also be achieved by running the same test multiple times, so this is not a hill I will die on :) This is just a bit more convenient - CI infra often runs these tests once every time a new kernel is built. > > > + for (int i =3D 0; i < size; i +=3D 4095) { > > + if (mem[i] !=3D 'a') > > + ret =3D -1; > > + } > > + } > > + } > > + > > free(mem); > > - return 0; > > + return ret; > > +} > > + > > +static int allocate_bytes(const char *cgroup, void *arg) > > +{ > > + return allocate_bytes_and_read(cgroup, arg, false); > > +} > > + > > +static int read_bytes(const char *cgroup, void *arg) > > +{ > > + return allocate_bytes_and_read(cgroup, arg, true); > > } > > I don't like how we reuse allocate_bytes_and_read(), we are not saving > much. Let's keep allocate_bytes() as-is and add a separate helper. Also, > I think allocate_and_read_bytes() is easier to read. Fair! > > > > > static char *setup_test_group_1M(const char *root, const char *name) > > @@ -133,6 +155,45 @@ static int test_zswap_usage(const char *root) > > return ret; > > } > > > > +/* Simple test to verify the (z)swapin code paths */ > > +static int test_zswapin_size(const char *root, char *zswap_size) > > +{ > > + int ret =3D KSFT_FAIL; > > + char *test_group; > > + > > + /* Set up */ > > + test_group =3D cg_name(root, "zswapin_test"); > > + if (!test_group) > > + goto out; > > + if (cg_create(test_group)) > > + goto out; > > + if (cg_write(test_group, "memory.max", "8M")) > > + goto out; > > + if (cg_write(test_group, "memory.zswap.max", zswap_size)) > > + goto out; > > + > > + /* Allocate and read more than memory.max to trigger (z)swap in *= / > > + if (cg_run(test_group, read_bytes, (void *)MB(32))) > > + goto out; > > + > > + ret =3D KSFT_PASS; > > + > > +out: > > + cg_destroy(test_group); > > + free(test_group); > > + return ret; > > +} > > + > > +static int test_swapin(const char *root) > > +{ > > + return test_zswapin_size(root, "0"); > > +} > > Why are we testing the no zswap case? I am all for testing but it seems > out of scope here. It would have been understandable if we are testing > memory.zswap.max itself, but we are not doing that. Eh it's just by convenience. We already have the workload - any test for zswap can pretty much be turned into a test for swap by disabling zswap (and enabling swap), so I was trying to kill two birds with one stone and cover a bit more of the codebase. > > FWIW, I think the tests here should really be separated from cgroup > tests, but I understand why they were added here. There is a lot of > testing for memcg interface and control for zswap, and a lot of nice > helpers present. Yeah FWIW, I agree :) I wonder if there's an easy way to inherit helpers from one test suite to another. Some sort of kselftest dependency? Or maybe move these cgroup helpers up the hierarchy (so that it can be shared by multiple selftest suites). >