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 52A63EB64D7 for ; Fri, 16 Jun 2023 06:57:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DDF308E0002; Fri, 16 Jun 2023 02:57:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D69508E0001; Fri, 16 Jun 2023 02:57:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C08388E0002; Fri, 16 Jun 2023 02:57:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id AAD168E0001 for ; Fri, 16 Jun 2023 02:57:41 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 79B69A09FC for ; Fri, 16 Jun 2023 06:57:41 +0000 (UTC) X-FDA: 80907705522.23.B38B74C Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf22.hostedemail.com (Postfix) with ESMTP id 91DE2C0003 for ; Fri, 16 Jun 2023 06:57:39 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=fuhb4Evg; spf=pass (imf22.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com; dmarc=pass (policy=quarantine) header.from=collabora.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686898659; 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=dIl3p+GC4Bc6ajVcmccInEx3j6rt2wsomj9MjQpuhaA=; b=19mO8Jc77rudAFy321g9h81l+sL+oeEhyNOvJFHQ1xK0LyIebO7VUTaEtYRcUypzJ8pNUv dlEi/2jh9nv2uqMcW16KPzPqvpx9y5HSjDIN8DeieAzR/PkjkVBPxy/0sdVMJBeGdu4QEO EV3IuK1jbALVEs8OsVs8MDGujk7yMsk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686898659; a=rsa-sha256; cv=none; b=Q1KjUFyRbsYgGzu1cTQruPyJH2lhkC4UOrl6aIcfqSquWKLt9uLiOZfg81716vsIlz6fKq zdi1otB9GSK+AkLmJa9HWwctLFbkHIhimr78ZIAvw66v5bOirO7+habEVRfmsMt24UJzTd D7GcF77OCHuL4vYCqTXabdaUn4YyQag= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=fuhb4Evg; spf=pass (imf22.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com; dmarc=pass (policy=quarantine) header.from=collabora.com 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 C34F56606F1A; Fri, 16 Jun 2023 07:57:30 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1686898657; bh=Omzo0pLNcXzbmnJ6mj+hFNJQNMry5y1BPxh4GdO6tsE=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=fuhb4Evg3aejWQIX4CKPzco+2hgCL3d6CyC4qsV3igE3nOOZKISrKbp0AbrkKHnlX ijAYqKE4+v19IQsFGYoAbaYk5GVXdvGlAcQza+BBCNqTQFJFNlUOyvPq4CQDxmAN7H 4CxItF6P4ZlHecWC2qfhrBIz2HmTDaabArJ8in7K8PiIRc3NYp6yIlnm9Fn0pUwIK1 /G2ZC8HkUSco0P4gTBMmvaOEH13ZrR8jeh0dbY1LU9MdvozBv829fCfOdKQlZs74XU nKihSFE3NtHpqQg/sYM4j/Rvg5WJxlgKt5vJ+QMa7srQfrqTnECJ2ROQT7xdt9KF/G eegFGSxYPvvrQ== Message-ID: <2e1b80f1-0385-0674-ae5f-9703a6ef975d@collabora.com> Date: Fri, 16 Jun 2023 11:57:27 +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 Content-Language: en-US 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> <96b7cc00-d213-ad7d-1b48-b27f75b04d22@collabora.com> <39bc8212-9ee8-dbc1-d468-f6be438b683b@collabora.com> From: Muhammad Usama Anjum In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 91DE2C0003 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 6fxhm6s3jysbzwxp7ytnxdib173hnxu9 X-HE-Tag: 1686898659-68704 X-HE-Meta: U2FsdGVkX19HR0lLoug6lJmkqewxsX1VvZEgXAy+Jht5uUvlwFwP7doGMj8c/E+aZ4oJvueSR+yn0kEqv8OxtYcQSBGzOHQdaAQRMPO7lxp+a+5nPT2QgZ+VcciuIqgM5FORn6QfU14OP7FD8AKXk1VXkqzKjcYImcjHC7s73wN4rCRod5xMRMi11DCeBKrCXTwVg6W9n6bU9KMozIB3F6pv0F52j6GpqHdzy0QGWfXDEoSXPxtmB0xJFfygRYXtZgyYsQqCDejwuLJJOKlWhVULP4gXMOoE/6Mr8WMj3Hpi9IoDbt63FPnUJhLP+9xk02jMZl4GZPiotgoonlCPG4p4KF+XT4fbobopLbQjPB/+KThmI0eaed92sN3dJsTVt6FGFMF5k24VKAWGZF1wWHDjHHOeBha6+68yxpvAdunODrJVbX4+0eL63/eRcJ5dldk3ib8eMxlDP6gRP/FmNQ5J8GfwIb3oFJlvFv7dWcVoYXYnc7hlZQIPnpf2u/XjK/dFUelHf8U+feHvCXyoJjNEYGzMeiXurY7vDUyGg0F+NH29l2kX7d6UMx91wjtn856FUBYdHBnQbi4uITCXsqfySrO28SAE6lsBjJyr0m6/UiEc9223FgDdM0Gkeni1rvGRy7Vyo2hdCZanJgXAWobqI7gBWJCY/W00lNJkj7jvMKBw8n96GbcizsC3Ckqxm+hKWgtfyMGPsMfPOrnQR9W3tj3QXZTxVNm659KAgLaBIbV+qmTCllQLWrqvcfB93ISwli12IMmk2/XZJWeQc0m6FUxohwhpMqfQGYdVehYov3JaXIJjuakkq8dPsiPnaxJapQbYFNfSXO7BoOMdE+E5G3ega8qAsml2RjgnTtVj3cyCXLs/OtscSepN4z5+/uillj6lWFnc00PDHGxeaMkeoztHR8orCABu7W5yQIq/RGzE3YZAD7ECRMWflUV7ppuQPnAfQJjT74rqPpw 9YmeAgP1 iYA5IW2o+JlDuX4lvJDQZhWhtKXzpl9exq7gWVZ0Ae6dHIkjx+skVobXb/mCwHmBRm3N3f1MXbBCeMw2Xagb0PxnAto3lwl4gnyFUOo8lUIk3gcTcFZy21qj9bSG0U6FyFU7UQwegRltctUtyE7sPBhyQY/HlyaK1351i9kaHGJoMQWGD4hnYEHj6yU09H9evMpnAQgfhl3RlCh0PopDW3lKo1cFhaFNUz9Szkzm9zqGAt8xvUHZz2TBFunyuKBXgxpaK342YcTJsL1ev8sYPx1jgQZMugwgLX+DZOu+BX8e4WkJwQUPZcpPAZMpoz/CSQV0zK/7HTJjGgAC9VqGBHD3xS7PQiGY3/ru2y0eAbNM/t9k= 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 6/16/23 1:07 AM, Michał Mirosław wrote: > On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum > wrote: >> On 6/15/23 7:52 PM, Michał Mirosław wrote: >>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum >>> wrote: >>>> 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. >>> >>> This case is exactly what `n_out < n_pages` will cover (if scan_output >>> uses max_pages properly to limit n_out). >>> Isn't it that when the buffer is full we want to abort the scan always >>> (with WP if `n_out > 0`)? >> Wouldn't it be duplication of condition if buffer is full inside >> pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we >> check if we have space before putting data inside it. I'm using this same >> condition to indicate that buffer is full. > > I'm not sure what do you mean? The buffer-full conditions would be > checked in ..scan_output() and communicated to the caller by returning > N less than `n_pages` passed in. This is exactly how e.g. read() > works: if you get less than requested you've hit the end of the file. > If the file happens to have size that is equal to the provided buffer > length, the next read() will return 0. Right now we have: pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES; pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error You are asking me to do: pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES; pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret > 0) // success make_UFFD_WP and flush else if (ret == 0) // buffer full return PM_SCAN_BUFFER_FULL; else //other errors buffer_error So you are asking me to go from consie code to write more lines of code. I would write more lines without any issue if it improves readability and logical sense. But I don't see here any benefit. > >>>>>>> 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. >>> >>> If it's cheap and easy to provide a user with a way to detect the >>> supported features - why not do it? >> I'm sorry. But it would bring up something new and iterations will be >> needed to just play around. I like the UFFD way. > > Let's then first agree on what would have to be changed. I guess we > could leverage that `scan_len = 0` doesn't make much sense otherwise > and let it be used to check the other fields for support. We are making things more and more complex. I don't like multi-plexing variables. Can you give examples where multi-plexing has been done on variables inside linux kernel? Muti-plexing means user gives input and takes output from same variable. It makes variable double meaning. > > Best Regards > Michał Mirosław -- BR, Muhammad Usama Anjum