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 E4706EB64DB for ; Thu, 15 Jun 2023 13:58:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 483ED6B0072; Thu, 15 Jun 2023 09:58:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 40DD06B0074; Thu, 15 Jun 2023 09:58:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2860E8E0001; Thu, 15 Jun 2023 09:58:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 124636B0072 for ; Thu, 15 Jun 2023 09:58:40 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C571EC08D6 for ; Thu, 15 Jun 2023 13:58:39 +0000 (UTC) X-FDA: 80905137558.21.2A31DDF Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf30.hostedemail.com (Postfix) with ESMTP id 990F080005 for ; Thu, 15 Jun 2023 13:58:37 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=VxGFKchc; dmarc=pass (policy=quarantine) header.from=collabora.com; spf=pass (imf30.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686837518; a=rsa-sha256; cv=none; b=SXI2xZRP8if6R79Rv7Wtg2F7gH9A5AevEon3k0c5+yd/L9ueO9/oKngzDYyuTrrG79H043 PCP5TzaQEgOenzcpJFJU5gNfLtvP9mJTbak6JN95Bq/JzbzqQrb9126NTb9jbsuXWKEg8k 18XwD7wrWDQpcW0UMm+REiBPZ5oU6mU= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=VxGFKchc; dmarc=pass (policy=quarantine) header.from=collabora.com; spf=pass (imf30.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686837518; 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=gfIE9zMUm2ztF7FdPl2Z2NoY1MlznkIH+qyfY3u1jq4=; b=DEYwNsVa+UU4MchYgc/1ogZNggglpYtzeW4wchkHB8VTgl3czRyl8nsFmrNF1dzC3WgvZI BpFyjZGCnsq/gjSGK4kaQfagmpyCRWG8ybzZ6Ngw27f3pF3BaRCHhdwYlJXifqNUFmHVEV kj5NMb33fqTzUwDz2wbzv25W0F6gGWE= Received: from [192.168.10.55] (unknown [119.155.33.163]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: usama.anjum) by madras.collabora.co.uk (Postfix) with ESMTPSA id 389C96606F72; Thu, 15 Jun 2023 14:58:25 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1686837515; bh=kx5ol8dVQuFz6zHRXKupWEVKiLJuymrLWyiB/fDBI7E=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=VxGFKchcDHZtSZ54SX9G5nzFsUSpPQ/eo07jpAO3UTAnyX1g7QnEXgZddBIAbvl42 KcaP/xtuIrfeNOQdQC+msgJV0ZcYPvCZ9ySNfOBgGC6rlgiMzS/K8+XC15S914w7Ln iXloj8BpyouTnvSW89t+CUf4dgOxs6TCsbXOuy2yn3w3LQt43sKlgtD8KT4jy8Ff4q U/I5z9viy+FdoWFSPT0ZfVlK1NoP4dmS0ze36dg0VIVVkvVehm0TEcRlNPNyJASi5s iyQptO5a5+veE0YZgZUFWvETgcqllQwYMEP8LNL/ElrWeGKbd09n53/1onCCMOZdoB JGTUpXZQoAWkA== Message-ID: <96b7cc00-d213-ad7d-1b48-b27f75b04d22@collabora.com> Date: Thu, 15 Jun 2023 18:58:22 +0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Cc: Muhammad Usama Anjum , 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 Subject: Re: [PATCH v18 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs To: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= 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> Content-Language: en-US From: Muhammad Usama Anjum In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 990F080005 X-Stat-Signature: mozyp8diw619x8wabwy1ok1wixc87rri X-HE-Tag: 1686837517-421409 X-HE-Meta: U2FsdGVkX1+jIfCn8GdN0IG69cWnCiFloIlLSkg7nAQh8/IFchAJgB3R4f0K9R5d/kjdVCybttOWqH9EIHwFtGmvKS09X4jcittP1Rptb7OSYy79zYhAWWk+tuhQ2RJqVv5HGQj5WTj3EnyYsXC34MthxF355wol2B1qwy4qx5OIhezuHRiHAqjcdhFe6Gy16NhS5Fbp+JKx5Yj/bBlhjlFmXrTJPKkiNUOOFnCcHrc/44KUotRk390/pVyAfXlcKnk+NaYAn+5SUvRsX2K+C32q42Q56wFnCcyquh36TUNcb/sfd7qLtOwYDVO0r0EDYs93qEBwzQa9Qb6o307+wtV8UMaMF4lOFKSJYrER58kluQZvog5NryEZAi0rVMPcpB577KKegGiFvlECVXC5jONYYoOgvJ4khMC7I0gJbGmIc5PUJKOY9ixqxL3mn50OUPCy327eh+XpWtqTNjsHDEAQR03RfyEOzr9lGGwy+knyEQAxQqeDCNjZiQt0WbFPrvH6YZ3q2U/BGXHooNl4ztxwjgxjUi0SYC8S/3/X6uUKkd2wemhyCgnP7sfSTlh/sRmx6L96k+crktuWDLWf1A3tmrGdlK/fLPc7+cgTt7tG0yOP+TUZakObL5Ng0hEcUrw1M/mbYsmBQwjUmLB6FHx8vNLqqLQm4J8bFYoGMsQfq0TLGt78wCCT4x5YJPP1j2a0Xiy0f5y/DaCnOkFXl5Toqki+PKfNBC4Ug0C8CtxkWa6jQMenatoT+Bee1wnHdP3rtUMuUAbrlg3MFCDKDFRMWwMNUK0ubTFOFnUG9xlQdLTPo+Voh3fbpWOor0OpUUrQr50hTSTrAQABioG7ZqpBFY5hFhLGiH3aRaBON5BaMhOyanVBeJWng88sEpL5d9f0/uI9VGw+5U4N6PBTHRQ0ahF+eqof0oM3E2l3gvxZDaOoYxf7qTTs/Qf9ZVPAxkdSYhLtk+e3fI3ftNe hyOqcUdO k3ogU8iqQk5J6LxpQdeW8zPuW7feUl4uvQnZoIM7fGs5uYvc6jBVRbcBj9BBMu3p/j1LrsUxAAO4g7LzTT3We3xDAGGi1uwVAzW0vbQ6tAbCM966JgiUJdDvyyCVnOhSq7qWO5dZrUQ4AxHPEsNdCFs7HbO21DAIhpVFHS3Fn3Nw/IfuodKXCnyfVRtKkVGHWdmIUvC3splvse4CFeioNuOTJromGMiRCqGZ4/iNlg/OJxVYeVSzLbwEkOY95I11Mq1t2tKguw+YsSuSg8vAK2DNTZLlBGsF2SbnLOn64t9joODRtiZBMRXUffEUB+1EsrpUUTI5n3FXfELxaC4U6fCrqTVfNTQ6tLkQZSprFzKhejcA= 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: I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote: > (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 PM, Michał Mirosław wrote: >>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum >>> wrote: >>>> >>>> On 6/14/23 3:36 AM, Michał Mirosław wrote: >>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >>>>> wrote: > [...] >>>>>> + if (cur_buf->bitmap == bitmap && >>>>>> + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { >>>>>> + cur_buf->len += n_pages; >>>>>> + p->found_pages += n_pages; >>>>>> + } else { >>>>>> + if (cur_buf->len && p->vec_buf_index >= 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 the >>>> page walk from next entry >>>> * 0 ---> continue the page walk >>>> * -ENOMEM --> Abort the page walk from current entry, user buffer is full >>>> 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 = output_range()) < > n_pages` means this is the final range, no more will fit. And if > `n_out == 0` then no pages fit and no WP is needed (no other special > cases). Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is. Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted. I'll just add is_interesting_page() in next version. > >>>>> 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 = 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 = false` work for vmas >>> not registered via uffd? >> UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region >> for it to report correct written values on the memory region. Without UFFD >> WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state >> undefined. If user hasn't initialized memory with UFFD, he has no right to >> set is_written = false. > > How about calculating `is_written = 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. I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk(). > >>> 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 is >> 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 look >> 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? Again I don't think UFFD is doing anything like this. > >>>>> [...] >>>>>> --- 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 = pages are in this category, value 0 = pages are not in this >>> 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 distinction >>>>> 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 output >>>>>> + * @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 from >>>> 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 getting >>>> 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_pages >> 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ł Mirosław -- BR, Muhammad Usama Anjum