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 D2A1FD3C52C for ; Thu, 17 Oct 2024 19:49:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4163B6B007B; Thu, 17 Oct 2024 15:49:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3C5ED6B0082; Thu, 17 Oct 2024 15:49:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 266C96B0083; Thu, 17 Oct 2024 15:49:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 04CFF6B007B for ; Thu, 17 Oct 2024 15:49:55 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 97733C0268 for ; Thu, 17 Oct 2024 19:49:43 +0000 (UTC) X-FDA: 82684134582.04.D38C1BA Received: from mail-oa1-f45.google.com (mail-oa1-f45.google.com [209.85.160.45]) by imf22.hostedemail.com (Postfix) with ESMTP id 8739DC0009 for ; Thu, 17 Oct 2024 19:49:41 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=iEJJ2fR7; spf=pass (imf22.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.45 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729194545; 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=7XkO2r3zzUNqzUbdoVFCuN5G5HuPfrDJEkzimPq4G1w=; b=tqx3zAAK6tl7fWUQARwnOvD8FCJmjdMhFwBHWAiPX7fRIQgnWFRa+GaOoxPp/5KHG9EHvZ 7ZOyVZAtgIvz7oEAk8sTCjAbH1H+3eO/CGrx0KomYM0AbyOaC101WDELREBF0/wHc+jFwX K2r+cj7YeuvwQxXHCoo9rdgs3aw1dCc= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=iEJJ2fR7; spf=pass (imf22.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.45 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729194545; a=rsa-sha256; cv=none; b=hkDF0D4fEVSrRzolUxg+L/IyBikbR94IMTUghWsk3z/dWka2Yh2F+Ta22N5MhIraFCVNne JvypyOp8/V3JudY2wxxXKs7tQKpqSVLuL6CCU3r12RnK+aemXPeOJ8PJJHYFHRQvONXuCH 4jUBGli2AsQDtbo0KNcfNF4FpCUf/40= Received: by mail-oa1-f45.google.com with SMTP id 586e51a60fabf-288b33cbd04so80958fac.3 for ; Thu, 17 Oct 2024 12:49:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1729194593; x=1729799393; 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=7XkO2r3zzUNqzUbdoVFCuN5G5HuPfrDJEkzimPq4G1w=; b=iEJJ2fR7fD4FecmeX6tmbWsOhddRg+fvyOBbV1pNfAlIeAWBbdRQNKVIAA4bscb1SG vAURmb3cM7MhGm5i4uGDKNpWbFPHjM2PbvM5jq+rDTiqFPRDbXs6XEqgjl7ER1LqFm/P TaZPAxIzU+C7qo8segOI/8ZEmsC7BVXb0aEYQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729194593; x=1729799393; 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=7XkO2r3zzUNqzUbdoVFCuN5G5HuPfrDJEkzimPq4G1w=; b=exGxo8MaG2PuwDkgWq7JmZL2Gf1/Dy1DHnkqyfWmX1BQN3tJA08AzpwEjzJQa1lHK3 3fZjICD4PCGI2DZZRXZfELZLk1sH+WgRc8SCjplJbdkCd4SRqAkRMs1P6X9s/mxOvsVL FHRP5OnxCj+vQY7VsP9m42u1sUw5MSGuJWckfO8zJOQgv+MhOh5wkiA9wjOZJWGqABPc Vv1cukfT3XMMgD4JWUrJ/Gd8APk89iirXPULzLHY05ukHFim/mULp6nkVSMjdMa3unnR 31fWWEgRk1xeK6WjCSe97qWNw8E+VaFpZvB7J7ObtZNKVekCqwyBA0wbNTTJV8c0/lKF E40g== X-Forwarded-Encrypted: i=1; AJvYcCVMAon08UIQSETu3ayaL2VgQo/mmJ7Yt54jAMf4At9VSxZmCT7uS/JTMPCS1N2RwJs8aeu3difZqg==@kvack.org X-Gm-Message-State: AOJu0Yzn0dEhPo47OeD8JZwwsjPGsWVZIe6/ZRblMLKzEiCZeutRf3aD /XZ3alxnSoIo2W7JzMt1CYXYW8nDV15bUnzKTWtrmrrCQK9uFWVp06YGegcvrBrMoto2Cdx27Cf hh2P2IseQVkntaIvKT+gcNKE6AIXBrk7eqTRl X-Google-Smtp-Source: AGHT+IEm/koDWIdypp8c7oq3Fku1eryn80Mc4GgdMntt4UDONrJNKs6/3s0FNzVqndj5P3G6PpGYxGY+1ORX2XtKfNw= X-Received: by 2002:a05:6870:f115:b0:27c:df37:9e0c with SMTP id 586e51a60fabf-2892c3426e3mr7600fac.8.1729194592767; Thu, 17 Oct 2024 12:49:52 -0700 (PDT) MIME-Version: 1.0 References: <20240830180237.1220027-1-jeffxu@chromium.org> <20240830180237.1220027-5-jeffxu@chromium.org> <4944ce41-9fe1-4e22-8967-f6bd7eafae3f@lucifer.local> In-Reply-To: From: Jeff Xu Date: Thu, 17 Oct 2024 12:49:40 -0700 Message-ID: Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap To: Lorenzo Stoakes Cc: Muhammad Usama Anjum , akpm@linux-foundation.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org, pedro.falcato@gmail.com, willy@infradead.org, broonie@kernel.org, vbabka@suse.cz, Liam.Howlett@oracle.com, rientjes@google.com, keescook@chromium.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: eyy49tthisdbn9on57itm85ok48dxihb X-Rspamd-Queue-Id: 8739DC0009 X-Rspamd-Server: rspam11 X-HE-Tag: 1729194581-685313 X-HE-Meta: U2FsdGVkX191PRndXb/FNyUvph+QDML/OOh87P7CKaWdCbsuOhOyIgNgFSy/VR+RUHkKZVCpUfYpAGOO13rtU2l66AILZB4mHdfO5IbNVH9/ZPAqln17EdHpYnRouMhY7AOdSdo1XVA+sA2cjPuxi5KgHh8j1KXbB1iFqtbdjo3cvWVzNx/6eTC8QBvBJxPNpVqUS0Hq+3jU0THB6YF4JBDSs/PB4kXHMdkSw3K/wKtbfcG7e31cvt5TrXghpoZzE234haZtmGOKXK7v3UvxnCHKShfC5faJce48c6mrJIx3kNnzTggEBGU8wHkbcJ8jq8xj6yn5u6TF1a8fp+y0zddIF5mVJ5Fz5WQfrzSlekmDJb6030AASiFJVZdU4XSD8/yqCXNifL1KB5Pnkqv5QQFrL3wDktHiIO2PVOCDehU3y/f1RIJaOwIyXjDR+XBCWlP+67qHiaBdvE4+E1WL6jUFjC8cbaUjNbQ+tW6E1CvRNzbmqeXpns7pQxrLoVy5mvMXD18+eLjtzXEdTc/cZOzMw+3ig8hIAl/cKy9NCxbpOzg4oI0rwQVO1ec3BcMg28qi7EGLLStGP9JGVJiyS/PTaDLHFEa9c+xi9ByWj6sb+kdXY5WaUCYjL80hi+w6C5dcCMFfHXud9In/Xgsv7aRMWAyXU7xk9iXiP5eLNoPOg+kiTqoCu6gXgOJCH0XN+mvHkKeMNPpQhobAw5IcEnQuA64DcwXBzQFLqsJASEtKtppxVC9S6lc5fQR1oc99HXO5e4dtimRXvVVzcKmhqTRklU5mWsaMk91TSjzjQ+SJg0EHQ2smi97b/Bya69qQx06OnKrZVA0GlRi2tlSSWrKyOKXo42Qy4sDyR4Rqmn5dHZPRQOyIkAv7B4hDBsM+NFllf0fP6nN6SRhMLeoAt6zqQqKhW8MZo2h8EokVWPwBzYov7CAjB9tSMzVgcuh3uz0xInhXBfeE1zImaFY H0cLU6+g /Kj8yFYyByZWCfpk6UDp+xKkA0OoiW55POTKVy6gOg4pPRqrtEMbUsQYiZmfOCMOkwKiDm5TwGr6Wp2MYWUfLg5L/NEXEObEvV3D20mdrXvqN/d8ZufG1jq1iiJC/ppCSCMK+cMGO2f+LJauuuhVLZx4uZKn8/ZJSHTPaZjhJl4Ux2uO3uWegzZM+tHvwnw87j5ZOnUT+NnV7umIZgPBpdOiT3PQS3YQ0oTnINLdCpRCa/8+gB43/5YGglmTOGF2NUj+pOcqXQQP72F5eBsnujjJGABy8qXDUeRR86ac+PPWg/Ypm/YFBU6W+L/4sxbPUZni5qTgTUNsz6LD6ywAW2+jvlvR25UIRZNckLi8oAZZM3ZSugpL4z4TwNox5pwxh/aRcmFWI7bOlYf3Kj426RfKHWYj8tVlTqFgFRRY+IdCwzeA= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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, Oct 17, 2024 at 12:00=E2=80=AFPM Lorenzo Stoakes wrote: > > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote: > > On Thu, Oct 17, 2024 at 11:29=E2=80=AFAM Lorenzo Stoakes > > wrote: > > > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > > > Hi Lorenzo and Muhammad > > > > > > > > Reviving this thread since the merging window is closed and we have > > > > more time to review /work on this code in the next few weeks. > > > > > > > > On Fri, Sep 13, 2024 at 3:50=E2=80=AFPM Jeff Xu wrote: > > > > > > > > > > Hi Lorenzo > > > > > > > > > > On Sat, Sep 7, 2024 at 12:28=E2=80=AFPM Lorenzo Stoakes > > > > > wrote: > > > > > > > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at= this point > > > > > > too - I may be missing something, but I cannot for the life me = understand > > > > > > why we have to assert negations only, and other self tests do n= ot do this. > > > > > > > > > > > My most test-infra related comments comes from Muhammad Usama Anj= um > > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f33= 6b6@collabora.com/ > > > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (se= e > > > > [1] above), and I quote: > > > > "We don't want to terminate the test if one test fails because of a= ssert. We > > > > want the sub-tests to get executed in-dependent of other tests. > > > > > > > > ksft_test_result(condition, fmt, ...); > > > > ksft_test_result_pass(fmt, ...);" > > > > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > > > replacement of assert. > > > > > > > > Please let me know if you have questions on this and Muhammad might > > > > also help to clarify the requirement if needed. > > > > > > > > Thanks > > > > -Jeff > > > > > > Right this is about not failing the test i.e. equivalent of an expect > > > rather than an assert, which makes sense. > > > > > > What I'm saying is we should have something more like > > > > > > EXPECT_TRUE() > > > EXPECT_FALSE() > > > > > > etc. > > > > > > Which would avoid these confusing > > > > > > FAIL_TEST_IF_FALSE(!expr) > > > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. > > But you don't only test position conditions, you also test negative ones. > So it is not a problem with the MACRO, but where is it used ? ret =3D sys_mseal(ptr, size); FAIL_TEST_IF_FALSE(!ret); Take this example, it would be assert(!ret) The syscall return usually returns 0 to indicate success, where a negative comes from, but dev should be so used to (!ret), it is a common pattern to check syscall return code. e.g assert(!ret) Or do you have specific examples of code that caused confusion ? > 'Fail test if false false thing' is really confusing and hard to read. > > I struggle to understand your tests as a result. > > I understand 'fail test if false' is expressive in a way, but it's really= hard > to parse. > If you just read it as assert(), would that be easier ? (or you don't like assert() ?) > Obviously it's also misleading in that you're saying 'fail the test _late= r_ > if false', which I hadn't even realised... > > It's well established in basically all normal test suites that: > > * assert =3D fail test _here_ if this fails (actually a valid thing to do= if > you assert something that means the test simply cannot > reasonably continue if that condition is false). > * expect =3D the test will now fail, but carry on. > > I mean you work for a company that does this :) [0] this is a very well > established precedent. > > [0]:https://github.com/google/googletest > Let's use expect as an example, let's say I create a new Macro: TEST_EXPECT_TRUE, which basically is same syntax as FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have !ret in the code. ret =3D sys_mseal(ptr, size); TEST_EXPECT_TRUE(!ret); Or is the FAIL_xxx_IF_FALSE pattern more un-readable than EXPECT_TURE ? maybe .. > > > > It is same syntax as assert(expr), e.g: > > > > man assert(expr) > > assert - abort the program if assertion is false > > > > FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting > > the program, it just reports failure in this test. > > So doesn't at all do what assert does, because that _does_ terminate > execution on failure... > I don't know what you mean, the test case will fail, but the next test case will run. This the point, the mseal_test continues to run all test cases to finish, even if one of the test cases is failed. > We are writing unit tests in a test framework, let's use very well > established industry practices please. > > Also note that you don't even need to reinvent the wheel, there is a > fully-featured test harness available in > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > EXPECT_xxx() helpers. > The EXPECT_xxx() doesn't take care of reporting though, or maybe it needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't spent much time on those, but on brief look, it seems it is for repeating some tests, which doesn't exactly fit into what I needed, e.g. the sealed memory won't be unmapped. It is possible that those tests can be adopted to use test fixtures, but I don't see significant gain for that. > I've used it extensively myself and it works well. > > I'd basically suggest you use that. Though moving existing tests to that > would be some churn. > > On the other hand I really can't accept patches which are totally > unreadable to me, so you'll need to fix this one way or another, and the > churn is worth it as a one-time cost to be honest. > > > > > Is this still confusing ? > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax > > of assert is well known.) > > It's a super misleading name as it says nothing about _WHEN_ the test > fails. Also the syntax of assert() may be well known but you don't call > this function assert, you don't reference assert anywhere, and you don't = do what > assert() does so, you know, That's not a great example. > > The semantics of unit test frameworks are very well known, and already > implemented for you, and also do not require you to do unreadable double > negations for no reason, so let's use those please. > As stated previously, I'm not sure whether the test fixture is benefiting mseal_test at this moment. But I'm open for future conversion when I have time for this. For now, I like to get those tests in so we can properly detect possible regression for memory sealing. What will help you better review this code? Would the below help ? s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g > > > > > > > > > > Things. > > > > > > Hopefully that's clear? Thanks!