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 126FFD3DEA6 for ; Fri, 18 Oct 2024 18:01:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8E1236B00A2; Fri, 18 Oct 2024 14:01:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 891636B00A4; Fri, 18 Oct 2024 14:01:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 70BF06B00A6; Fri, 18 Oct 2024 14:01:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 4CC406B00A2 for ; Fri, 18 Oct 2024 14:01:58 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A432D1A1A7F for ; Fri, 18 Oct 2024 18:01:35 +0000 (UTC) X-FDA: 82687490970.13.01B7312 Received: from mail-oa1-f45.google.com (mail-oa1-f45.google.com [209.85.160.45]) by imf06.hostedemail.com (Postfix) with ESMTP id 8C1D918000D for ; Fri, 18 Oct 2024 18:01:47 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=kD6XUpgo; spf=pass (imf06.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=1729274368; 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=9OKp72oFdRRG8t+D/9JG91r+3KRfEXqiJBYSKDyOMOA=; b=na2tZwcaVR71WIHVuFzrhuo8nLgSxFwz9RxSZP+hY/m8TuIdeF0ynddelq/jexSWvAs/Ze 2OEgfvN5VOiV//NESL+cuIg+D3g8oE1G1m0e8k4K3bmnGwcpFbGeVBFmTviAQg1SsyCMLV tjCIO5Nb8P6j/nAdzo6cOpGXNnUvEm8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729274368; a=rsa-sha256; cv=none; b=FFWgmc7T+aI4Pc/zOd0KwEM9ijH2HfS8Dc579ceorcpnwJL0DyxccIJeh+Ww7gxf+XRIi9 bh+NRbwj7/ljlb8qcO1lV4THbji0reyyULXH5L1NiP4xsOG15Y3juRIrf+cZS66R+bjVQP JbPoCf8TsAGMWhqR+zkTC70SEp8vHKo= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=kD6XUpgo; spf=pass (imf06.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 Received: by mail-oa1-f45.google.com with SMTP id 586e51a60fabf-288824e9cc4so302353fac.1 for ; Fri, 18 Oct 2024 11:01:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1729274514; x=1729879314; 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=9OKp72oFdRRG8t+D/9JG91r+3KRfEXqiJBYSKDyOMOA=; b=kD6XUpgoFHp4Gavwv3B321Z8blNKzU0rS4xKP0VMzoFsmOBURWHv0tXlmo9UJioNFX h4ClLudsN1uIORcRq2UHLJS608rZhCQj6ddKnHoNUJvndKPJjFfw74Xb+fEJ1iqCEW1H yC3CjvvJ+ylWOCTkbvR93E8HpKpwjP6lbnhYk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729274514; x=1729879314; 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=9OKp72oFdRRG8t+D/9JG91r+3KRfEXqiJBYSKDyOMOA=; b=QUedn5Qk4SDnbC6quxxmnOZ3O9Z87AJZGHgZdpR0HCcz5yMFPJVGwijyaX7jjCX45v /81QmEuArGtOgXzMYuuaM5xJPLmd8cxDeb7CkoVkGi6drDOvh3DN5BsjtQoE3HKs9PAN RP91xrYfipImlcfevBwAqJhuxnpMAySdUm0xrro72w1EawtR+f9Xt1Bmql++TSZvbqyx Kg1R6QKn932aOyI9NalEIPCYTuetm99BscOx9Jcg7GldsrWr4sfh6nqAPv/AbfzcxGF8 O3iFEofCwQszVekk+urvTuWio7VLSp3mq3NVu6BH+DrJOo/jUpCVLFW43SiYLofuEz+v 0jXg== X-Forwarded-Encrypted: i=1; AJvYcCWHBPYlsPlsxE2E8RdyotcuD7TZ8KLCglfkseavx+HWTDg4EVOUmdirfck3bN+wGdQaveo8Za9Xzg==@kvack.org X-Gm-Message-State: AOJu0YwzHmx55TA/dnkTeQOt5ohqxtMTKZtlGrjWGVVBvizmsFMNvfjp no0dBpziJuSWVrzxREzd1+kX10Ep/IOSwaVWdd+NLZw9pcG8TZAKmZHLe3ov7sXKOg3mWobLj80 p/vvyYPJ3N2SSRQh2pAMmHyXCqkehYeLuE81/ X-Google-Smtp-Source: AGHT+IGKPvjDV4dZmtSKWkrmR0IHyPkLSBZLU0gQOTWtc6F9ygGetic1QXOrzVfdzggME43tb3cFYb+uxSD7ggykO7U= X-Received: by 2002:a05:6870:f115:b0:27c:df37:9e0c with SMTP id 586e51a60fabf-2892c3426e3mr779683fac.8.1729274514353; Fri, 18 Oct 2024 11:01:54 -0700 (PDT) MIME-Version: 1.0 References: <4944ce41-9fe1-4e22-8967-f6bd7eafae3f@lucifer.local> In-Reply-To: From: Jeff Xu Date: Fri, 18 Oct 2024 11:01: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-Stat-Signature: 8huka1bup4hfgfmg9hfznsz59psjpfya X-Rspamd-Queue-Id: 8C1D918000D X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1729274507-379372 X-HE-Meta: U2FsdGVkX18bsoGhYJpfN67zLLXg5b/vgaNLdXlEbEqL0+zpJ1vGAt4wdZ1jpaubP90RSIGLF//L+PvMLSJK99FkR5N5kdsHkmqMzLvKNvFOe4HwBGUNp2YSdREkTjQ1S+ZjNcW4anEISwwSy7XLn+PGUMIx6wnnHoNfLicRZ1a2XKaRY0i89tnmvVATN/Tsbeb3Y2nqxBd5RHYAeeAT0Vs3Rv53lgV5ZcC/EIg3z6OKeWOggyIyOGKg8Ro0MuuIXPo+QZlaW994rqh3Ce4Gjdeofo7MZbWbl/vGSrtepaGvDnQvZTj5AyvElwKLgImIGoMUiZBJ35NFtY9wOCzHrohqlX/75cMTYLgjqxbkSix5EAinIvZn6Y6wX4F6WLaHMj59dVngLC/odmD1S7zu9W2XE42wO4p257l21DGE5irtCFmbTMkdX2tyBSU8emCA77fHsfavyulYUJZka2MOPGD6RyYKbbYRgtGwQIHjQA3TBMz8lBdCET0MTgyiy7+Q9mZIQCrhDyPhAmDjhK2FfBvVw6qLWVkI574JX2j9cs0APl5F09wcJGLBvT1Qfa6PuMbmf0TRaYE6EH+sQTJRa9aNGMT3GRqfKfwBsEi5H42uwXgl+qrTTX5ZVUchCpZ30Fti7K/ioKKQlvGSE2GRFXx8UsaABfUSPAI0Wx9R6nidKNPkDpcHlTI5lzNWm+t/0kHWCBCKjz9DkDFYDFB8deul0ATIX5fdIbQX1RVQDbU6P5xfLkOkxRK11cibV3x/66zYdTlpAqymHVAhHkMLZNmM5L6lHw4oJ+nJBDqQ2YLWvC6gDtwVHXk29Kvz0M2YfzGnuEa7Gp5CQdw/v/ddiOVaepYJAmKdFeEDYRJyp/Y03y6umLJB115aVPhF/lW3ZQd1sgd4SvZ4avyVp4ZBXHg++mFnaywnN4vm8KrIcmJoCaTlGOYVxnw/Oi+mfsc8KDRHUEvLP3rRiJ9ouc+ Eh6chcsS T9kNjMTw5KhLVsDGAaRuc6ZL7FXvQCgsv88Qm7xoPWmS+vc90PibGrPs28AReNzPymwW2/Z/VHmy4tsWD7e1BDOHZwqF1JF/y2SfMBEaAh8G4AteU3fwbWvK8Q9fXNucSByxzwwwF578TEoizvL4B31lf3j2+gwnTnxZXgXDGp5ynpbQndtf6AxphhcV/z/4p+CqlKaIiau2sX9TPfZo4d86Gb5bC5mKc2YS+BBuBIqW9NPFPGlkHU6zlb53eTItfB72ssIRDyXLTkmdjiXUt9dA8V23zp9nzGkHQ2O6VKtH7PSIF0UUE45+Shf4Ubupl1XXjmLWczhkyxmfHWRMlbYyIxwBb3fSmvHC11Y6ZMwClT/PUl7D33AcxIXpdwbcOsJ6bRUG4gBmJ3b2gPklj57G8w+PUAsNaQKCKDGqwRpkuP7PpAGRTA7bL1LHCFuNxU5I78JaETLRJV2eXen3Wkchl9JTYIsi4gzCB/SiiBDoJC2VzZYeh2jJFbJIG0ZXTS24R3VzxUJUm6QDAQt6Tj/X+mQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000481, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: HHi Lorenzo On Thu, Oct 17, 2024 at 11:38=E2=80=AFPM Lorenzo Stoakes wrote: > > On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > > 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() thin= g 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 not do this. > > > > > > > > > > > > > > > My most test-infra related comments comes from Muhammad Usama= Anjum > > > > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e5= 8f336b6@collabora.com/ > > > > > > > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FA= LSE > > > > > > > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest= (see > > > > > > [1] above), and I quote: > > > > > > "We don't want to terminate the test if one test fails because = of assert. 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 m= ight > > > > > > also help to clarify the requirement if needed. > > > > > > > > > > > > Thanks > > > > > > -Jeff > > > > > > > > > > Right this is about not failing the test i.e. equivalent of an ex= pect > > > > > 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 o= nes. > > > > > 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 re= ally 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 _= later_ > > > 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 t= o 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 we= ll > > > 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 abortin= g > > > > 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() an= d > > > 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 t= hat > > > 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 synt= ax > > > > 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 ca= ll > > > this function assert, you don't reference assert anywhere, and you do= n'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 alread= y > > > implemented for you, and also do not require you to do unreadable dou= ble > > > 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 > > Jeff, you're falling into your usual pattern of being unreasonably > argumentative for apparently no reason and I really don't have time to > constantly respond inline when you're just ignoring what I tell you. > > You do this on nearly all code review and this just isn't working. If you > want to effectively be involved in mseal you need to LISTEN. > > The more you do this the less patient everybody gets with you and the les= s > likely your series will ever get merged. This is not good for mseal or > anybody involved. > > On this issue - either use sensible macros that YOU ARE DEFINING, not > assert.h, but YOU, that allow you to evaluate whether a condition is true > or false - or I will not accept your unreadable test code. > > It's that simple and I'm done discussing this. Thanks for your time on discussing this. Please, if I may say, when presenting your argument, keep it technical and avoid personal attack. Using personal attacks rather than using logic to refute your argument is =E2=80=9CAd Hominem Fallacy=E2=80=9D [1] = and will make it harder to get your point across. [1] https://www.txst.edu/philosophy/resources/fallacy-definitions/ad-homine= m.html#:~:text=3D(Attacking%20the%20person)%3A%20This,who%20is%20making%20t= he%20argument. Additionally, The mseal_test was reviewed-by Muhammad Usama Anjum during original RFC discussion. IIUC, Muhammad Usama Anjum has domain knowledge for selftest infra, and I have relied on Muhammad=E2=80=99s comme= nts and implemented all those comments. I'm not saying there is no room for improvement, but it should happen in more respectful and constructive ways. In any case, I hope we have common interest and passion to get more test coverage to avoid future regression. Given that we had 2 regressions in the past during code reviews and a pending regression to fix at this moment in memory sealing area, the benefit of additional test coverage is obvious. Specific on FAIL_TEST_IF_FALS macro, during the course of this discussion, your comments have started with, and I quote: =E2=80=9C Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy. Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().=E2=80=9D =E2=80=9CThis is beyond horrible. You really have to add more asserts.=E2= =80=9D TO my response: =E2=80=9CASSERT_EQ and ASSERT_TURE are not recommended by the self-test. Th= e FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks related to self-test infra, such as counting how many tests are failing.=E2=80=9D And your question: =E2=80=9Cwhy we have to assert negations only, and other self tests do not = do this.=E2=80=9D And my response: "My most test-infra related comments comes from Muhammad Usama Anjum" (added into this email), e.g. assert is not recommended.[1] , [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collab= ora.com/" And my additional try to clarify about your question about negations: =E2=80=9CSo 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)" And I offer an alternative approach for macro naming: "ret =3D sys_mseal(ptr, size); TEST_EXPECT_TRUE(!ret);" Which you didn=E2=80=99t respond to directly. Given the situation, I think it might be best to let domain experts from the testinfra team, such as Muhammad to suggestion a better replacement for this macro. Best regards, -Jeff