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 76200C47DDF for ; Thu, 1 Feb 2024 09:08:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E8DFE6B0075; Thu, 1 Feb 2024 04:08:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E3E336B0078; Thu, 1 Feb 2024 04:08:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D05586B007D; Thu, 1 Feb 2024 04:08:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id BE83E6B0075 for ; Thu, 1 Feb 2024 04:08:39 -0500 (EST) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 54ADE140DC9 for ; Thu, 1 Feb 2024 09:08:39 +0000 (UTC) X-FDA: 81742659558.27.B5AA853 Received: from mail-yw1-f202.google.com (mail-yw1-f202.google.com [209.85.128.202]) by imf08.hostedemail.com (Postfix) with ESMTP id 9831316000A for ; Thu, 1 Feb 2024 09:08:37 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=SK41Szqm; spf=pass (imf08.hostedemail.com: domain of 3lF-7ZQoKCHUrhlkrTafXWZhhZeX.Vhfebgnq-ffdoTVd.hkZ@flex--yosryahmed.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3lF-7ZQoKCHUrhlkrTafXWZhhZeX.Vhfebgnq-ffdoTVd.hkZ@flex--yosryahmed.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706778517; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=PyPRSJB8R7sstDIzxYvvLOQP9nz0Xs4tmV9kMbdp44Y=; b=dfONzekfRmhGqidkY+ntmH9PcegRtGPHFqb/3OH6ECXWiCcuFGIxPbuU7VDYZANM9qUhcp wB5kbv0zoHbaa6DnszHR29Z3GpwGA47CQfFpULt3GTnkC2QUErGyHR9NV8cw3VKhZmtfiE ptAAxYlxYYK/Hs6i30L+InokSjvyrxs= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=SK41Szqm; spf=pass (imf08.hostedemail.com: domain of 3lF-7ZQoKCHUrhlkrTafXWZhhZeX.Vhfebgnq-ffdoTVd.hkZ@flex--yosryahmed.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3lF-7ZQoKCHUrhlkrTafXWZhhZeX.Vhfebgnq-ffdoTVd.hkZ@flex--yosryahmed.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706778517; a=rsa-sha256; cv=none; b=jVTSxe/XeaEt6T/U8Wl5C4oyp5MvXmPhgkBviokfqaP1KkCbQLBI3dLncPyfgMy6OxThDQ kAxM4bDPdUu8UTmW+DJUS5WORvqTG8OHVWcmcC2ovILA+d15KmXG4fhaJvyDL5mQvyJ3mx GBVxF7YMjYbpvcpCXD1pMZwTL89e7Z4= Received: by mail-yw1-f202.google.com with SMTP id 00721157ae682-5f0fd486b9aso11893787b3.2 for ; Thu, 01 Feb 2024 01:08:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1706778516; x=1707383316; darn=kvack.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=PyPRSJB8R7sstDIzxYvvLOQP9nz0Xs4tmV9kMbdp44Y=; b=SK41SzqmIxHeNT3M3Rj9IjK4hTNS3p/ezZH1zp5NDFrioAexsH4NKlXTwMH8r+gfGU 6k/ZgwOKvWAyIhYSJWybA2uWa1fBc+9/hwjAmCU45PzSoAXASbuvliJCBShKyIAUa/m5 h74SP2FfKIznioW892iZLrtAvpS+oXUDVAPz5alTrkKSmTpcJVx19vgbkXH20bT4x87b AiSMwuLXbCIFwLupD4MCgF6VsoFwXdbpxJBRGODEctj7ytwGYM3Sdc9gBh5tU5fRneLR 5qsVrkg9x2W7X1AS7umuG6BxJTbcuDtuc5XpyKSon6RLue8deNw4gyxmKH6BlnWj0/QK 9Ztw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706778516; x=1707383316; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=PyPRSJB8R7sstDIzxYvvLOQP9nz0Xs4tmV9kMbdp44Y=; b=DJo98M9bGHOSqpVw+FwcZ1ZctqTMTzNgs8q3QJdQLx85uUCtlWGJTkjS2f93H8Z1FK S8Eo082g8RK1gbr7TQV8DiCpNvHPVN6py4sXRyJB6/xatRwYN5Ygt92kK1p6hUSzAIP6 Mzl2E4Z8qx97l9M+AiUsemY8uuhtkp1dQTQxBeZ9advzRh/BpH8WvYUjcyXV0ybtiPqq 805g5poNfYI/mUr3X4cljZ5xHjymCsvb0HogTFHXXWk/+sx49Qdq73GcRJHAP0hNEwj4 7J6CfvnBDRveAY/1f+m4kgzow/zPs556UZ7JT74m2I2k4M05QXrLWdpmG5HsqP33K64I O1ew== X-Gm-Message-State: AOJu0YxZ0eGbxCOT24aa9CibnIt9l1P2ReUrgM0v4pGVGIxpC6GKLjSk NOWjuFPQJBpu+vTd6OPJ46FsXMdL9whdctkZB+tyj5wHTUHk5dxEltqNdwsEQeRKu3X2LpH7VRH wK+d6lYNQwJxJK09dJg== X-Google-Smtp-Source: AGHT+IEal3S+0/LT8L/+ihFESpfF3v8jase7MpGggk1/LchQ6suhlyF5F50sZ3ZPPJU5OFZDNh2MSLQST8IUcSCG X-Received: from yosry.c.googlers.com ([fda3:e722:ac3:cc00:20:ed76:c0a8:29b4]) (user=yosryahmed job=sendgmr) by 2002:a05:690c:ec5:b0:5ee:2b5:7d75 with SMTP id cs5-20020a05690c0ec500b005ee02b57d75mr276209ywb.8.1706778516753; Thu, 01 Feb 2024 01:08:36 -0800 (PST) Date: Thu, 1 Feb 2024 09:08:34 +0000 In-Reply-To: <20240201032718.1968208-4-nphamcs@gmail.com> Mime-Version: 1.0 References: <20240201032718.1968208-1-nphamcs@gmail.com> <20240201032718.1968208-4-nphamcs@gmail.com> Message-ID: Subject: Re: [PATCH v2 3/3] selftests: add zswapin and no zswap tests From: Yosry Ahmed To: Nhat Pham Cc: akpm@linux-foundation.org, riel@surriel.com, shuah@kernel.org, hannes@cmpxchg.org, tj@kernel.org, lizefan.x@bytedance.com, roman.gushchin@linux.dev, 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="us-ascii" X-Rspamd-Queue-Id: 9831316000A X-Rspam-User: X-Stat-Signature: zy8ufocjco1nkon4yeimpdtkwf1ypkjy X-Rspamd-Server: rspam01 X-HE-Tag: 1706778517-606989 X-HE-Meta: U2FsdGVkX1+aasm0l9lm+PYWf0QdjkN8ETmfy7zsKGnbhtIMkZr33qnxdEEqogoAXsmIy25DjrgQsToadbVkoxk7V6EhxUtjt526ePT5ahlKLd0xpCuMH9AyIVUHvGIdXMb6F8S2DVkeyPqYCBW7nwQUxVGbKSGozNhDh+4AP7eiTZoMwVASr7TZEpIIXSwvU5SScjzPhg36/S4WctsOthlvkfQMUY7wezMLcH/E28qyoUoOGd1MRk8MVjlYFh72Fntom0MlkButlsOw17MvMczSAGFBgq32L/XRVIG/jTjR6+9jyg3bunoWDrWmovb1nO8KR77rpC8Sx4gxdXCEiGfQ/7Af8qxOmfop0AkElLXP99G/3JDNAuP9mVE2K66/m3yTLoqLzFgSgBXaRqkOmq7CsansLZeOUkItUPmpuyIs0tlGOUl0zeEVbwkPvIbg9YmqJT9tRKdCuZTNDg63a47dNkI/9jet+WnYDI/JtgeawSj1jBRda7iJqKnOVioWu8JJpWKIHw+Fkvzv7A6FbsqxbUU8A03khadUeF1jvSUUSCCJwuCtnE/+C8nF0bVKuQEfitw6SKKMMah/qQdb9nSgwErlbsG85AKzhvlbi+38mJNqgRBG7gtrMSBcEbKn5FM6A5nDRjyn2O7HfCeGCJWxKNnTQb10q8M3gUZXctwDgXi0aSKOGjX5o6GJWPv5vUKWFPmbiB+lYFNRrnRSY6pPqkCk/tL43NKT22YgOLzXjC0Un1y39T1FNYCYY1ZJ+MG0FbxA+QscJf5iBUtm45hA2rLDaiJojFQJmQbB+97PuAvtEGdMKZwlgiuoqH7T4Jwn1ZlCIChTDZj7jJNP7t3D5zgKDut/vpckZEBnu46VdLHNTZU+Mx3u5BdHAzAxPPzxB/r5y6uns81bfEXs27BGJ4OdYEiJXG0UenNFAE9hBNy80oFAAju1UpsYMzbhpkKo22UgsbHhH4l0+Pg diGb6X3o LwbdF+wkUX0OLScGtZGMIriXQt4o2cgj+Q2RKU2Sp02UhxcaXgp+aWjBUZDlp8BmUhSgzJ68qbC20d0M0PljZAcBM+C9q8rFUABOT1DADA2utMbH606f11RkdoZBfq76ZbTPMUzVT8emgS/KRvk7NZDt574aiMJZgbVLAu4iyy8Xv3786TbUDNaNafJXJtk6TBQQSD65tn/r15dGbYXrdeEMzW0RVK2zl0+473koozqcY35FNYXoWfn3B8Sky8/c72TDicOXx6O6eLI103QxV9mfHMUXKxoFETY7Ufp7tJl2AbTndTzwGFtZtuiNOd7sTBXacAcnj01IR+jOfP3Trbx8E3kC3pTbHV9UI8FdSgGpG67NHusXK/5TRy5wGmEO3kKZdILnMmFE1SugV+dt4lc5+qT9AGp5/ZfCZ0FWI1qF7hVjvLYHNigZNegNIk8K29pzfkyjudpo9542MTgL/GvZ0CGX3PHat3BwdfBgN8EIDIXdFWl9zZtX2RVV5IelZ7SY2t4wIOcNYS2/XoKogNPXMLaoUxvuyACRhsxjy5RfGwEj5FqxdWHp5HqFPBBc2xQl5XP8TY57UPxfVjpQCZnip/5D98Cju4cYwoB1xpDBCg/igi6h+nHrKPgN/LPtt3yooRM+RlWc4kiDqWkrjquaEtuDo0hAwsSdRzavN/w4WuvfofBFSrMsBWeTnPEUSL/6BFFKrabdKq0UpdaLzEzzd21VAKMgd2uXT X-Bogosity: Ham, tests=bogofilter, spamicity=0.196975, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Hey Nhat, I have a few more comments, sorry for not catching everything the first time around. Adding Roman to CC. On Wed, Jan 31, 2024 at 07:27:18PM -0800, Nhat Pham wrote: > Add a selftest to cover the zswapin code path, allocating more memory > than the cgroup limit to trigger swapout/zswapout, then reading the > pages back in memory several times. This is inspired by a recently > encountered 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 verify that when memory.zswap.max = 0, no pages can go > to the zswap pool for the cgroup. > > Suggested-by: Rik van Riel > Suggested-by: Yosry Ahmed > Signed-off-by: Nhat Pham > --- > tools/testing/selftests/cgroup/test_zswap.c | 97 +++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c > index 32ce975b21d1..14d1f18f1098 100644 > --- a/tools/testing/selftests/cgroup/test_zswap.c > +++ b/tools/testing/selftests/cgroup/test_zswap.c > @@ -60,6 +60,27 @@ static long get_zswpout(const char *cgroup) > return cg_read_key_long(cgroup, "memory.stat", "zswpout "); > } > > +static int allocate_bytes_and_read(const char *cgroup, void *arg) I think allocate_and_read_bytes() is easier to read, but I don't feel strongly about it. > +{ > + size_t size = (size_t)arg; > + char *mem = (char *)malloc(size); > + int ret = 0; > + > + if (!mem) > + return -1; > + for (int i = 0; i < size; i += 4095) > + mem[i] = 'a'; cgroup_util.h defines PAGE_SIZE, see alloc_anon() for example. On that note, alloc_anon() is awfully close to allocate_bytes() below, perhaps we should consolidate them. The only difference I see is that alloc_anon() does not check for the allocation failure, but a lot of functions in cgroup_helpers.c don't, so it seems intentional for simplification. > + > + /* go through the allocated memory to (z)swap in and out pages */ > + for (int i = 0; i < size; i += 4095) { > + if (mem[i] != 'a') > + ret = -1; > + } > + > + free(mem); > + return ret; > +} > + > static int allocate_bytes(const char *cgroup, void *arg) > { > size_t size = (size_t)arg; > @@ -133,6 +154,80 @@ static int test_zswap_usage(const char *root) > return ret; > } > > +/* > + * Check that when memory.zswap.max = 0, no pages can go to the zswap pool for > + * the cgroup. > + */ > +static int test_swapin_nozswap(const char *root) > +{ > + int ret = KSFT_FAIL; > + char *test_group; > + long zswpout; > + > + /* Set up */ I think this comment is unnecessary. > + test_group = cg_name(root, "no_zswap_test"); > + > + if (!test_group) > + goto out; > + if (cg_create(test_group)) > + goto out; > + if (cg_write(test_group, "memory.max", "8M")) > + goto out; > + /* Disable zswap */ I think this comment is unnecessary. > + if (cg_write(test_group, "memory.zswap.max", "0")) > + goto out; > + > + /* Allocate and read more than memory.max to trigger swapin */ > + if (cg_run(test_group, allocate_bytes_and_read, (void *)MB(32))) > + goto out; > + > + /* Verify that no zswap happened */ If we want to be really meticulous, we can verify that we did swap out, but not to zswap. IOW, we can check memory.swap.current or something. > + zswpout = get_zswpout(test_group); > + if (zswpout < 0) { > + ksft_print_msg("Failed to get zswpout\n"); > + goto out; > + } else if (zswpout > 0) { nit: This can be a separate if condition, I think it would be more inline with the style of separate consecutive if blocks we are following. > + ksft_print_msg( > + "Pages should not go to zswap when memory.zswap.max = 0\n"); We can probably avoid the line break with something more concise, for example: "zswapout > 0 when zswap is disabled" or "zswapout > 0 when memory.zswap.max = 0" > + goto out; > + } > + ret = KSFT_PASS; > + > +out: > + cg_destroy(test_group); > + free(test_group); > + return ret; > +} > + > +/* Simple test to verify the (z)swapin code paths */ > +static int test_zswapin_no_limit(const char *root) I think test_zswapin() is enough to be distinct from test_swapin_nozswap(). The limit is not a factor here AFAICT. > +{ > + int ret = KSFT_FAIL; > + char *test_group; > + > + /* Set up */ I think this comment is unnecessary. > + test_group = 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", "max")) > + goto out; > + > + /* Allocate and read more than memory.max to trigger (z)swap in */ > + if (cg_run(test_group, allocate_bytes_and_read, (void *)MB(32))) > + goto out; We should probably check for a positive zswapin here, no? > + > + ret = KSFT_PASS; > + > +out: > + cg_destroy(test_group); > + free(test_group); > + return ret; > +} > + > /* > * 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 > @@ -309,6 +404,8 @@ struct zswap_test { > const char *name; > } tests[] = { > T(test_zswap_usage), > + T(test_swapin_nozswap), > + T(test_zswapin_no_limit), > T(test_no_kmem_bypass), > T(test_no_invasive_cgroup_shrink), > }; > -- > 2.39.3