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 27CD9C76196 for ; Fri, 7 Apr 2023 09:42:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B1DF4900003; Fri, 7 Apr 2023 05:42:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id ACE3C900002; Fri, 7 Apr 2023 05:42:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 947E4900003; Fri, 7 Apr 2023 05:42:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 8240D900002 for ; Fri, 7 Apr 2023 05:42:41 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 0ADE6ACB64 for ; Fri, 7 Apr 2023 09:42:41 +0000 (UTC) X-FDA: 80654105322.05.CF3BAF0 Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by imf06.hostedemail.com (Postfix) with ESMTP id A6A1A180009 for ; Fri, 7 Apr 2023 09:42:38 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=ibm.com header.s=pp1 header.b=jp+hLqpG; spf=pass (imf06.hostedemail.com: domain of rppt@linux.ibm.com designates 148.163.158.5 as permitted sender) smtp.mailfrom=rppt@linux.ibm.com; dmarc=pass (policy=none) header.from=ibm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680860558; 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=qQyhNJNdHFXjk30Z2Yy0V7Voo81BtWkh809AYuBFthk=; b=cNj0BzcKEmGgZ3BggugbN5RbvNbHOc6uPddusl1iUnbsEbv1Y6H/070uqHzHslyHLDUPEi Ty/4rnHsqDzZI5aBumXVb/zwZnWJSWE4AgSvPciD/NSolVHQv84/Q4mDJcP1f471FxPsbT zsf+yzmewaijFiAoaTzcqpICovkN03U= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=ibm.com header.s=pp1 header.b=jp+hLqpG; spf=pass (imf06.hostedemail.com: domain of rppt@linux.ibm.com designates 148.163.158.5 as permitted sender) smtp.mailfrom=rppt@linux.ibm.com; dmarc=pass (policy=none) header.from=ibm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680860558; a=rsa-sha256; cv=none; b=ZDc27uh4T8Cj3nI2Q5yRpmvLKEDkbA09bCNVn2gepgIfA1B3Qvx5/dH2b5TERNvfvuZJ2x 51hMxgxOwCO1if94L1hJkCYK0rRZBglwYI5JHDxMicJnJi5pbMgQmES0mT9/fUE9zOym55 iLua3Yu2tt/pFtmXQ0oUgktKQt81SYY= Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3377cQKX021429; Fri, 7 Apr 2023 09:42:37 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : content-transfer-encoding : in-reply-to; s=pp1; bh=qQyhNJNdHFXjk30Z2Yy0V7Voo81BtWkh809AYuBFthk=; b=jp+hLqpGwvaIg5fjfaWo0Eo+oAvWMEbjv/KRhUxwdrvAPQaDlUZkk169x0SZSy9LuzwQ XaT1hm6+vX2jwYctEbS5TNU/Amc9Y2Ob7lAawybVNK2kQ2XBHUWpctNjSyEC5/c48jtk pTKWSwrQk9XSq8Ddg73NE2ilVThqUPTCUpMza3pPox1KIT/3/tGjllgPUja267YXVKXR X+xDLfJN2JnCqwMF2RBfQTpbWxSrPyU8aRCaWQXXPKxxieHDAAXZy23/oryjHK5UEzcY P7xlBswF7NH8jg/Un0thCTGzT9MaVaOaOVaAURDZPEpWbYRYnXULoXX77f1FcL6g5LM7 iA== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3pt9bb0drk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 07 Apr 2023 09:42:37 +0000 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 3379ga8q004752; Fri, 7 Apr 2023 09:42:37 GMT Received: from ppma03ams.nl.ibm.com (62.31.33a9.ip4.static.sl-reverse.com [169.51.49.98]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3pt9bb0dqx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 07 Apr 2023 09:42:36 +0000 Received: from pps.filterd (ppma03ams.nl.ibm.com [127.0.0.1]) by ppma03ams.nl.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 336JjpJm026329; Fri, 7 Apr 2023 09:42:34 GMT Received: from smtprelay04.fra02v.mail.ibm.com ([9.218.2.228]) by ppma03ams.nl.ibm.com (PPS) with ESMTPS id 3ppc874vq4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 07 Apr 2023 09:42:34 +0000 Received: from smtpav03.fra02v.mail.ibm.com (smtpav03.fra02v.mail.ibm.com [10.20.54.102]) by smtprelay04.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 3379gWQ744040558 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 7 Apr 2023 09:42:32 GMT Received: from smtpav03.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2B47E2004B; Fri, 7 Apr 2023 09:42:32 +0000 (GMT) Received: from smtpav03.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F15C120043; Fri, 7 Apr 2023 09:42:30 +0000 (GMT) Received: from linux.ibm.com (unknown [9.171.71.120]) by smtpav03.fra02v.mail.ibm.com (Postfix) with ESMTPS; Fri, 7 Apr 2023 09:42:30 +0000 (GMT) Date: Fri, 7 Apr 2023 12:42:29 +0300 From: Mike Rapoport To: Peter Xu Cc: David Hildenbrand , 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 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-TM-AS-GCONF: 00 X-Proofpoint-GUID: BCe5lz7u0fyPfJFkX7eMe75r4mOsQX7C X-Proofpoint-ORIG-GUID: JnxH7-LKgQJS7qckHiZO_RSmFi6K-Wma X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-04-07_04,2023-04-06_03,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 mlxlogscore=999 lowpriorityscore=0 clxscore=1015 impostorscore=0 malwarescore=0 phishscore=0 priorityscore=1501 spamscore=0 bulkscore=0 mlxscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2304070084 X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: e4qnq1xmqh9hpxdnjc67kz74tqo3tiky X-Rspamd-Queue-Id: A6A1A180009 X-HE-Tag: 1680860558-454057 X-HE-Meta: U2FsdGVkX18wp7/U0iAe4WpXHL5uBJ/apDz//q7zvTgkzjvLssyzZbx+mVVsYj9rgPc9mw4d9VZFsEfuXpCY1/xTHBX/BOguA4XXzBtVjbm3X9gtxK1M3LNrKGzEmvvIqUKtJpUq19V8GFQhvpYXEEamvGnzUt4VDLYnCqk+QtSqwL02eGQ3gHojnhCfGHjfgMM2t+I4VA0gHfIgylWARdz/5kar0KWXIDf0IHwg3YOEIhvs79wmr/cEA2o0jI29k2UBV8WRGDbuQgdhzppjtRrm+SrQvMWhdIa1DxWylrI/S/dPHSuJr6AJTJT8K1Ljto9QkCxoqDyCp5tmpYa8b3F3zygbv0b6Go+mkepYvby3dpJdxCguJUmfH05hV9gurpWgu7SsF4mQZmnXQoEXHcIsgprvfmO3dj8JozUfFGMdHDPNJ9bn2FlxwAS6oEpyxD6+MqnBqAv9UCxyyAGRk191oR4bHD5FJnvhm4dj1dqEm+ieCfrvOMUvbEdtiE8fNfuz8n/KNXM4ywqBuJwc0SjRViFbLK+Rqi4EkBhCtUTeXDR5ABT/GcMSoOOrN7GLtPd+EyQ3kKxlFR2AU3SSjx+6p3bxUyYfWNDQFFMu5+cXrLQxE9vj+yyj8iRtr/2KOCszrAlTGjDvffhDM8Rwrv7aFmr7k1pFg5RZHQ1XFM4HJ1LU5nUDfzXRKjdORdGPi5oMv4rjsr2Gn3G8eoqTIsiSdEvjLqsiPw3vFpwbx4c3jE0PBm5pXk5bfb5/zCnq+Zz6nPhfZ9x2ea6/ts2BUNf6qi7LG7Fskn6c5wqZi0C/lVytDzgQEvowtByNI2bQUV/JLbAfu5VCUkbwNPu4Rh2AwsoZqIeTUp29njFtOhwHjqylowNo+nV56rQTLlwzkj32DgLBFIVtP5lSA0jGhd06+xU8tfC2BndCi7N4IXW8doBIZFiQAJvTAbS3qqwr0ZyKcAgEzyiwWthhYpe twPBq1d+ qpK0zR/TLHwe4WomCmbl01qmM9p8ugftjrwwQq8HN188HZp1ocHdT8iIZTiemIMX27wAhx4+yZXt4DWgx6olkV5mCY/KK9eeKFwwEczl2WbRRwrEL2uKk1G4cXVATQItgLx76HZp0Rx8jZ27MMjSxF3quemr0bta2TLPuQpJzjyXUu2SifFPOMLUFJg9akood4/+XUIa8+Vb0wDN1zogJ5zR6Y7G9MU22srq6l95l6NzYyvG2HTr8hmvEddXQP8nvdu07wPCwsi05JBQYYtdAGBTmWgbiNtqKkHGMYbJ5DB8D+v9mrdmtURK46zZHUg4KV7CUhWzvHv0AKfb6JljRwcCyclq11f9WQTJQxNzftdJEbWw= 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: On Mon, Apr 03, 2023 at 12:10:43PM -0400, Peter Xu wrote: > 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. I agree that it makes sense. A nit below :) > Thanks, > > -- > Peter Xu > 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) Let's spell out 'zp' as zeropage, what do you say? > +{ > + 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 > -- Sincerely yours, Mike.