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 C176EEB64D9 for ; Wed, 14 Jun 2023 18:00:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1D7AA6B0074; Wed, 14 Jun 2023 14:00:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 188658E0002; Wed, 14 Jun 2023 14:00:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 04F8A8E0001; Wed, 14 Jun 2023 14:00:36 -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 E91566B0074 for ; Wed, 14 Jun 2023 14:00:36 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id B4D801C861A for ; Wed, 14 Jun 2023 18:00:36 +0000 (UTC) X-FDA: 80902118472.11.8ED618E Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by imf15.hostedemail.com (Postfix) with ESMTP id 2C1AEA0023 for ; Wed, 14 Jun 2023 18:00:31 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=PWFmQmN9; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of emmir@google.com designates 209.85.208.43 as permitted sender) smtp.mailfrom=emmir@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686765632; 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=GgxJlLA8wLn/M0aAHTm/1/LrgoyOzeZsPSKgsschXV8=; b=D1Au+SMNslBFRF7qQtYik+UkT1Pz2+5Xop7uQ+OAZZs3o/eJpotvLCQOWouCRJWKhm0w9c LMuyQsQFIO1pa8fS5JR+8SXHrlkeG3h8GVENl15pCk4m/Ql9iECNskyOxlG5eqkL7puT6o u4SXL9EN29S9OTefL/fpt93H6U1/zyg= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=PWFmQmN9; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of emmir@google.com designates 209.85.208.43 as permitted sender) smtp.mailfrom=emmir@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686765632; a=rsa-sha256; cv=none; b=VyhDFRAuF4WKFz1waBadSwEgzn890oSdQ4QpgaaU1kJOZB9BMI3oWJDswh6YwXAmyOiheb P8Dja1J4CE+eOPi7qXoljWgLTNCpPqmBEWZKnJeBjQfuAMr0h2xtaWvM6cHF5Jlk3stQ++ FI34J0NM6FKO882WcBTNNB5KcVAOCwo= Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-516500163b2so954a12.1 for ; Wed, 14 Jun 2023 11:00:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1686765630; x=1689357630; 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=GgxJlLA8wLn/M0aAHTm/1/LrgoyOzeZsPSKgsschXV8=; b=PWFmQmN98IB62twE2ZU+8m/C/KOvlENeQx/4VYYwQSk4EI3Ii4ll5KzI1JKLeJ4Ef4 qX64TsKn9VfLtprU93X96UtDc7rwUlDkJeChFBv0tiLQQY0Hj9KrSJhLhIyl7NfI+6mM 9lN3g7xv7vIUXpHMoM0iOkvwU779rc7tjxg8mbTF6HTDUj5oV5O87RP6hVn7QvXm/tD3 3bEzTb33hGiuNA10HMvsGmWtZAi3ZRjev56jROLxTGZHNOgZT6G1VAbrK4wMHvYP8eC8 VI3oYYBQqo5V4zk9PYPUleqgiLkKLGvqhivf4S4ONcsjpYyoPFQ0Qyq/bVKL0RnBpdqC 6EpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686765630; x=1689357630; 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=GgxJlLA8wLn/M0aAHTm/1/LrgoyOzeZsPSKgsschXV8=; b=KDACBbURMabaZ7M3yXEsCFDDB7Y3ImEPiIDByhYPH7nzGF1DXAx1lSbaoMEbt0FB5P lvyC5GNXJCy2csS7Pp/55DcDGC9bnfDNsCUoq6E4UYeQlcS2CS1Hhf21/EQ6pf0EgF76 I9bipyK3FrnCf9QrO2RjyTZqxlKL+5jrmd5h0TdZN11+/qaY/Tzeyru6RRW2Dy5KcY8/ M8GN4d/3bMJCoWwioWHkGQpWQvan1S0GguR24WUJOO/4h6pYqgkJ9lKGUyZlcGawW750 rJtvioa4/ozRjy258XL6vKVXlyz3+hDBDbvbM9ZZQo/pGcki4pxn65dBLIC6xJzS0lIP I/WA== X-Gm-Message-State: AC+VfDykc2q6LIDEZO9HoK2BlOl41khEF6aGsPJhVi4qIUH9eNkQH858 5ybfLuNq0W7Zexi8iy/jqeNYHRuuelWewhGQx4R7LA== X-Google-Smtp-Source: ACHHUZ54WuluU7EgpzhjBSJClrf7OTSFZ7FwHmH45AFMEak4zwz7jxM7srJNVGtkYL6NsNcglFQGcHERY54JY8l3kW4= X-Received: by 2002:a50:9b17:0:b0:51a:1d77:e69d with SMTP id o23-20020a509b17000000b0051a1d77e69dmr2540edi.3.1686765630113; Wed, 14 Jun 2023 11:00:30 -0700 (PDT) MIME-Version: 1.0 References: <20230613102905.2808371-1-usama.anjum@collabora.com> <20230613102905.2808371-3-usama.anjum@collabora.com> <0db01d90-09d6-08a4-bbb8-70670d3baa94@collabora.com> <34203acf-7270-7ade-a60e-ae0f729dcf70@collabora.com> In-Reply-To: <34203acf-7270-7ade-a60e-ae0f729dcf70@collabora.com> From: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Date: Wed, 14 Jun 2023 20:00:18 +0200 Message-ID: Subject: Re: [PATCH v18 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs To: Muhammad Usama Anjum Cc: Peter Xu , David Hildenbrand , Andrew Morton , Andrei Vagin , Danylo Mocherniuk , Paul Gofman , Cyrill Gorcunov , Mike Rapoport , Nadav Amit , Alexander Viro , Shuah Khan , Christian Brauner , Yang Shi , Vlastimil Babka , "Liam R . Howlett" , Yun Zhou , Suren Baghdasaryan , Alex Sierra , Matthew Wilcox , Pasha Tatashin , Axel Rasmussen , "Gustavo A . R . Silva" , Dan Williams , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Greg KH , kernel@collabora.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: xrbyknxguqj7ic93b1eyqsiyqgta5q6e X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 2C1AEA0023 X-HE-Tag: 1686765631-228481 X-HE-Meta: U2FsdGVkX18gAU4HJRCeoFsEfiilNMZazjLJtC4/x0fWlNgH7lz9alV+6sF9cXVleXYMldqlWmKZ0ZrdUWR1llNlcxn6FrCy6gtY4zJI1hK9hkvAXySVo7G3bx+YUtfYZAswzYw6GUt5MQiNBkyoF2D3OkPR8dBnWlUez2sA46SR4tXfpb/F+1ov2NxKENGVCDOejnog3KQftsh3lNexBL9vDvZI2rrDG56Yw3Jvdg53UDpAzI5mGf/6eicJXrcIcso1LbX1nXxlzRJqIdUMDpyO4cd9dHgKG3rjmiNvwckMAI8CyMukkyfCaxhEsMWDCbwbuQ/qX9n70huoFEdjzA4vbavlXtZS21F4QhZWs7to/ywFqP7lCxGsXKBwODmavEA7zI3F0YSJq+IMZa0Q9HMpQ7KsAA6b8sZAs7Ch3h2Yvr7WNK5IIlQUGOd5r9LbNh+wt24UZNhVjKhgLBsJl+zC6ZqStqFdsDZmP3uUPPlmaqoWuDgtwRRsF+8W755tHVBCytOc1lRA6nJqQvcIsCNoF4xqOBtmvPGQFyk3WhrIJcrymcTzw0cVWSAJWQwt98e8PnOu7Pqhm31XimyLrJKDh8eE4F7nwK7pA6AUzrdaprk2kGs4t3j9BuR4fpaQTakG00aTtbB2VKGNef1DAhQA0gqch026M4H/ejFPrQvna1gZBdBr5hvbyW10N5J6Y9ebM4qu/8mFJBZ2KKwbNW0OfRSwpGllBkeOSFDLF1DaCLnWG3bY9uJGbJfkw6/IkHANNHmw49AQoHV+kZbCo7nBQsZZDFsMAklacmqDl8X2VhMqT/goP6sNj9dGd5mQ5kVClv+7WkdSPs+7BVOZv1bPrM2neha8M85e+KF/MFY4rq2XMFUKSOgLCp09v4O+quA9EkN3KOqqyjHuW3ZYa1xdeAuwUV3QwxDomWoT89KSDI51ejXPTIpFX7e4Owg8j14Dn3DBCr/mTd5HYKs b63pbwkq iGk1Z9UkBlYuBgff8wcyBFdTdTyApe4xhJ3SBULOd83U3mGoOb32loaM+K2GNVn9HY0bIB4h6CXJNKckLMhFZG9nwC4UJ8/nQBW5RdavsPLbfGWwvUhQ+F3qc/gGjbYA0Q4leNnLVbccB+kO4LRao8CDw6Y4I3/dRMTSg5PlLHiJNXMt65hsLqkO/RBueX8DyymQHor420RnRPA9ZgXL5w3EgnEUOw9Qg47DnWgmm8rJm20dZEBaUfS+S8fthA+qxEvfqaFWmwfjR9TF18jSFR1Ro2f8/xyO2SEpN+nnlSLTGsocjVTgtWZWz2yezZ+/KJIpL0ihIv6mHRyfWHaQvDiaIHDJPnMUJPHNA65kqAfgQZb8E8sw9heflb/ZKolnSQyKYGDeMu+Ai7qCkZLl0SYUfvA== 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: (A quick reply to answer open questions in case they help the next version.= ) On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum wrote: > On 6/14/23 8:14=E2=80=AFPM, Micha=C5=82 Miros=C5=82aw wrote: > > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum > > wrote: > >> > >> On 6/14/23 3:36=E2=80=AFAM, Micha=C5=82 Miros=C5=82aw wrote: > >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum > >>> wrote: [...] > >>>> + if (cur_buf->bitmap =3D=3D bitmap && > >>>> + cur_buf->start + cur_buf->len * PAGE_SIZE =3D=3D addr) { > >>>> + cur_buf->len +=3D n_pages; > >>>> + p->found_pages +=3D n_pages; > >>>> + } else { > >>>> + if (cur_buf->len && p->vec_buf_index >=3D p->vec_buf= _len) > >>>> + return -ENOMEM; > >>> > >>> Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel > >>> ran out of memory when allocating, not that there is no space in a > >>> user-provided buffer. > >> There are 3 kinds of return values here: > >> * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort th= e > >> page walk from next entry > >> * 0 ---> continue the page walk > >> * -ENOMEM --> Abort the page walk from current entry, user buffer is f= ull > >> which is not error, but only a stop signal. This -ENOMEM is just > >> differentiater from (1). This -ENOMEM is for internal use and isn't > >> returned to user. > > > > But why ENOSPC is not good here? I was used before, I think. > -ENOSPC is being returned in form of true error from > pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it > wasn't true error here, it was only a way to abort the walk immediately. > I'm liking the following erturn code from here now: > > #define PM_SCAN_BUFFER_FULL (-256) I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be: bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit In this setup, `is_interesting_page() && (n_out =3D output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out =3D=3D 0` then no pages fit and no WP is needed (no other special cases). > >>> For flags name: PM_REQUIRE_WRITE_ACCESS? > >>> Or Is it intended to be checked only if doing WP (as the current name > >>> suggests) and so it would be redundant as WP currently requires > >>> `p->required_mask =3D PAGE_IS_WRITTEN`? > >> This is intended to indicate that if userfaultfd is needed. If > >> PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if > >> userfaultfd has been initialized for this memory. I'll rename to > >> PM_SCAN_REQUIRE_UFFD. > > > > Why do we need that check? Wouldn't `is_written =3D false` work for vma= s > > not registered via uffd? > UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory regio= n > for it to report correct written values on the memory region. Without UFF= D > WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP stat= e > undefined. If user hasn't initialized memory with UFFD, he has no right t= o > set is_written =3D false. How about calculating `is_written =3D is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered. > > While here, I wonder if we really need to fail the call if there are > > unknown bits in those masks set: if this bit set is expanded with > > another category flags, a newer userspace run on older kernel would > > get EINVAL even if the "treat unknown as 0" be what it requires. > > There is no simple way in the API to discover what bits the kernel > > supports. We could allow a no-op (no WP nor GET) call to help with > > that and then rejecting unknown bits would make sense. > I've not seen any examples of this. But I've seen examples of returning > error if kernel doesn't support a feature. Each new feature comes with a > kernel version, greater than this version support this feature. If user i= s > trying to use advanced feature which isn't present in a kernel, we should > return error and not proceed to confuse the user/kernel. In fact if we lo= ok > at userfaultfd_api(), we return error immediately if feature has some bit > set which kernel doesn't support. I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose? > >>> [...] > >>>> --- a/include/uapi/linux/fs.h > >>>> +++ b/include/uapi/linux/fs.h > >>>> +/* > >>>> + * struct page_region - Page region with bitmap flags > >>>> + * @start: Start of the region > >>>> + * @len: Length of the region in pages > >>>> + * bitmap: Bits sets for the region > >>> > >>> '@' is missing for the third field. BTW, maybe we can call it > >>> something like `flags` or `category` (something that hints at the > >>> meaning of the value instead of its data representation). > >> The deification of this struct says, "with bitmap flags". Bitmap was a > >> different name. I'll update it to flags. > > > > From the implementation and our discussions I guess the > > `bitmap`/`flags` field is holding a set of matching categories: a bit > > value 1 =3D pages are in this category, value 0 =3D pages are not in th= is > > category. > > > >>>> +/* > >>>> + * struct pm_scan_arg - Pagemap ioctl argument > >>>> + * @size: Size of the structure > >>>> + * @flags: Flags for the IOCTL > >>>> + * @start: Starting address of the region > >>>> + * @len: Length of the region (All the pages in this = length are included) > >>> > >>> Maybe `scan_start`, `scan_len` - so that there is a better distinctio= n > >>> from the structure's `size` field? > >> As start and len already communicate the meaning. We are making things= more > >> verbose. > > > > We are describing (in the name) only that it is a range, but not of > > what or what purpose. That information is only in the docstring, but > > it is harder to get by someone just reading the code. > Agreed. But I'm using same names, start and len which mincore (a historic > syscall) is using. I've followed mincore here. mincore() doesn't take parameters as a struct, but as three positional arguments (whose names don't matter nor appear at call point) - I wouldn't take it as a precedent for structure field naming. > >>>> + * @vec: Address of page_region struct array for outp= ut > >>>> + * @vec_len: Length of the page_region struct array > >>>> + * @max_pages: Optional max return pages > >>>> + * @required_mask: Required mask - All of these bits have to be= set in the PTE > >>>> + * @anyof_mask: Any mask - Any of these bits are set= in the PTE > >>>> + * @excluded_mask: Exclude mask - None of these bits are set in= the PTE > >>>> + * @return_mask: Bits that are to be reported in page_region > >>>> + */ > >>> > >>> I skipped most of the page walk implementation as maybe the comments > >>> above could make it simpler. Reading this patch and the documentation > >>> I still feel confused about how the filtering/limiting parameters > >> I'm really sad to hear this. I've been working on making this series f= rom > >> so many revisions. I was hopping that it would make complete sense to > >> reviewers and later to users. > >> > >> What do you think is missing which is restricting these patches gettin= g > >> accepted to upstream? > >> > >>> should affect GET, WP and WP+GET. Should they limit the pages walked > >>> (and WP-ed)? Or only the GET's output? How about GET+WP case? > >> The address range needs to be walked until max pages pages are found, = user > >> buffer is full or whole range is walked. If the page will be added to = user > >> buffer or not depends on the selection criteria (*masks). There is no > >> difference in case of walk for GET, WP and GET+WP. Only that WP doesn'= t > >> take any user buffer and just WPs the whole region. > > > > Ok, then this intent (if I understand correctly) does not entirely > > match the implementation. Let's split up the conditions: > > > > 1. The address range needs to be walked until max pages pages are found > > > > current implementation: the address range is walked until max pages > > matching masks (incl. return_mask) are reported by GET (or until end > > of range if GET is not requested). > > Maybe we need to describe what "found" means here? > Found means all the pages which are found to be fulfilling the masks and = we > have added it to the user buffer. I can add the comment on top of > pagemap_scan_private struct? But I don't think that it is difficult to > understand the meaning of found_pages and also we compare it with max_pag= es > which makes things very easy to understand. After fixing `return_mask` and the selection/action split I think "pages found" might work - as now the count will be exactly what pages match the required/anyof/excluded criteria. > > 2. user buffer is full > > Matches implementation except in GET+WP edge cases. > I'm not sure which edge case you are referring to? Probably for hugetlb > error return case? Yes, that one. Best Regards Micha=C5=82 Miros=C5=82aw