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 1348DD3E195 for ; Sat, 19 Oct 2024 00:10:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 758DF6B00A8; Fri, 18 Oct 2024 20:10:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7083F6B00AA; Fri, 18 Oct 2024 20:10:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 55CD56B00AC; Fri, 18 Oct 2024 20:10:18 -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 959BB6B00A8 for ; Fri, 18 Oct 2024 20:10:17 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B6D891203A7 for ; Sat, 19 Oct 2024 00:10:05 +0000 (UTC) X-FDA: 82688419044.09.4330FC7 Received: from mail-oa1-f53.google.com (mail-oa1-f53.google.com [209.85.160.53]) by imf15.hostedemail.com (Postfix) with ESMTP id 7FA8EA002D for ; Sat, 19 Oct 2024 00:10:03 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=S6h8QzuN; spf=pass (imf15.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.53 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=1729296469; 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=E22HImJZ028W/1sQ4maAbpjY4QAR955Gkk9SdRCpvUQ=; b=hWzzr9V+ctGcCul5jaz3zuu3BrSL2ydzVKqO0wvvtx63C6rvT9v6ciqa2+9Nzaq1YfcgLl 7u64nac1es/Upz/l8X9iVGVQt6L+//a0lsAWJ2DOmbJm8duBnSOVE78+AIqmdfDav/THKl i+neHaUv0uBcm2/wrEXjpSIuMF7Le4w= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729296469; a=rsa-sha256; cv=none; b=PUgyZRi48itQ7MWb91TptJq33/3f0P64FFrc0X6KptNrM/hbHKxbkNVHZLky/UEo870svr FHWxOH3rDn5a2wtwC6qb2PO+X8Hn+qEn6RDttBOmYoWGHBNyrHR1rm4FWgR/M1oNpA5EiV GD0AbvBFNpf02SKf3j3FU5gD+NNxCJ0= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=S6h8QzuN; spf=pass (imf15.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.53 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org Received: by mail-oa1-f53.google.com with SMTP id 586e51a60fabf-28893cc3acdso438002fac.2 for ; Fri, 18 Oct 2024 17:10:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1729296614; x=1729901414; 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=E22HImJZ028W/1sQ4maAbpjY4QAR955Gkk9SdRCpvUQ=; b=S6h8QzuNVPqHfXJ36GC2a/7buywig0cl5Ez8dl6bkAqbOFIT8yUdrLH25wCP7gS4Mk HbZZmS82X58jofbWndih+ZWV+x36IL3C4byddnDaEwmWDzbHSaHUCVyZntpiS0i9eit3 JKxiAucPSqNJHU5lHdBp7cOwP6wubFbOl8NCw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729296614; x=1729901414; 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=E22HImJZ028W/1sQ4maAbpjY4QAR955Gkk9SdRCpvUQ=; b=WlINyAwBulyyCvtvoNjUl6/7IqmvDgWy2LZu6aDlbuUUvC9gjCbautOR2ySAjkZENa a3HQabUmJdzMhl5DdPpQtDLkhAbtyhUdX2jwmj5Ek62oeQJHfQ9dR30X4sLn4k5JWynG fjK48NDH5Hq/qeoeOQN/jnYhOWGuZyZhnF7mOSi+Oos1/ZVpzwk/EOgSbt2d5orl5LQH GJQbHHSLGXMgfLj+Yroc1/J/GCooKG7HbNUmB3j1NH/o6JBkH+JJoVT8iP9gQnO9iEXS sFDIFlnBNGEUV0LrlRYmD8hJMH6NQuraEX7HAIhAQfojQhF0W9/8cwQjc4ucDKCHGEr3 6f8g== X-Forwarded-Encrypted: i=1; AJvYcCXyxfGVTnju0qS/uSzl7eHo9+mWGMlECJtcacObd5NzR3ngAM/1AEbxYUyytH9/EW0qg3tcYGynVg==@kvack.org X-Gm-Message-State: AOJu0YzQnw7EcTuWzRPoBQDVsDBoILZPOlN6/ZPd2RiX0uNcMO6NZMoi S231TLOThR33Fnz/m0l7UrovZUdUC/1t4asEOGlNUO2oBNEy+UeKuT00VkMDBeE0HXhB/GEeN9/ hCBHCp3phtI/iFXj5HNBzZ56AdScYCzHzHRDC X-Google-Smtp-Source: AGHT+IFIihPyXoNPdentrsybY42eayzsimY1yFxEH7573nql2cE6j/C/m4TScAB/nVOH3efQdB/x8+eCEYm3oUOJiR8= X-Received: by 2002:a05:6870:95a1:b0:27b:9f8a:7a80 with SMTP id 586e51a60fabf-2892c55337bmr967265fac.13.1729296613985; Fri, 18 Oct 2024 17:10:13 -0700 (PDT) MIME-Version: 1.0 References: <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk> <6aefd38b-d758-4e7c-a910-254251c2a294@sirena.org.uk> In-Reply-To: <6aefd38b-d758-4e7c-a910-254251c2a294@sirena.org.uk> From: Jeff Xu Date: Fri, 18 Oct 2024 17:10:01 -0700 Message-ID: Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap To: Mark Brown Cc: Muhammad Usama Anjum , Lorenzo Stoakes , 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, 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-Rspamd-Queue-Id: 7FA8EA002D X-Stat-Signature: uwek1qmqftjkmbetkk56udfsu6axsy7n X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1729296603-175115 X-HE-Meta: U2FsdGVkX1/HBKfoCshE8V4CH9Ys3kXaY+OB6S3QkHAmlGu1T8XKvJr+yC/5mBSXkNMee8ENTD1AteqzH4dabA7qDowV8vsrO1TYQ4aNFfS4TOnBwTyxSzqSKIaj3NuxIUc14b0EDCDXFpt5se9WsuSDsiI4U9OFvX9tIskQ40d3+xDcssqpvahgNHEw+mHtG0MiwERIf0bZAzBvFLERbcAstT2vnxvvbaCx4/uoA6kuXZkvBgMyWNrQAPElEkLxQeXwQblvnp//UgAjA0fVNPH3B3pftGx3ovDunVOD7gW+NwXQpB0sd7GR+xtpztD56JB2Kn17lLgYhXh8yAy8rVeY0b+aZXHrzovKl1mKwr237hQc9xpNmxBBCNaAS+M89djTWeBmGFECEGl5mx+x7XZ2BAMetLHLoi/nI/nTC2zINs80m9opEq8cq3oYFJWbCFgAlF8AxnvF/ryc8s0QTjbRCg9mp/FjKUcqJn7mkB4nZmStTKwpTgK6iYKoV5pbXu4mcKbzzc7GtkwXy6dXQta4SyEAJi7dRQDWJgDVUzLXm26ShhAvUQ0oDfz5uDgJQ3kSDSDAAX1PLPjE3mCzUsGKrXEVUEQZQFN7kjrYEi1iQ+0iEBwEWetiYmkDJjKYXiuZ27+ij+jbO3u+ta10wbBYKHwSrpICe1iYPh90yg7wXRpMRmLfiOqSppXQ6wiIR69krmEJU7jbU32yYUAtBovdNFzPj56Ab6ysxbdbqTmd6pKOgv9ZIbt2o5KRfWiLwQjHphEe5QbQGKrtiFvmU1DJnrnkMcTOwkVDvYuCMwVG1dDkQcyWSHalmZ57ip9bfyAgE9kXBZRrGCBoKf2tqAKynA+5dSyi8lGcLsA6alkZlHVL2v+UohG5EBSwgj06a4DX3FGWida8thzS3eFj83N+E1KM73ypKQExaj1EWjXuYEPBEa1J9bQGLjLk/LIGmD7OQYYUaq7okPa23ig BeNvb/8w Lo9e7UAoR6cCn3iXgkF8UKoU6ucGK9cLvww9/KXtefMre++eb5Tn2jMoA4b0v3tH4L9c95B4MSg75GYo/zM6PVEhpNTCDRZ3BTKX+pU+gKCd8bzJEf48ZpEDgzzbwUYDPnvxkyig8e4tnoC0lLY7b9+wNY7IGfQ+5RxwehwU/M5HYeKJKnTWIIIfsIQJIrL3kUNjxxWiVhHkWrHQyLydfdJSgX7uEBRs7nEJsTEpyWJOVt7p/lJPefp4zWVVI3K8QG9R1PjZQpKvrni+7O7uSempZVDsGqg/Zs9uyuR3rBKb7ZYU= X-Bogosity: Ham, tests=bogofilter, spamicity=0.020155, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: HI Mark On Fri, Oct 18, 2024 at 2:05=E2=80=AFPM Mark Brown wro= te: > > On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote: > > On Fri, Oct 18, 2024 at 11:37=E2=80=AFAM Mark Brown wrote: > > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > > > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split: > > > line:2573" which automated systems aren't going to be able to associa= te > > > with the passing "test_munmap_free_multiple_ranges_with_split", nor w= ith > > > any failures that occur on any other lines in the function. > > > I see. That will happen when those tests are modified and line number > > changes. I could see reasoning for this argument, especially when > > those tests are flaky and get updated often. > > > In practice, I hope any of those kernel self-test failures should get > > fixed immediately, or even better, run before dev submitting the patch > > that affects the mm area. > > That's not the entire issue - it is also a problem that the test name > is not the same between passes and failures so automated systems can't > associate the failures with the passes. I failed to understand this part. Maybe you meant the failing logging is not the same across the multiple versions of test code, by testname you meant "failing logging" >When a test starts failing they > will see the passing test disappear and a new test appears that has never > worked. > This will mean that for example if they have bisection support > or UI for showing when a test started regressing those won't work. The > test name needs to be stable, diagnostics identifying why or where it > failed should be separate prints. > If the test hasn't been changed for a while, and start failing. Then it is quite easy to run the same test on recent code changes. I think you might agree with me on this. The only thing that bisec needs to check is if the entire tests are failing or not. I haven't used the biset functionality, so I'm not sure how it works exactly, e.g. when it runs on the old version of kernel, does it use the test binary from the old kernel ? or the test binary provided by dev ? > Actually, prompted by the comments below about test variants I've now > run the test and see that what's in -next is also broken in that it's > running a lot of the tests twice with sealing enabled or disabled but > not including this in the reported test name resulting in most of the > tests reporting like this: > > ok 11 test_seal_mprotect > ok 12 test_seal_mprotect > > which is also going to confuse automated systems, they have a hard time > working out which instance is which (generally the test numbers get > ignored between runs as they're not at all stable). The test names need > to roll in the parameterisation: > > ok 11 test_seal_mprotect seal=3Dtrue > ok 12 test_seal_mprotect seal=3Dfalse > > (or something, the specific format doesn't matter so long as the names > are both stable and distinct). > Yes. Agree that this is a limitation of this macro. > > Having line number does help dev to go to error directly, and I'm not > > against filling in the "action" field, but you might also agree with > > me, finding unique text for each error would require some decent > > amount of time, especially for large tests such as mseal_test. > > In these situations if it's a typical Unix API function setting errno > that failed I tend to end up writing diagnostics like: > > ksft_perror("open()") > > possibly with some of the arguments included as well, or something > equivalently basic for other kinds of error. This is fairly mindless so > quick and easy to do and more robust against line number slips if you're > not looking at exactly the same version of the code, sometimes it's even > enough you don't even need to look at the test to understand why it's > upset. > I understand what you are saying, but personally, I still think line numbers are a faster and more direct way to failure. > > > Honestly this just sounds and looks like kselftest_harness.h, it's > > > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for > > > for asserts. The main gotchas with it are that it's not particularly > > > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixtur= e. > > > If I switch to test_fixture, e,g, using TEST(test_name) > > > how do I pass the "seal" flag to it ? > > e.g. how do I run the same test twice, first seal =3D true, and second = seal=3Dfalse. > > > test_seal_mmap_shrink(false); > > test_seal_mmap_shrink(true); > > That looks like fixture variants to me, using those with > kselftest_harness.h will also fix the problem with duplicate test names > being used since it generates different names for each instance of the > test. Something like: > > FIXTURE(with_seal) > { > }; > > FIXTURE_VARIANT(with_seal) > { > bool seal; > }; > > FIXTURE_VARIANT_ADD(with_seal, yes) > { > .seal =3D true, > }; > > FIXTURE_VARIANT_ADD(with_seal, no) > { > .seal =3D false, > }; > > FIXTURE_SETUP(with_seal) > { > } > > FIXTURE_TEARDOWN(with_seal) > { > } > > then a bunch of tests using that fixture: > > TEST_F(with_seal, test_seal_mmap_shrink) > { > if (variant->seal) { > /* setup sealing */ > } > > ... > } > > TEST_F(with_seal, test_seal_mprotect) > { > if (variant->seal) { > /* setup sealing */ > } > > ... > } > > You don't need to actually set up anything in your fixture, but you do > need to have setup and teardown functions so the framework can emit > required boilerplate. The gcs-locking.c test I recently added in -next > is an example of a similar thing where we just need the variants, > there's no actual fixture. Thanks! This is really helpful, I think the existing mseal_test can be quickly converted using this example. (A side note: if selftest documentation is updated to include this example, it will be much easier to future dev to follow) Thanks -Jeff