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 B0DC1C52D7C for ; Thu, 22 Aug 2024 17:47:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3077B80047; Thu, 22 Aug 2024 13:47:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2B6D880044; Thu, 22 Aug 2024 13:47:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 17F1680047; Thu, 22 Aug 2024 13:47:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id EEC0C80044 for ; Thu, 22 Aug 2024 13:47:47 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 70044818E8 for ; Thu, 22 Aug 2024 17:47:47 +0000 (UTC) X-FDA: 82480614174.16.3EFBB50 Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by imf14.hostedemail.com (Postfix) with ESMTP id 76F69100029 for ; Thu, 22 Aug 2024 17:47:45 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="Whso/TKr"; spf=pass (imf14.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724348824; a=rsa-sha256; cv=none; b=lRQAOEbwXSXf1qC8ACqpvqrdm5Uh2dnEAs7jqUQUfl+AYkovCBgwGBuhdgJJYQqva3qsue XtnK8ydcaGLbtRq5P8Txb5MDcUa6OeOcmVRZ0QKS6htGJ13JohfcPnfyYU/wvawb888tBO uwNmmB8hqHAseIqAHzrMNKUTmoXGKeo= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="Whso/TKr"; spf=pass (imf14.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@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=1724348824; 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=+d+/3JN2j+IR6X0GnobgQBaQc+2mwLkx4YG52rHVeyw=; b=e9VCSmrhivlIERVSASB+WQapjcBOsOSnVtYJcqAbd3yBfOx0hofYzRjuVlJVOhT//2nrXZ b0jV8Z3EbeK+KQh5I9cxxOliLGPuGUrymM8aEUzMneAp3lum8P9kmAzqq9nJeizZWzxQDk 6oLwxvIynkV3NcWZvwO4iim1WruDv9A= Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-a86883231b4so146604166b.3 for ; Thu, 22 Aug 2024 10:47:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1724348864; x=1724953664; 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=+d+/3JN2j+IR6X0GnobgQBaQc+2mwLkx4YG52rHVeyw=; b=Whso/TKrcipwIKD4MrRn59rLtAvdLCxF/8plz7gfGES0lbEzn31oUreMSANH1LWN4G 2M2R31cersIFs7OFkpugUbbKO0rcGRHZpW2Bkz5wVj8Tfy+M0vqUZ3YRYRZ3b7vfUSXV rHrJ/XSXAVsyyULzaAQOnGItrDPCX114UzkgLbhSHO2TW0qCTs+Cy6ABHwEjrQNlmVoS a2UCzAMiZl1T0QmYCSR4JZunyGnqDPIA5fhv5YZbM2ujndr9ckRro+0y2goIXT5uPjSA N37Ov9Ut2kdeL6eAFNv0lSctHEfvh3Bo7LoX0g4UCx+xl8zStzvTJI15e255nRcjLssa poqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724348864; x=1724953664; 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=+d+/3JN2j+IR6X0GnobgQBaQc+2mwLkx4YG52rHVeyw=; b=MaknWhgrERjvie0hNSRCJTyvq88mZjCwW30NLWxlWuax7KA5a+qZuO7Ze1sQQ/1aAk Ztv03ivuR1xwvLfZMV5N2Na8u/ozE9/qQ1sZRDWMB1RuSOeehvT2kPWH41uBV9azRInV +fWvRwL7MpraDP/Tw6mjWyI9iFJg7fUL1Me2vkwTEaHDpPoenZIkJzky78qjOlDEEjiO uWBMbSKuu8SV3O6BWWVj/9eskoR7P1QaS9thdoE7Qujzs9mw29D3tcoVoveujCoTbaIb 78cuO2a8UtSrQ8MHgI5lJbMsuqxn9rwxlIwSZoZbFvZrPMB/eep0NqRJP7DDPbLSUi/a detg== X-Forwarded-Encrypted: i=1; AJvYcCUgxOYNkc7QIINUmxhgiCb9fl88j8/n0l3LqkhimG1y1VXZd+VDcHqv/jcCAd0UltEOmTgEzjmaBQ==@kvack.org X-Gm-Message-State: AOJu0Yxyq5SAdfcX5xHCyt0e50ARG3L8QraolixUgtVVL3u5DZU7K+V9 ox0lO9KIU9OEcwUlUF705Ygpozhkji1AopDuy0CZoaJj8OhAEAAg700R4ZjCAuuetTZmjf9y9qL D99YKvX1rKYRG0O0SSrtXOcL+vm3DrPGT1nhw X-Google-Smtp-Source: AGHT+IEIB/sAcCVMEpN3z08j+zbYb8nUxKknSRWyl3cnas6NGtB+MeRmDWhfLCACOfDLHCFajCUgiqMgcKAl/lGIRG8= X-Received: by 2002:a17:907:5cb:b0:a86:700f:93c0 with SMTP id a640c23a62f3a-a8691b64e4emr280264866b.35.1724348863010; Thu, 22 Aug 2024 10:47:43 -0700 (PDT) MIME-Version: 1.0 References: <20240816144344.18135-1-me@yhndnzj.com> <20240816144344.18135-2-me@yhndnzj.com> <9e39ed273b5e25e3d2f33c5a2b8e3131efbfcedc.camel@yhndnzj.com> In-Reply-To: <9e39ed273b5e25e3d2f33c5a2b8e3131efbfcedc.camel@yhndnzj.com> From: Yosry Ahmed Date: Thu, 22 Aug 2024 10:47:05 -0700 Message-ID: Subject: Re: [PATCH v2 2/2] selftests: test_zswap: add test for hierarchical zswap.writeback To: Mike Yuan Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, cgroups@vger.kernel.org, Nhat Pham , Johannes Weiner , Andrew Morton , Muchun Song , Shakeel Butt , Roman Gushchin , Michal Hocko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: dwb8yqxgtsnudzrdwpyt1yh5c5igd7r4 X-Rspamd-Queue-Id: 76F69100029 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1724348865-206425 X-HE-Meta: U2FsdGVkX1+HkmYVVXYMpSeFjxT33r2nMMo6upQdOekz/09BDmRRsfOUFbg2MuEIgoVU2m7GlLAFlQsBTGwW6DsR/shdP/R+OandAZYp/Kk8rGnRki37eQiHtmN+953/8En+vTxxBE8YfesferCYe7redpEu4ZNbt1I5Jl42QU19K4I0AOTjgsAhNk+L3W+idilFs1o3F0XkJ7JvtGFNMXFL/tzi/wk0oRt0l70E9BxQm3moLlKNZ1FWUJqOPYatBV2x/cJc2GVXj5pEdb8I3ZMXxa6ukJ3kautxPlVBuYaGHogMx18LQdZcjD5Fdh9cOgfHnsL1Yvbn8GvG/hGAihKFaIPLce4brBmiYQuSUOY05G0AJ1s5A57QDXHIAXDW2uFs3GLVNJRMc9etmVnrMJdsBVEUObUeUkFGuL8I5FthivELkUtOLSu4NFrf/olshgQR5APFFgPBwUB4arlLSxdzmDvfd9h2k7Mx+CnwmojEigNZS2f4sUHze5FdAlkMTj3rsDQs16Qs64zVwDUeTgdKrEHdJmitq/QXUejmxwLHT3XKVMrGqFj1CwWHPqISWWFuAXzFwkAgJPk5yk9kAxk2cOqi9WMWy1XyGL1UzVZvxlL3p227gfcEKpeFIbIth3fldL/VsH/jU1hMFpw9N2YW5iduj/7muBVJmDFvOw7MeCMBurBKh7rACH+6YabEQx6t2ioq4KzPA6rVtu46TyKYhpw6Wx2RzpbZ73JqX1FGqk4QAnuRw4JJqFG/pwY7hNGJ8XimPdMWya3MhyQmQ2TrHRP1L6jG3iPyqU27nwf+02El2l+udAK1aDeLRcI5tseTGfLAJvP+fmq9g4w+N8ikt1M5rM70AAf0q95lPqeTNhdt7VbjAV4IADd6HhYwh1BCi8CGFlY6TX7M0Tf6djc8zojuLWLw5tBkFZiqTEkrrtd8vmLcR01cNbbddROw1kUHpD3+Z9Mf4MLHUsR vCsbFci9 r2s30nly/KQptloS0vE3uXe6cZxx1Sm/fnOt1EcN8J7QyRLkqn7gg1gO2E36DR5DiIBkLNQ45LR8iKuHTHzJoKJNlIXGzWxQWcB1DXCTUztKbkof+6z8cPRIXj0PiueTxkGVYpFTjEqHGUZrx/0b/55fsubAKwNddnPWfVPaBXgtCCyfJSLValOOhAT1gRymbzIZSXXDNoAjIIBoIN8KwcHQ6QKtP3jYo+FtXui/lhjF1Ka+MqGqXhDlcYwXWrmHyQYpgIkWjrsfvh0fu2e+oLnMHj9AFvfueZgFgWhQwwNH9Yu5JVJExQXf+AQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000567, 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 Tue, Aug 20, 2024 at 2:44=E2=80=AFAM Mike Yuan wrote: > > On 2024-08-19 at 12:19 -0700, Yosry Ahmed wrote: > > On Fri, Aug 16, 2024 at 7:44=E2=80=AFAM Mike Yuan wrot= e: > > > > > > Ensure that zswap.writeback check goes up the cgroup tree. > > > > Too concise :) Perhaps a little bit of description of what you are > > doing would be helpful. > > The patch has been merged into mm-unstable tree. Do I need to > send a v3 to resolve the comments? You can send a new version and Andrew usually replaces them. If the changes are too trivial sometimes Andrew is nice enough to make amendments directly :) > > > > > > > Signed-off-by: Mike Yuan > > > --- > > > tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++--- > > > ---- > > > 1 file changed, 48 insertions(+), 21 deletions(-) > > > > > > diff --git a/tools/testing/selftests/cgroup/test_zswap.c > > > b/tools/testing/selftests/cgroup/test_zswap.c > > > index 190096017f80..7da6f9dc1066 100644 > > > --- a/tools/testing/selftests/cgroup/test_zswap.c > > > +++ b/tools/testing/selftests/cgroup/test_zswap.c > > > @@ -263,15 +263,13 @@ static int test_zswapin(const char *root) > > > static int attempt_writeback(const char *cgroup, void *arg) > > > { > > > long pagesize =3D sysconf(_SC_PAGESIZE); > > > - char *test_group =3D arg; > > > size_t memsize =3D MB(4); > > > char buf[pagesize]; > > > long zswap_usage; > > > - bool wb_enabled; > > > + bool wb_enabled =3D *(bool *) arg; > > > int ret =3D -1; > > > char *mem; > > > > > > - wb_enabled =3D cg_read_long(test_group, > > > "memory.zswap.writeback"); > > > mem =3D (char *)malloc(memsize); > > > if (!mem) > > > return ret; > > > @@ -288,12 +286,12 @@ static int attempt_writeback(const char > > > *cgroup, void *arg) > > > memcpy(&mem[i], buf, pagesize); > > > > > > /* Try and reclaim allocated memory */ > > > - if (cg_write_numeric(test_group, "memory.reclaim", > > > memsize)) { > > > + if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) { > > > ksft_print_msg("Failed to reclaim all of the > > > requested memory\n"); > > > goto out; > > > } > > > > > > - zswap_usage =3D cg_read_long(test_group, > > > "memory.zswap.current"); > > > + zswap_usage =3D cg_read_long(cgroup, "memory.zswap.current"); > > > > > > /* zswpin */ > > > for (int i =3D 0; i < memsize; i +=3D pagesize) { > > > @@ -303,7 +301,7 @@ static int attempt_writeback(const char > > > *cgroup, void *arg) > > > } > > > } > > > > > > - if (cg_write_numeric(test_group, "memory.zswap.max", > > > zswap_usage/2)) > > > + if (cg_write_numeric(cgroup, "memory.zswap.max", > > > zswap_usage/2)) > > > goto out; > > > > > > /* > > > @@ -312,7 +310,7 @@ static int attempt_writeback(const char > > > *cgroup, void *arg) > > > * If writeback is disabled, memory reclaim will fail as > > > zswap is limited and > > > * it can't writeback to swap. > > > */ > > > - ret =3D cg_write_numeric(test_group, "memory.reclaim", > > > memsize); > > > + ret =3D cg_write_numeric(cgroup, "memory.reclaim", memsize); > > > if (!wb_enabled) > > > ret =3D (ret =3D=3D -EAGAIN) ? 0 : -1; > > > > > > @@ -321,12 +319,38 @@ static int attempt_writeback(const char > > > *cgroup, void *arg) > > > return ret; > > > } > > > > > > +static int test_zswap_writeback_one(const char *cgroup, bool wb) > > > +{ > > > + long zswpwb_before, zswpwb_after; > > > + > > > + zswpwb_before =3D get_cg_wb_count(cgroup); > > > + if (zswpwb_before !=3D 0) { > > > + ksft_print_msg("zswpwb_before =3D %ld instead of > > > 0\n", zswpwb_before); > > > + return -1; > > > + } > > > + > > > + if (cg_run(cgroup, attempt_writeback, (void *) &wb)) > > > + return -1; > > > + > > > + /* Verify that zswap writeback occurred only if writeback > > > was enabled */ > > > + zswpwb_after =3D get_cg_wb_count(cgroup); > > > + if (zswpwb_after < 0) > > > + return -1; > > > + > > > + if (wb !=3D !!zswpwb_after) { > > > + ksft_print_msg("zswpwb_after is %ld while wb is > > > %s", > > > + zswpwb_after, wb ? "enabled" : > > > "disabled"); > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /* Test to verify the zswap writeback path */ > > > static int test_zswap_writeback(const char *root, bool wb) > > > { > > > - long zswpwb_before, zswpwb_after; > > > int ret =3D KSFT_FAIL; > > > - char *test_group; > > > + char *test_group, *test_group_child =3D NULL; > > > > > > test_group =3D cg_name(root, "zswap_writeback_test"); > > > if (!test_group) > > > @@ -336,29 +360,32 @@ static int test_zswap_writeback(const char > > > *root, bool wb) > > > if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" > > > : "0")) > > > goto out; > > > > > > - zswpwb_before =3D get_cg_wb_count(test_group); > > > - if (zswpwb_before !=3D 0) { > > > - ksft_print_msg("zswpwb_before =3D %ld instead of > > > 0\n", zswpwb_before); > > > + if (test_zswap_writeback_one(test_group, wb)) > > > goto out; > > > - } > > > > > > - if (cg_run(test_group, attempt_writeback, (void *) > > > test_group)) > > > + if (cg_write(test_group, "memory.zswap.max", "max")) > > > + goto out; > > > > Why is this needed? Isn't this the default value? > > attempt_writeback() would modify it. Oh yes, missed that. > > > > + if (cg_write(test_group, "cgroup.subtree_control", > > > "+memory")) > > > goto out; > > > > > > - /* Verify that zswap writeback occurred only if writeback > > > was enabled */ > > > - zswpwb_after =3D get_cg_wb_count(test_group); > > > - if (zswpwb_after < 0) > > > + test_group_child =3D cg_name(test_group, > > > "zswap_writeback_test_child"); > > > + if (!test_group_child) > > > + goto out; > > > + if (cg_create(test_group_child)) > > > + goto out; > > > > I'd rather have all the hierarchy setup at the beginning of the test, > > before the actual test logic. I don't feel strongly about it though. > > > > > + if (cg_write(test_group_child, "memory.zswap.writeback", > > > "1")) > > > goto out; > > > > Is the idea here that we always hardcode the child's zswap.writeback > > to 1, and the parent's zswap.writeback changes from 0 to 1, and we > > check that the parent's value is what matters? > > I think we need a comment here. > > Yes, indeed. > > > TBH, I expected a separate test that checks different combinations of > > parent and child values (e.g. also verifies that if the parent is > > enabled but child is disabled, writeback is disabled). > > That's (implicitly) covered by the test itself IIUC? The parent cgroup > here is in turn the child of root cgroup. This assumes that the root has zswap writeback enabled, but that's a fair assumption as otherwise all the writeback tests will fail. TBH I'd prefer a standalone test rather than these implicitly tested scenar= ios.