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 58E37C76188 for ; Mon, 3 Apr 2023 16:10:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8DABD6B0075; Mon, 3 Apr 2023 12:10:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 88A126B0078; Mon, 3 Apr 2023 12:10:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 752266B007B; Mon, 3 Apr 2023 12:10:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 66DB16B0075 for ; Mon, 3 Apr 2023 12:10:53 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id E664A1205CF for ; Mon, 3 Apr 2023 16:10:52 +0000 (UTC) X-FDA: 80640568344.27.B2F3D64 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf04.hostedemail.com (Postfix) with ESMTP id 8E08540017 for ; Mon, 3 Apr 2023 16:10:50 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=e79NsHBs; spf=pass (imf04.hostedemail.com: domain of peterx@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680538250; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=w+vX9YUXBW5zW75XHIINf4DIrUK+/z/c+v9mTBOLNso=; b=0WsJLM8X/Ja6q2eLRVUat5oxCtRg9NWebQdMiku3Qc5Vp8VkzkqpG+CAVFToken3FCbiWf fFAH15lZ3VlL441ROFy5GbI0osh35vxZmNxj32x06gfxYpyP44W8zSXEWe8WJEy0+i++mA ICHB3GKu15n6bUZoBZjSuEY8zhb+99k= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=e79NsHBs; spf=pass (imf04.hostedemail.com: domain of peterx@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680538250; a=rsa-sha256; cv=none; b=NSO7/hPr9boMUJNUWBMpq/JF9kvsdjPH0DiBJKoJmUtvhAjDH1gLJA1L2sDInZz2LbOVpG SoPpt6R8Z6fTGaLl4yEaKVyPNbUCVzKUXMwHeI4+N6b27tLBd9KClKssVebuihBYe+QUKP Pooxw5OaxrH2ma2hOZFWfDk+efl2P7g= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680538249; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=w+vX9YUXBW5zW75XHIINf4DIrUK+/z/c+v9mTBOLNso=; b=e79NsHBsL9J6iuP22zjdVvRTposh7kJbpdM01zCh5+DA/tX+JU/1k1XEHplF8ReQ3VX3kN ycowcog8iMbCs73SlSpn9wa7JRiyOtmgQHuXLcpUkBnxcM84BHQzjw2y/NcVf87CCwga39 dvW6hEpoWIUbFAtdNsaNCKh1apRR8bY= Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-34-Cp2h8PV8PtuXiniH0cxxQg-1; Mon, 03 Apr 2023 12:10:48 -0400 X-MC-Unique: Cp2h8PV8PtuXiniH0cxxQg-1 Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-3e1522cf031so9293651cf.1 for ; Mon, 03 Apr 2023 09:10:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680538246; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=PRUf4mF4hTo23LsJoejT2omq992QGUjosmoknsLXmlQ=; b=2KvoCxSMkQj0bArTtwY5yMFgkZA22BVDfdkEuTc6+84ApigY9xCIc3HkfE8Vwb2dm8 3xYKvOp48ijOTVy5k+hUkfCk6M+ffr3YcX9zuhhzp5hpn//S3BBpTl2BoMOaGnv0f3ed QtNoSlCVLA1HkhfSiq+mNrCsclEZwQXRg0SvgSQPVR+AQDmFFhUcLLUNTO64nnEek1lu AgIB+1ke4iEACj2ulGxxS8ECLEu7Ea0z4B44ZEFqNY8nyqCcmTgi/am8Rrl78PRXWyij f+p1uGycafD09UvUO5co69gQ1OjqSEbjcO/WFvKEbnQFSEf1ObEmSGDPhKXU0DwFhpPQ HF4g== X-Gm-Message-State: AAQBX9f9SY/N+z55aAR7DXzzXNKM9g1DSXLZa2ysgkZ9URc2yIOIF2Ht ukb2L88r5TBZymIy4jOIz9tl/FE3PtL9YrGoCDoGXRYtKlTvHKXYD3o810oMDXn8t0FK3vXijYb Mag0icUyVRNGWZWiOCg0= X-Received: by 2002:a05:622a:1802:b0:3e2:4280:bc58 with SMTP id t2-20020a05622a180200b003e24280bc58mr25368990qtc.3.1680538246205; Mon, 03 Apr 2023 09:10:46 -0700 (PDT) X-Google-Smtp-Source: AKy350ZrAEJ2CRWprD2fD+HlTJnehDWaAzKnA4S0xNyGtrumwgreRZFynUIy7Ewy6y8KrmqQFx/cYA== X-Received: by 2002:a05:622a:1802:b0:3e2:4280:bc58 with SMTP id t2-20020a05622a180200b003e24280bc58mr25368944qtc.3.1680538245759; Mon, 03 Apr 2023 09:10:45 -0700 (PDT) Received: from x1n (bras-base-aurron9127w-grc-40-70-52-229-124.dsl.bell.ca. [70.52.229.124]) by smtp.gmail.com with ESMTPSA id v4-20020ac873c4000000b003e29583cf22sm2588994qtp.91.2023.04.03.09.10.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Apr 2023 09:10:44 -0700 (PDT) Date: Mon, 3 Apr 2023 12:10:43 -0400 From: Peter Xu To: David Hildenbrand Cc: Axel Rasmussen , Mike Kravetz , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Andrea Arcangeli , Leonardo Bras Soares Passos , Mike Rapoport , Nadav Amit Subject: Re: [PATCH 10/29] selftests/mm: Test UFFDIO_ZEROPAGE only when !hugetlb Message-ID: References: <20230330155707.3106228-1-peterx@redhat.com> <20230330160714.3106999-1-peterx@redhat.com> <20230331183726.GD12460@monkey> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="/4lLZ6sttUXyfTXJ" Content-Disposition: inline X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: ja9trp8ze7q4q7qsxx65pumfnt8wo4wo X-Rspamd-Queue-Id: 8E08540017 X-HE-Tag: 1680538250-23028 X-HE-Meta: U2FsdGVkX18QtGImGrEfBcP32UfBAah3vzp0RkTU52wXQghOU4YY8IzDwzA6xuZ4mZSNdGhqChLey+FIofprnX/qE7ZTqBJe7cznCk/kSNdwOydg28LdkgiDziYx3g9uq9i2qt3tVyAirqYyupNPXTFdgTVHQodtVXU5kAuk5Yt7DsGxGvverfbxuT4idqVPBFArm1aMhWivl84IvKTIJnXwvxQ+s98H8junrnrydo5jew2/dG54GDTN4rI5/XiG3zntxULvTqCxGVjCnymwha1XgHv1Zs73g1HS6gDWXzW24kkMhtOqpMergsp24/WwgnPW+PMjxK9EXCNB3bY2SEalXO3KOj7wYy2Ww8qFpSqrfyU/nFxCTvqV9QTWs0GuQv90obm03pnOOwYlqVeflakTjXcJP6hd/48NLxsEJygad0choUrjXOitQP80ELMCCOrGMsQF/os0sr3WPlib+qBVIilfCDFk9RCPZ6s9eE3pc5c/ZbfsmqEbjzI/g6d38T2LVchYorvBDTuoi/sUl+a0ezzJR3sAZg4pz++1K0gf09mFbZ7RvQx/p9K7vd8P0EkKKQhpucWVzRNjD3GqMn4PrJqEvyIZehQeUdKPfmQrjwYjejn61/RWiEhAvMZudEI1HIhOtRrKxwKcmpddkbkjgWUrCj0m25eJ1LsjytPnqXM19uF3FRcBVBv3Hb1qQm/fmTKOuQuWMKn2F8KhsCYEFHA8l6sku8UagQgdOcvkrlbPRxLIQWwCTKSlwjyK/eKEWq/i28wm5nJIDE2Rx2Xj4vs8lVLiCeF4l2XFdzrqg5n56Zo4QEGCKJnlLvn+BuZiokTCL64cSrG1lMnbYELbdyEtcoCTcCIgPmm9CnuPGY0h+Cp+R+3Iylp0jI0LVnLEyQ9NDdk5zSJbL/j0gs7Linj95eQmtazRUt10o2ghXzeOGmUop/hpfgTuKcfptPhW79tKxg9MPHV9eeu wh62+p8K UER4v1MdTlRB6XI6mqKJq+aXLJFDPgi10iUFTXZ9MX9eTGHI/Dw2pxeQvHcdcVTCpkM3VpK5Vxi4YJH+p3OBwxee1vjry/FnxUKXDW+QGxNrwaMDYd8HVGyU6pSb9QFwHTwRxBf/E+780MjEyJoagNLFW56p7T/q9n7E0VeaKQe55IoUvA11eekrsaySgemc5vFKQX+UWUk3fgf2iJtOilcE792HgzLeiLiKBkbd4FZsRkKT/nLfimQ+K0VBS+acjURXMKq/vRHM2bJT3duiZ6qO8NSbKgA1mS+HB+xMsXG14MfVGgYzotdME7uEsrBxfed3FpMuetmQfVT8MvNVVvJIkafNarPavl/k+CE6vjGCLn8eyDvVbvnIpNqg2zk/w9SxzuxRFU7qB3LCaOIr49LbQqFUujg5eqpwsWiSFJ/N/TwgNVcYvBBp5xc9n2aJ4JAP65hjcaKCZCQQiLehA2JZHFb7s5iifEKrtu2wwMgxNd34lZbUSwNXCGy1dxUx0cgnPu9H6qiNiII8y7dExml9oYDY8HZcqKog8GDk7B8V/xfJis/5qqg5DNl3eDalZw460YJ1YVGhxYFI7UsKR/0WEU4KL+xSBvkJlMgCj0DbYmXcK/rBJj9IB50/t9ZLHsAn4U5cpyS5YMZVwc4Bkr7zitcK01TeS5o6j 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: --/4lLZ6sttUXyfTXJ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Mon, Apr 03, 2023 at 09:55:41AM +0200, David Hildenbrand wrote: > On 01.04.23 03:57, Axel Rasmussen wrote: > > On Fri, Mar 31, 2023 at 11:37 AM Mike Kravetz wrote: > > > > > > On 03/30/23 12:07, Peter Xu wrote: > > > > Make the check as simple as "test_type == TEST_HUGETLB" because that's the > > > > only mem that doesn't support ZEROPAGE. > > > > > > > > Signed-off-by: Peter Xu > > > > --- > > > > tools/testing/selftests/mm/userfaultfd.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/tools/testing/selftests/mm/userfaultfd.c b/tools/testing/selftests/mm/userfaultfd.c > > > > index 795fbc4d84f8..d724f1c78847 100644 > > > > --- a/tools/testing/selftests/mm/userfaultfd.c > > > > +++ b/tools/testing/selftests/mm/userfaultfd.c > > > > @@ -1118,7 +1118,7 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) > > > > { > > > > struct uffdio_zeropage uffdio_zeropage; > > > > int ret; > > > > - bool has_zeropage = get_expected_ioctls(0) & (1 << _UFFDIO_ZEROPAGE); > > > > + bool has_zeropage = !(test_type == TEST_HUGETLB); > > > > > > It is true that hugetlb is the only mem type that does not support zeropage. > > > So, the change is correct. > > > > > > However, I actually prefer the explicit check that is there today. It seems > > > more like a test of the API. And, is more future proof is code changes. > > > > > > Just my opinion/thoughts, not a strong objection. > > > > I agree. The existing code is more robust to future changes where we > > might support or stop supporting this ioctl in some cases. It also > > proves that the ioctl works, any time the API reports that it is > > supported / ought to work, independent of when the *test* thinks it > > should be supported. > > > > Then again, I think this is unlikely to change in the future, so I > > also agree with Mike that it's not the biggest deal. > > As there were already discussions on eventually supporting UFFDIO_ZEROPAGE > that doesn't place the shared zeropage but ... a fresh zeropage, it might > make sense to keep it as is. Thanks everyone. So here the major goal is to drop get_expected_ioctls(), and I think it's really unwanted here. Besides it's a blocker for split the test in a clean way, a major reason is get_expected_ioctls() fetches "wheter we support zeropage for this mem" from UFFD_API_RANGE_IOCTLS, rather than from the UFFDIO_REGISTER anyway: uint64_t get_expected_ioctls(uint64_t mode) { uint64_t ioctls = UFFD_API_RANGE_IOCTLS; if (test_type == TEST_HUGETLB) ioctls &= ~(1 << _UFFDIO_ZEROPAGE); if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp)) ioctls &= ~(1 << _UFFDIO_WRITEPROTECT); if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor)) ioctls &= ~(1 << _UFFDIO_CONTINUE); return ioctls; } It means it'll succeed or fail depending on what kernel we run this test on, and also on what headers we compile the test against. I actually mentioned some of the reasoning in a follow up patch (sorry maybe the split here caused some confusion): selftests/mm: uffd_[un]register() Add two helpers to register/unregister to an uffd. Use them to drop duplicate codes. This patch also drops assert_expected_ioctls_present() and get_expected_ioctls(). Reasons: - It'll need a lot of effort to pass test_type==HUGETLB into it from the upper, so it's the simplest way to get rid of another global var - The ioctls returned in UFFDIO_REGISTER is hardly useful at all, because any app can already detect kernel support on any ioctl via its corresponding UFFD_FEATURE_*. The check here is for sanity mostly but it's probably destined no user app will even use it. - It's not friendly to one future goal of uffd to run on old kernels, the problem is get_expected_ioctls() compiles against UFFD_API_RANGE_IOCTLS, which is a value that can change depending on where the test is compiled, rather than reflecting what the kernel underneath has. It means it'll report false negatives on old kernels so it's against our will. So let's make our live easier. But I do agree that it's helpful to keep a test against uffdio_register.ioctls in this case against _UFFDIO_ZEROPAGE, so it can be detected dynamically. IOW, even if we would like to avoid "test != HUGETLB" here, at least we should like to fix that with the UFFDIO_REGISTER results. Here's my offer below. :) Could I keep this patch as-is (as part of getting rid of get_expected_ioctls() effort; I can squash this one into "selftests/mm: uffd_[un]register()" if any of you think proper), meanwhile I'll squash a fixup to the "move zeropage test into uffd-unit-tests" explicitly check uffdio_register.ioctls in the same patchset? IOW, we'll have a few test commits missing this specific ioctl test, but then we'll have a better one dynamically detected from the kernel. The fixup patch attached. I think it'll automatically work when someone would like to introduce UFFDIO_ZEROPAGE to hugetlb too, another side benefit is I merged the zeropage test into one, which does look better too. Thanks, -- Peter Xu --/4lLZ6sttUXyfTXJ Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="0001-fixup-selftests-mm-Move-zeropage-test-into-uffd-unit.patch" >From 5b06f921cf8420600c697a3072a1459a5cb4956b Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 3 Apr 2023 11:57:07 -0400 Subject: [PATCH] fixup! selftests/mm: Move zeropage test into uffd unit tests Signed-off-by: Peter Xu --- tools/testing/selftests/mm/uffd-unit-tests.c | 62 +++++++++++--------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 793931da5056..247700bb4dd0 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -711,54 +711,58 @@ static bool do_uffdio_zeropage(int ufd, bool has_zeropage) return false; } +/* + * Registers a range with MISSING mode only for zeropage test. Return true + * if UFFDIO_ZEROPAGE supported, false otherwise. Can't use uffd_register() + * because we want to detect .ioctls along the way. + */ +static bool +uffd_register_detect_zp(int uffd, void *addr, uint64_t len) +{ + struct uffdio_register uffdio_register = { 0 }; + uint64_t mode = UFFDIO_REGISTER_MODE_MISSING; + + uffdio_register.range.start = (unsigned long)addr; + uffdio_register.range.len = len; + uffdio_register.mode = mode; + + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) + err("zeropage test register fail"); + + return uffdio_register.ioctls & (1 << _UFFDIO_ZEROPAGE); +} + + /* exercise UFFDIO_ZEROPAGE */ -static void uffd_zeropage_test_common(bool has_zeropage) +static void uffd_zeropage_test(void) { - if (uffd_register(uffd, area_dst, page_size, - true, false, false)) - err("register"); + bool has_zeropage; + int i; + has_zeropage = uffd_register_detect_zp(uffd, area_dst, page_size); if (area_dst_alias) - if (uffd_register(uffd, area_dst_alias, page_size, - true, false, false)) - err("register"); - - if (do_uffdio_zeropage(uffd, has_zeropage)) { - int i; + /* Ignore the retval; we already have it */ + uffd_register_detect_zp(uffd, area_dst_alias, page_size); + if (do_uffdio_zeropage(uffd, has_zeropage)) for (i = 0; i < page_size; i++) if (area_dst[i] != 0) err("data non-zero at offset %d\n", i); - } + if (uffd_unregister(uffd, area_dst, page_size)) + err("unregister"); - if (uffd_unregister(uffd, area_dst, page_size * nr_pages)) + if (area_dst_alias && uffd_unregister(uffd, area_dst_alias, page_size)) err("unregister"); uffd_test_pass(); } -static void uffd_zeropage_test(void) -{ - uffd_zeropage_test_common(true); -} - -static void uffd_zeropage_hugetlb_test(void) -{ - uffd_zeropage_test_common(false); -} - uffd_test_case_t uffd_tests[] = { { .name = "zeropage", .uffd_fn = uffd_zeropage_test, - .mem_targets = MEM_ANON | MEM_SHMEM | MEM_SHMEM_PRIVATE, - .uffd_feature_required = 0, - }, - { - .name = "zeropage-hugetlb", - .uffd_fn = uffd_zeropage_hugetlb_test, - .mem_targets = MEM_HUGETLB | MEM_HUGETLB_PRIVATE, + .mem_targets = MEM_ALL, .uffd_feature_required = 0, }, { -- 2.39.1 --/4lLZ6sttUXyfTXJ--