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 C057AD3E189 for ; Fri, 18 Oct 2024 19:32:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4B85E6B00AF; Fri, 18 Oct 2024 15:32:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4685F6B00B1; Fri, 18 Oct 2024 15:32:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 32F936B00B3; Fri, 18 Oct 2024 15:32:53 -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 15B106B00AF for ; Fri, 18 Oct 2024 15:32:53 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 81EA3C095C for ; Fri, 18 Oct 2024 19:32:39 +0000 (UTC) X-FDA: 82687720458.28.295530C Received: from mail-oa1-f46.google.com (mail-oa1-f46.google.com [209.85.160.46]) by imf22.hostedemail.com (Postfix) with ESMTP id 121D9C000A for ; Fri, 18 Oct 2024 19:32:36 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=MYZf9EEq; spf=pass (imf22.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.46 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=1729279824; 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=gplWmfGOWe5Pq8fZbVA+dPlQSygIYBkNS0fSsl2L6Z8=; b=g4IBdLumtHk8fs8lWqPU+w/1QfDrtCnOpseYf1Umupc3ZLGtPBUo64a5DgriHxF4rgdX87 BWRwMKFKzmOBmL0ssoTKFCq23WrA+uwgjAI0FVRGPI4aml6d7Uh0SIjVc2mUUsw7gcoWjz LLFTRVth2KgxqOCpcZFu57oXWwwxjVw= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729279824; a=rsa-sha256; cv=none; b=NmSG8zo/wTKWzUZ33JHJlS6qSetk6bWmxSW08t1BbvB6iVtSXINfTVK55yfqDLYZ+Jhs8b 3+Br/Ec2L+rDj+ta3KjRfN1q+MWRT9y9E1VEEjtbJca8NOmTcbXMNHDnFGSZpqk30ejhCd drgCqE3oPE2ynq2qdwTkVtOhBLAVveY= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=MYZf9EEq; spf=pass (imf22.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.46 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org Received: by mail-oa1-f46.google.com with SMTP id 586e51a60fabf-288acc703c8so98180fac.0 for ; Fri, 18 Oct 2024 12:32:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1729279970; x=1729884770; 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=gplWmfGOWe5Pq8fZbVA+dPlQSygIYBkNS0fSsl2L6Z8=; b=MYZf9EEqIB2k5RDdYFZx2Iwvx/YDxgtXjnFsv0LvBZdciBym4c6l6hzp7QcaZhsDXF /ObnT9bZEnC9cNfWfHwlD/lx7zki4/YDwb5VmGH3O+yyWRJfyTMjPLl4fV27m+Cq11fk FtYXwN01L026ofnoszPKBofvbu4YmKAjbMPxo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729279970; x=1729884770; 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=gplWmfGOWe5Pq8fZbVA+dPlQSygIYBkNS0fSsl2L6Z8=; b=vG5gjuQJOmKHdF8JYGDEQWwSREXGI8pRr2QtQ7nEd3Larnfdo0l8oW8HYK5wYATG/A PhH/53TkJoU0GfDfTH6W3WA+jjWEO05lrsh3dB6LqKTTTqhbnwXNLL/fBOgubPOWMrkp 1SA/dqy5rdBeX2D1OQoJ3HEgl1zFTrsUyzdLxjvsGsh3pkUK1Hxl+pnH5LR9FMlpSzsK cIudzkXYX5TYkyUpQx8aQJ/JaKExEcjISn1H5HXrdLDVhnskBJWnmKB93ENc6XriKez1 BlOcT2xw8MHI4lKWs2/8en+8jVJ/UGuwSK/huyCCqTgaWaKWlu61Oki9gBTT9PgunusM EkGQ== X-Forwarded-Encrypted: i=1; AJvYcCUOX6C6Cr48U7hFOTJ8HazdycOAUiZVWxRLUPjp8H5BGClH80t80TD1Sthntl2K4ORN7j5iQMqjeg==@kvack.org X-Gm-Message-State: AOJu0YwGRAXgqK9iUeZ5Z2QM3GZjbbpXwFP6Rzu9748d+axjrNHyai25 JZWYe2vnjSZnLUc0ID1kzp+xb1T6+K7MxjHJOb/lKYNA5ZEU5/lS8jD7KeyirgIYSXek4c5cAvR AeE9vD+5+Ej4/TtDaHbD+bWysxvI/9pTdYXGV X-Google-Smtp-Source: AGHT+IFVeKDD7KbSW5OcBfxhKKW+l7qGtWlzjoy4awnj9k9KumaMQ10QXXe1bakBP4FsadALWQQuMH8NVDjIEtvir80= X-Received: by 2002:a05:6871:813:b0:278:2698:7721 with SMTP id 586e51a60fabf-2892c3418dfmr810796fac.8.1729279969805; Fri, 18 Oct 2024 12:32:49 -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: From: Jeff Xu Date: Fri, 18 Oct 2024 12:32:37 -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-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 121D9C000A X-Stat-Signature: yxfbfzq6pqr7xzj7f6k9z1zct38qrzbq X-HE-Tag: 1729279956-158759 X-HE-Meta: U2FsdGVkX194beu6Rowvpa6XKOxZy9nmqeZvdc2UA9MiUO4z2RCD2E1uB6YrX8irSQdOLbN4fYJ/ZScVXYHZn89IsJZSnCILvgOosUK+pjQES6vdjZk86LXkZ1A9/FCBVDuTsK6DhEZ1JyNKqhC1GPCTm+ZrJFrVQmyOC6iLFxh7zG7cGCVEvq2lQp+C6t2VK3u/+fmp4J1ITGr9cGiEfeBbqNCIUkbqMR6mJUN3b9NbQvwdzq9eBllziHXtf1dey0rU9hPfW/iZSqpuaSX39OEJqI9KEV/BMkejK/krW8BK1aH2MuoMkK9DPU7LS25XbIRMmuE7tENMh4ATJfNDzaarfYwGl4/7wAz+SdAKtemkREQhpIVbNYipGwoxDzp5vX+6Bji7BcZeC+f5N2aK1dosmbteFnMXoKTXN+Ys4JTF4qYhJf3V0wC+sO3TCXSDLFyYP4/eU8lugxpUdm4AbRP5R574GoJwVcNy2g9CFPJYmz98KkJNcP1tPXZp3ZYng+X1Y8BC5XGNpADpOCZQrXncOwWNK6P1B3BbBMcEy7tTwHSNme3Kf/QMJY7kGJQ+Yk8fBFx9TqLYGbscuRlEmRcD9EsuXleLJ7rjjGDTvEjmUES24iIfEBrUprQw/xvhqSO4nfjIei8YgPLWDwlPOExr5zq+3xKdgOlSeCAqSxUoxgrDaj9KAZU6jP5z8/+qoJ/xCDrYjrQADwgVCP85OOTqwi5w6WK0GKtppybqFzPLDzChrFRZYXodLuOpgZrep52J9EnJRjq1aqaOb20ixYthGs9qwiZTkECPtsOM54qUsPQyiURN/+Yudx71WJYpYervTeAm9Qi9hXimxysbj46Nqt5UQPuysmKK9x/6biIaCiyjYyGX4YlESeMqNAdmBEcwonoPLcmZqbRCRB5UuaaOQ51UUeYeB6IjdnZL71kxQoDM+MRVFXcyzn7/krY6IwffUg5BFAZNOkcRvuR MxrKtiUs X7sYxx6uh4nBZsBoH0446d81QcA8jafMMPjYcbY8qTVn4f15BfH3QAsngKPTvcSViZGcIVlRDnQmXbfWGv2u19LnT4LdXpF06Iki3B2xyO0L/imY6dkO+vXbdUBmy6MjnR45kclciYhJQ469/SeaFM7yNKsNiWCu48TtHl4Ztld03px8UnhbzVpijj6o6Xu8IPsJdf2T+tBQ9pm8gPGDZa5I+4wOUpMitpQJ/s2o0KRhNRZCDMydzAvIvQrz5d4NlZIbT9KxA1Yeu6DTJYpVXNzg3ssTf/hfhnkesQG+Yn5A5a/eG9foGQUPCsf3ofjbNdGuKZ/rt8jA3UHia30fLcq2kGg== 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: Hi Mark On Fri, Oct 18, 2024 at 11:37=E2=80=AFAM Mark Brown wr= ote: > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > > On Fri, Oct 18, 2024 at 6:04=E2=80=AFAM Mark Brown = wrote: > > > > 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? > > Honestly I'd probably deal with this by refactoring such that the macro > isn't needed and the tests follow a pattern more like: > > if (ret !=3D 0) { > ksft_print_msg("$ACTION failed with %d\n", ret); > return false; > } > So expanding the macro to actually code ? But this makes the meal_test quite large with lots of "if", and I would rather avoid that. > when they encouter a failure, the pattern I sketched in my earlier > message, or switch to kselftest_harness.h (like I say I don't know if > the fork()ing is an issue for these tests). If I had to have a macro > it'd probably be something like mseal_assert(). > I can go with mseal_assert, the original macro is used by mseal_test itself, and only intended as such. If changing name to mseal_assert() is acceptable, this seems to be a minimum change and I'm happy with that. > > > 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 > > .... > > > > 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: > > No, I'm saying that having the line numbers is a problem. > > > 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 > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split: > line:2573" which automated systems aren't going to be able to associate > with the passing "test_munmap_free_multiple_ranges_with_split", nor with > 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. 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. > > 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. > > 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 > elegant for test cases which need to enumerate system features (which I > don't think is the case for mseal()?) and that it runs each test case in > a fork()ed child which can be inconvenient for some tests. The use of > fork() is because that makes the overall test program much more robust > against breakage in individual tests and allows things like per test > timeouts. OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. 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); The example [1], isn't clear about that. https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example Thanks