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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 495A4EB64DC for ; Wed, 14 Jun 2023 18:00:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230131AbjFNSAh (ORCPT ); Wed, 14 Jun 2023 14:00:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36390 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229996AbjFNSAe (ORCPT ); Wed, 14 Jun 2023 14:00:34 -0400 Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DF648C7 for ; Wed, 14 Jun 2023 11:00:31 -0700 (PDT) Received: by mail-ed1-x52d.google.com with SMTP id 4fb4d7f45d1cf-51400fa347dso1149a12.0 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=FU3qkuE8NpMGyKZIhHCxTzdcWCXT6utQJ0TIb50OHEFtWcS/o3RuUsf0d8PdYQ63XB 13u8DyW0Ve+Ffoj9Bhcn44d0aAoQFrfDXTBN7YMF8GRNRQ4+J9h3Pb+4tA/eTS0h524Y WneFLUGVqns3dpv6N29yywa4tQ5Tt2QjLfpDxQ5kS5QB4fvTc1E+AFD/pfNBh0313lM7 ytU70ULPUK0vYpBorU35xaUMq3gLTQQtZO7RKwB2BTtlCbnqL6JRTIyjg3qXGMdtxxnB lYoipX3aGd8W47Dip7sAxySSwiJc5lCT0Y7ERHZQY3QHPnTgaGbM0C9jprtzAhqc42rM 2Inw== X-Gm-Message-State: AC+VfDzhFy4MnUWKwsINwd5GXUem20D8RyTAxkV7Q6L/p/t/UCYg6Y9d hptziBlPStVtSZ1bkCEnx8vD2RA8yVZQocES3H9DBw== 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (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