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 ACE36C61DA4 for ; Tue, 14 Feb 2023 07:57:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 40EBF6B0075; Tue, 14 Feb 2023 02:57:37 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3970F280001; Tue, 14 Feb 2023 02:57:37 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 211686B007B; Tue, 14 Feb 2023 02:57:37 -0500 (EST) 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 0C0B06B0075 for ; Tue, 14 Feb 2023 02:57:37 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id DCE7D12062E for ; Tue, 14 Feb 2023 07:57:36 +0000 (UTC) X-FDA: 80465142912.11.AA58CED Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf28.hostedemail.com (Postfix) with ESMTP id E1724C0013 for ; Tue, 14 Feb 2023 07:57:33 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=ok3IRdMK; spf=pass (imf28.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=reject) header.from=collabora.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1676361454; 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=6uNbzgfz68j7Q/PMBt+6vlPF5CYJv2qVcAYfQds4uoc=; b=VKI2uYPyqUWMS8NCS0sgP3e1OjpouiqsUU+RDaZbPk0qTkZYoJP3vii59qKYmZWQMUZ3kQ zYWOUcDKPBb5msCT8/5eEUp/TIkFpUpO8jeuzW6EBfcCyUjucJi/vApzluAuhpQReUYAet 8e1M8Mopab6w2F7T9nI79E/qOFq+Llk= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=ok3IRdMK; spf=pass (imf28.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=reject) header.from=collabora.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1676361454; a=rsa-sha256; cv=none; b=CpcBo4z3MikC8g6UkPXxHnhM24qMP2/0vK28F67i9Cx/jNJsTm6E5ie6TVxhlGIlwX44fo bUDFXGWxidFN34E1uRtZ8bA1+AVM/5bGnsCebh98NrOz+GXJkAOYe+myHH6bHwxF82tP+T 4Yzp42NoN9IYHr+hp8+SqBfDW/INV0s= Received: from [192.168.10.12] (unknown [39.45.179.179]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: usama.anjum) by madras.collabora.co.uk (Postfix) with ESMTPSA id EECDE660216F; Tue, 14 Feb 2023 07:57:25 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1676361452; bh=bLuH3JNMnvAE/sd8bnRNmS8hADblvHZLNhfCr4oCcs0=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=ok3IRdMKvUkFcuLuDpafb/xmh9qr1fWMpvlJCB9RJ37YXB1qHtxPPzQJBmrpYhXGb aUEnFVi4PZCGRImQcghZRjXGN4C1VYoLrv+mopvvN5yUn1XRKNKgzMvTbmVngYGGZN T6dv5VOmBAwkkde4WCYfqhU8xnu6VTRXL6kpa5AYGNFpsjDVHtoUob5yZBRWC7hqIJ Wo4bV6ZwlQcKRCih0/JMbH42HY826QLorvWUQOsD7jI0LXmLV6oHe9VnzepM7WifWL SAGOqaa2X3WXuQEoQtknmsZaAJZ3P35wR2zCczus7cm14240bL+hM+/JZvqKl/iuWn TqvN+F4jTROjA== Message-ID: <39217d9a-ed7e-f1ff-59b9-4cbffa464999@collabora.com> Date: Tue, 14 Feb 2023 12:57:21 +0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Cc: Muhammad Usama Anjum , David Hildenbrand , Andrew Morton , =?UTF-8?B?TWljaGHFgiBNaXJvc8WC?= =?UTF-8?Q?aw?= , Andrei Vagin , Danylo Mocherniuk , Paul Gofman , Cyrill Gorcunov , Alexander Viro , Shuah Khan , Christian Brauner , Yang Shi , Vlastimil Babka , "Liam R . Howlett" , Yun Zhou , Suren Baghdasaryan , Alex Sierra , Matthew Wilcox , Pasha Tatashin , Mike Rapoport , Nadav Amit , 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 v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs To: Peter Xu References: <20230202112915.867409-1-usama.anjum@collabora.com> <20230202112915.867409-4-usama.anjum@collabora.com> <8b2959fb-2a74-0a1f-8833-0b18eab142dc@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-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: E1724C0013 X-Rspam-User: X-Stat-Signature: rdcqpmqf47mo4s3oq4bb3shmoh4erdgo X-HE-Tag: 1676361453-144031 X-HE-Meta: U2FsdGVkX19RmkLYXSoYvN5Uqw2oNFiWF1CKNZSPvYA64eefUkETGS3rjVlcvFrt+zU8NWbiTck2rdx5fakoEDh357lO7oHZbDLVG5TQu6+R+hEIagMoytlnWKilXdyQQOJdJVWvSKbAf4r6IAW2A/ObqnnRkL/nC88gLW90I7NphiSUDQPbo0vMaPE6l6/23fEmiUVEVrHU4nAmejE9c/kC8wG/TbNCi899wznUbBrlliiZ9fzouwTAviZMtREReA3bcKzCLWB28bNFFsVM8wAXrZszxrOjM1PKzGEg30XbZI3xIPiWFmRRRjk0CsV2tWRKa20J2LVAgAI63CTAddgpyyOJXZRioBi7EAn49PRZJ/OqhkeF9Zt8NoS75vEZKCjmRp+0CCE7cht6XBuZ4DRf5TpBTjt4YQhlulWybqIN1fluFlR0B105ye5RwC5yzmkqtcVHsvXCHuUVHxdaheRQJXCdVguWFAzcVVsqkNnAhXvxcluygSmApRGuzeU4XDuOyabjflLv4AZ5LeVSmcOfvZNKjIVxV/y00gC8FVSDqJKSTBHpZGYLnM28a+RhOyLcsJMHPp7tIqqj3k/vcu5x1N0Q92ecYL3DajPDOLYXVBO/ulHz8x+JefsWJYGLUjzyncqT6b8sqjgZtZM23FCISBOYd8kf72R4KaQ6dYaP7Px+IQzQ3g0WlGqYwWGhqNAXZPbVRU+x0KbF4tdxCUlJwCo/JC90bGQC5sDX6L2q8IYVeSHqcoJ63sm5l7MT/p0/ZfKlD635QvG2OdAEVzXckB9r+Ox0Eka3cxzdZ8h0J20PQOXwfsKg6Mj/zfazILXxz3qOykMGIBlHS9ZCegNDhyGjZgf5up2xobuvDwc+id6FpLQVlQkmXoxoLlQwkfBIj8w3UAhto4ZkqomzoahPoKl6XPnTjVNpd2ZGF9kZRrwaUzQ3LFFTM2UZ4n8Dde6grNiR6uqHMfaQ8Zy IESH7pwX +Z1Eh6ncU/xmsZFNTweHLqA3jIHQS9PNV6f+Nr+0DKd1KwMdbNc/+3u2/wKgR/7qoJrDPappblaiSjgJxDCQLEUz8cWBZt2DkrM1/Krgq4Z/3b4tb0SsRZvsDqTD11MHYgoJZEbN8/PZTR/VrvW++/MOqdX1l4G5e417EoaTRA/B0GuO+gAmepm4WJ1b7kh0/9vLS/5kEgD5cs6NK/P2JpmZE0ofteMCo3QIf044BpfVSJCj0gUsHO5m/bJ40kdDfSOPp+iTvuDNYwjIUnYcj3dmIJmqFoINukTJknY9Q1SvF4IfdgpUh0fAimajYAd/nGMBQRs/VdVblRWiJd5Op/XqrrVNVWCT7Q+qjZZmkzv6jY6Y= 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 2/14/23 2:42 AM, Peter Xu wrote: > On Mon, Feb 13, 2023 at 05:55:19PM +0500, Muhammad Usama Anjum wrote: >> On 2/9/23 3:15 AM, Peter Xu wrote: >>> On Thu, Feb 02, 2023 at 04:29:12PM +0500, Muhammad Usama Anjum wrote: >>>> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear >>>> the info about page table entries. The following operations are supported >>>> in this ioctl: >>>> - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), >>>> file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped >>>> (PAGE_IS_SWAPPED). >>>> - Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which >>>> pages have been written-to. >>>> - Find pages which have been written-to and write protect the pages >>>> (atomic PAGE_IS_WRITTEN + PAGEMAP_WP_ENGAGE) >>>> >>>> To get information about which pages have been written-to and/or write >>>> protect the pages, following must be performed first in order: >>>> - The userfaultfd file descriptor is created with userfaultfd syscall. >>>> - The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL. >>>> - The memory range is registered with UFFDIO_REGISTER_MODE_WP mode >>>> through UFFDIO_REGISTER IOCTL. >>>> Then the any part of the registered memory or the whole memory region >>>> can be write protected using the UFFDIO_WRITEPROTECT IOCTL or >>>> PAGEMAP_SCAN IOCTL. >>>> >>>> struct pagemap_scan_args is used as the argument of the IOCTL. In this >>>> struct: >>>> - The range is specified through start and len. >>>> - The output buffer of struct page_region array and size is specified as >>>> vec and vec_len. >>>> - The optional maximum requested pages are specified in the max_pages. >>>> - The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE >>>> is the only added flag at this time. >>>> - The masks are specified in required_mask, anyof_mask, excluded_ mask >>>> and return_mask. >>>> >>>> This IOCTL can be extended to get information about more PTE bits. This >>>> IOCTL doesn't support hugetlbs at the moment. No information about >>>> hugetlb can be obtained. This patch has evolved from a basic patch from >>>> Gabriel Krisman Bertazi. >>>> >>>> Signed-off-by: Muhammad Usama Anjum >>>> --- >>>> Changes in v10: >>>> - move changes in tools/include/uapi/linux/fs.h to separate patch >>>> - update commit message >>>> >>>> Change in v8: >>>> - Correct is_pte_uffd_wp() >>>> - Improve readability and error checks >>>> - Remove some un-needed code >>>> >>>> Changes in v7: >>>> - Rebase on top of latest next >>>> - Fix some corner cases >>>> - Base soft-dirty on the uffd wp async >>>> - Update the terminologies >>>> - Optimize the memory usage inside the ioctl >>>> >>>> Changes in v6: >>>> - Rename variables and update comments >>>> - Make IOCTL independent of soft_dirty config >>>> - Change masks and bitmap type to _u64 >>>> - Improve code quality >>>> >>>> Changes in v5: >>>> - Remove tlb flushing even for clear operation >>>> >>>> Changes in v4: >>>> - Update the interface and implementation >>>> >>>> Changes in v3: >>>> - Tighten the user-kernel interface by using explicit types and add more >>>> error checking >>>> >>>> Changes in v2: >>>> - Convert the interface from syscall to ioctl >>>> - Remove pidfd support as it doesn't make sense in ioctl >>>> --- >>>> fs/proc/task_mmu.c | 290 ++++++++++++++++++++++++++++++++++++++++ >>>> include/uapi/linux/fs.h | 50 +++++++ >>>> 2 files changed, 340 insertions(+) >>>> >>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c >>>> index e35a0398db63..c6bde19d63d9 100644 >>>> --- a/fs/proc/task_mmu.c >>>> +++ b/fs/proc/task_mmu.c >>>> @@ -19,6 +19,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> #include >>>> #include >>>> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma, >>>> } >>>> #endif >>>> >>>> +static inline bool is_pte_uffd_wp(pte_t pte) >>>> +{ >>>> + if ((pte_present(pte) && pte_uffd_wp(pte)) || >>>> + (pte_swp_uffd_wp_any(pte))) >>>> + return true; >>>> + return false; >>> >>> Sorry I should have mentioned this earlier: you can directly return here. >> No problem at all. I'm replacing these two helper functions with following >> in next version so that !present pages don't show as dirty: >> >> static inline bool is_pte_written(pte_t pte) >> { >> if ((pte_present(pte) && pte_uffd_wp(pte)) || >> (pte_swp_uffd_wp_any(pte))) >> return false; >> return (pte_present(pte) || is_swap_pte(pte)); >> } > > Could you explain why you don't want to return dirty for !present? A page > can be written then swapped out. Don't you want to know that happened > (from dirty tracking POV)? > > The code looks weird to me too.. We only have three types of ptes: (1) > present, (2) swap, (3) none. > > Then, "(pte_present() || is_swap_pte())" is the same as !pte_none(). Is > that what you're really looking for? Yes, this is what I've been trying to do. I'll use !pte_none() to make it simpler. > >> >> static inline bool is_pmd_written(pmd_t pmd) >> { >> if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) || >> (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd))) >> return false; >> return (pmd_present(pmd) || is_swap_pmd(pmd)); >> } > > [...] > >>>> + bitmap = cur & p->return_mask; >>>> + if (cpy && bitmap) { >>>> + if ((prev->len) && (prev->bitmap == bitmap) && >>>> + (prev->start + prev->len * PAGE_SIZE == addr)) { >>>> + prev->len += len; >>>> + p->found_pages += len; >>>> + } else if (p->vec_index < p->vec_len) { >>>> + if (prev->len) { >>>> + memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region)); >>>> + p->vec_index++; >>>> + } >>> >>> IIUC you can have: >>> >>> int pagemap_scan_deposit(p) >>> { >>> if (p->vec_index >= p->vec_len) >>> return -ENOSPC; >>> >>> if (p->prev->len) { >>> memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region)); >>> p->vec_index++; >>> } >>> >>> return 0; >>> } >>> >>> Then call it here. I think it can also be called below to replace >>> export_prev_to_out(). >> No this isn't possible. We fill up prev until the next range doesn't merge >> with it. At that point, we put prev into the output buffer and new range is >> put into prev. Now that we have shifted to smaller page walks of <= 512 >> entries. We want to visit all ranges before finally putting the prev to >> output. Sorry to have this some what complex method. The problem is that we >> want to merge the consective matching regions into one entry in the output. >> So to achieve this among multiple different page walks, the prev is being used. >> >> Lets suppose we want to visit memory from 0x7FFF00000000 to 7FFF00400000 >> having length of 1024 pages and all of the memory has been written. >> walk_page_range() will be called 2 times. In the first call, prev will be >> set having length of 512. In second call, prev will be updated to 1024 as >> the previous range stored in prev could be extended. After this, the prev >> will be stored to the user output buffer consuming only 1 struct of page_range. >> >> If we store prev back to output memory in every walk_page_range() call, we >> wouldn't get 1 struct of page_range with length 1024. Instead we would get >> 2 elements of page_range structs with half the length. > > I didn't mean to merge PREV for each pgtable walk. What I meant is I think > with such a pagemap_scan_deposit() you can rewrite it as: > > if (cpy && bitmap) { > if ((prev->len) && (prev->bitmap == bitmap) && > (prev->start + prev->len * PAGE_SIZE == addr)) { > prev->len += len; > p->found_pages += len; > } else { > if (pagemap_scan_deposit(p)) > return -ENOSPC; > prev->start = addr; > prev->len = len; > prev->bitmap = bitmap; > p->found_pages += len; > } > } > > Then you can reuse pagemap_scan_deposit() when before returning to > userspace, just to flush PREV to p->vec properly in a single helper. > It also makes the code slightly easier to read. Yeah, this would have worked as you have described. But in pagemap_scan_output(), we are flushing prev to p->vec. But later in export_prev_to_out() we need to flush prev to user_memory directly. > -- BR, Muhammad Usama Anjum