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 B2D32D3DEA4 for ; Fri, 18 Oct 2024 18:06:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3DD0F6B0096; Fri, 18 Oct 2024 14:06:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 364256B00A8; Fri, 18 Oct 2024 14:06:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 18EDB6B00AC; Fri, 18 Oct 2024 14:06:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id EBBD86B0096 for ; Fri, 18 Oct 2024 14:06:35 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 33D60A1B3F for ; Fri, 18 Oct 2024 18:06:13 +0000 (UTC) X-FDA: 82687502940.18.0C8458E Received: from mail-oi1-f182.google.com (mail-oi1-f182.google.com [209.85.167.182]) by imf14.hostedemail.com (Postfix) with ESMTP id 00F06100010 for ; Fri, 18 Oct 2024 18:06:20 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=Teh0bq2I; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf14.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.167.182 as permitted sender) smtp.mailfrom=jeffxu@chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729274745; a=rsa-sha256; cv=none; b=sRpgYh/hubXKT7RHk6yARGSwcGtvygoPyADoQWfbvVXYFfoUaQW0ChGHeyhnrEV6C6yYZS 71V70+iiJW+1cPbCetN83kbTglPkop496iz/Lqb44CP8eedr+iUW5ti2nr82N6VuYWQGa6 9h7v39175+2O4ImbBfxoBq2VlHOAUkA= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=Teh0bq2I; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf14.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.167.182 as permitted sender) smtp.mailfrom=jeffxu@chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729274745; 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=4xdUFp2BJs4C+TugGVY1v+LpNLjlv9m1e/7lI0oYkHs=; b=n1dGUBAponRdUjzgRZ6USOxnAFdNmI6LmwIX3/AfxsO3frlPY0L0eW5URDuqY+FMXQdxdw /lJOby/BjZ9cYyUUMsZOIQSBxFZHAbbS2ws9KQ6xNup/g7tiJVQgmite/e6Fz22kJWbiuv 2WARXXjgrRxzUC4oGppfR9MTkfP4/cw= Received: by mail-oi1-f182.google.com with SMTP id 5614622812f47-3e604aeb8e4so26729b6e.0 for ; Fri, 18 Oct 2024 11:06:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1729274793; x=1729879593; 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=4xdUFp2BJs4C+TugGVY1v+LpNLjlv9m1e/7lI0oYkHs=; b=Teh0bq2I1FW/gJTe00bPFzBi/uoWpgDPLGRpHuzz0VbfKq8ekAihE0Nx9CQwZDEYUe YV2SJVXywCJQuUjpuffcGDoQf8k7iIvntKGJZOYLwmsVheHBgwFLsJwnnp3LhBrkvJ1k 7ZAOfv4acRxc6FyHkk4Si5uvwk5oDWJxvbsCM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729274793; x=1729879593; 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=4xdUFp2BJs4C+TugGVY1v+LpNLjlv9m1e/7lI0oYkHs=; b=bz3LFjC3GJmCZdSdq3db9MFkhLsW4/mI7x/zcf5AVXMIjza+CLlpbzlOG1tEpsM8RI kdXvLtl736bNEhwUvOhie6BaYTSBtww4DqKfHVwSRsfkBMlJvAUO9oYNcMEVcTQUNuiF qgf/WQ47hGQiUt24CXnacHVSE7YYRV2W48mPd7LfIsngrf8m73Ie7oa3/Voy3BBOzFE2 up+ZH7NTQzqpQPP0u6hFaSm6CwTpDx+7Qwg8M91t15xCx42JrG23COKDZncOQmxLQf8c iHiW8eVXqBoV2lk1A/Nc5DafUqQpCAfL+IOyLz60CDmtAki7jPNbKzl4xY0TjRx9J0OI c8RQ== X-Forwarded-Encrypted: i=1; AJvYcCU1zQ3MM53z6XeYSQeriZk9SJF6uwub7PWAS3D6p+SOjX2ydv4EvEtqVQkELORpl1dEDZrVQDB5kw==@kvack.org X-Gm-Message-State: AOJu0YyRCCwaafrRea0eBHIuwqCuO+vLjXKVyT8sKPlp/lZTqmnXs6s+ wiwaJrq3DvTDMTmNtE4w/KRly/GyY0ra2S7xK9PvAKJZST7/49kMgg3VPxKFD5Z+BxJnNxuWxUT KNisj5rLNdUYdskz2mkZAfMZyHLoCiJLKayFw X-Google-Smtp-Source: AGHT+IGiqYAsYbp2TuEnbpop10/Cv/Z2PACZd+MZbdFdWLD0k5JeofybCn+2idssVW0UDzrceUreh3xg83fY1LOa5g0= X-Received: by 2002:a05:6870:5590:b0:27b:b2e0:6af with SMTP id 586e51a60fabf-2892c22ff16mr852559fac.2.1729274792779; Fri, 18 Oct 2024 11:06:32 -0700 (PDT) MIME-Version: 1.0 References: <4944ce41-9fe1-4e22-8967-f6bd7eafae3f@lucifer.local> <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk> In-Reply-To: <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk> From: Jeff Xu Date: Fri, 18 Oct 2024 11:06:20 -0700 Message-ID: Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap To: Mark Brown , Muhammad Usama Anjum Cc: 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-Rspam-User: X-Rspamd-Queue-Id: 00F06100010 X-Rspamd-Server: rspam01 X-Stat-Signature: 3aayqfkai97ryzwow3jebwu7kdcdjuj6 X-HE-Tag: 1729274780-202747 X-HE-Meta: U2FsdGVkX1+jMqp7kJ/Y08MfLFp9Ywlk4Nv6w8UN2+6vzoOHq92HwkrksnZZfBvM2x4wj3dBm5e0h9qOsZt4W1SmE6ykSzSrfdIGCWLMdAyY+lKvH0WoW/vE7Os3v9hkY+kiX3OHdhFjff+fAAwIJROMD7JTEbeEMbFJfr7ip08zYsSK5fdI5a6xby/fKaweWSmxBg1G3FW4tkfioNrLJ1/7Kp3agNTlvwtaA+f1l67Zp/9gvHj+DjeXg7JZHTxuOzAWjkgTnVphtb6ftuOkepjZl70pjj8zuQBa7eJHGUPBLHu2APNnETHyO2oohqA4A6Dw1ktAe8nkTZH4BOH0oAaSx+ycjTA/AYTlZxWnhmEInSEZ4661rrmkdSUKOaswmD8pJ5PhJ2BlfGrVw4k5IBvIUxhAfDbLn0iCqn4wN8MF2LPBoEe2yemnMRjNp7v5RoJS5ZADWbsQpC4dfYWWT5/pQtT1EWYSHnfAJYbu5bT9em3gWJvahyfBu/qaczDRuV/6NmCarbXIeAgCBqRTLUTR0yzshjb/7GPOxrzaxtTU6gDsnAolXhE7KgoGTc+/iAgrD8EVO7FcWuqJHhwsV/z9g1RaXyKNud585fpGcf5j2fJvRb1V0UBnsWPdyJmXYTaCaXf7bsZqBPG9/6RDU9VMTajkNE20ocR29NPn12eq3/+Fa02p8uAA/S4iJ801W1pgVQMH6KgCZHOKZXBbC1b2Pysf8M2OXWbVFNF4yZsVHabHOjujgRysDUInQRjlGexvvNaOPWfudRd9jYvVkAe7ugP1BODULfKptGnxtNsebNx+7geLrHDvHaHQHwOvK4Fhp3djLXXrBEULcqkj7hLLPiYrsJY/9UwJUOvyEKtfni/BWJCaagcQXtehL08jEDqePNYIou1OLlDTU6JNHGdC7/BY02ESukn3pZsi+Sy4qnNWBxgPaAva5njyXchgQwb5HG9VrMUciuitRy6 +dm8ajuY gxkMJ96BRsi6RrC49p3Cny0eeKT3CmoFOqGS50DGRCo9Q3vaREl6erHIV5E0U+c1e1bozsKWutCDpetkGOPe+P1JDdKh9cM0kfACTdtxFTb+e2LwQAAybbXwApTru1lWD5YKvcyt0kXDNAxOrUkffIXPFPRFioRLpjOsonqI/OG+0h0TF4rPaTwtGgng53uICVuyPbiLNp5wzouapg000LUhWO6kaLcQKXUsOQLkSBzOAmT/JUqTrq3tX7wMQlirMEuuMGfuZCdQA9bA0XVzvelNyV6jnFW3g3hL4x18dcrNx5B8= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000242, 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 and Muhammad On Fri, Oct 18, 2024 at 6:04=E2=80=AFAM Mark Brown wro= te: > > On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > > > 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 problem is that the macro name is confusing and not terribly > consistent with how the rest of the selftests work. The standard > kselftest result reporting is > > ksft_test_result(bool result, char *name_format, ...); > > so the result of the test is a boolean flag which is passed in. This > macro on the other hand sounds like a double negative so you have to > stop and think what the logic is, and it's not seen anywhere else so > nobody is going to be familiar with it. The main thing this is doing is > burying a return statement in there, that's a bit surprising too. > Thanks for explaining the problem, naming is hard. Do you have a suggestion on a better naming? > I'll also note that these macros are resulting in broken kselftest > output, the name for a test has to be stable for automated systems to be > able to associate test results between runs but these print > > ksft_test_result_fail("%s: line:%d\n", \ > __func__, __LINE__); \ > return; \ > > which includes the line number of the test in the name which is an > obvious problem, automated systems won't be able to tell that any two > failures are related to each other never mind the passing test. We > should report why things failed but it's better to do that with a > ksft_print_msg(), ideally one that's directly readable rather than > requiring someone to go into the source code and look it up. > I don't know what the issue you described is ? Are you saying that we are missing line numbers ? it is not. here is the sample of output: Failure in the second test case from last: ok 105 test_munmap_free_multiple_ranges not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 ok 107 test_munmap_free_multiple_ranges_with_split # Planned tests !=3D run tests (106 !=3D 107) > A more standard way to write what you've got here would be to have the > tests return a bool then have a runner loop which iterates over the > tests: > > struct { > char *name; > bool (*func)(void); > } tests[]; > > ... > > for (i =3D 0; i < ARRAY_SIZE(tests); i++) > ksft_test_result(tests[i].test(), tests[i].name); > > then the tests can just have explicit return statements and don't need > to worry about logging anything other than diagnostics. > > Depending on how much you need to share between tests you might also be > able to use kselftest_harness.h which fork()s each test into a separate > child and allows you to just fault to fail if that's easier. > > > > We are writing unit tests in a test framework, let's use very well > > > established industry practices please. > > Plus also the fact that we have a framework here... > > > > 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 > > I rather think people would've noticed if the test harness was so broken > that it was unable to report failures. If it is that broken we should > fix it rather than open coding something else. In general, I agree with those comments, but I would like to rely on domain experts in test infra to recommend what to use, or is acceptable. In this case, I hope Muhammad, who reviewed this code in the first place, can make recommendations on a replacement of this macro. I would image the needs of something similar to FAIL_TEST_IF_FALSE is common in selftest writing: 1> lightweight enough so dev can pick up quickly and adapt to existing tests, instead of rewriting everything from scratch. 2> assert like syntax 3> fail the current test case, but continue running the next test case 4> take care of reporting test failures. Thanks -Jeff