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 53C2FC61DA4 for ; Thu, 23 Feb 2023 07:10:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B89236B0074; Thu, 23 Feb 2023 02:10:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B12476B0075; Thu, 23 Feb 2023 02:10:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 98BC86B0078; Thu, 23 Feb 2023 02:10:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 825DB6B0074 for ; Thu, 23 Feb 2023 02:10:54 -0500 (EST) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4E3301A119C for ; Thu, 23 Feb 2023 07:10:54 +0000 (UTC) X-FDA: 80497684428.27.DDCF2C2 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf08.hostedemail.com (Postfix) with ESMTP id 67C8416000C for ; Thu, 23 Feb 2023 07:10:52 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=jXxbccMk; spf=pass (imf08.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=1677136252; 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=uS9v2Rz/muU77xnyphQmM+22LDHERyrN3mDiNXgRc3g=; b=Y+iW+mgHdQ6xASgJc83ofRKV7GEAU9F4YfYdA4nG5Fc9CR+EYxKBhFh7fpvHA1Dhad0x5K Fhrxh54iGiSO/XFN0MNxQ2723oYsbfiRYbGH/zDdfD8ZwhjwzX5yEnyChFGPhr7Sj7pmZG geEZNV6av5e4mN2IWyhgGNhYr1MIWXA= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=jXxbccMk; spf=pass (imf08.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=1677136252; a=rsa-sha256; cv=none; b=BC+75dRcEOywqyKm8syqIo8JcCBmhxxBt28mriDeWnXWSw1vBDpKFgpG2Tt6b6UvhWBzNA Uj2pxCOudk4TKzYoNmrLuUqR+avtoGzCjQ1m2PX9rtIBY5hN0Iq5mwkrCGHMInxVq+VhE3 PoAmQ2pxbbMBbj6dJQLzDmCBrDfJKDk= Received: from [192.168.10.12] (unknown [39.45.217.110]) (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 5A5D366021EE; Thu, 23 Feb 2023 07:10:44 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1677136250; bh=l/p5o6Wgs9SQqz6XM6V90URqUHPwWct0pb1tZfq2qT8=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=jXxbccMkuSbQ+H0cNGv5LnGxMywISsVIclNBnzIYcGl/abV5K/IO87gZ9YKhN9rKS Ld/QTGNc6sMW8SwKqk+sQAFtffKPP53Yp+tQZCvgePK8Oa4gI6FPlge1jetOe+0Bt8 rxidSPecRM2vLvm8Khloj5UfIEd0mFSPieVa1VzFUNGXmrMAsBdEezU3I+sL9o9sTW z6Qmee2QXBwWbVfxr6n4KnbkKpO4A9heQ7n9fD0BeGu7nsUFvTIoo51fcyaHl9fY2O vqv4Ed4Iv0fwynrOvQVtaEuzUKKkrvMqX5s4kHvFydWBJeqgRw/ZOrn9qcBDdF52Wc QcvwF/s72GgyA== Message-ID: Date: Thu, 23 Feb 2023 12:10:40 +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 , Andrew Morton , Alexander Viro , =?UTF-8?B?TWljaGHFgiBNaXJvc8WC?= =?UTF-8?Q?aw?= , Cyrill Gorcunov , Paul Gofman , Danylo Mocherniuk , Shuah Khan , Christian Brauner , Peter Xu , Yang Shi , Vlastimil Babka , "Liam R . Howlett" , Yun Zhou , Suren Baghdasaryan , Alex Sierra , Matthew Wilcox , Pasha Tatashin , Mike Rapoport , Axel Rasmussen , "Gustavo A . R . Silva" , Dan Williams , kernel list , linux-fsdevel , linux-mm , linux-kselftest , Greg KH , "kernel@collabora.com" , David Hildenbrand , Andrei Vagin Subject: Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs Content-Language: en-US To: Nadav Amit , Mike Rapoport , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= References: <20230202112915.867409-1-usama.anjum@collabora.com> <20230202112915.867409-4-usama.anjum@collabora.com> <2fe790e5-89e0-d660-79cb-15160dffd907@collabora.com> <751CCD6C-BFD1-42BD-A651-AE8E9568568C@vmware.com> From: Muhammad Usama Anjum In-Reply-To: <751CCD6C-BFD1-42BD-A651-AE8E9568568C@vmware.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 67C8416000C X-Stat-Signature: jrid9byuk9tnuj91miawkwojwp6cq4au X-HE-Tag: 1677136252-931370 X-HE-Meta: U2FsdGVkX18IOzbF8YW3Cn1TjwHPqbaHf7J5Cuq9JpPDJuUeIo8SC2Svido9BsJzKImxPagjeaWSmdiUPH51+SsK+VxcOb1CtsFnSWudxabZxbm8teUL6JmYBjJvnHT37OvZtVKF1DP7FHVHlDaL4XJsooMiw+8BWYScfswbJ+cNxESFIvZSgLSqultoHvYMyDytOMgPjpgge0yr7pg1jQ5pAyH0WYqOyvThMtzhTjwaofSCzILWHm3hoP+pSEnHv0mlVoi2LI8pTZQx8oCoRDvADBfQJ/MKGthxheHz9hhkvYfUJIghNhD6k/f70fxHYJTA0HoaK11hFsMg5pu4Wp6e56Y8DSQGdPsOytjxT2vvXdCChQm3pikgV8duwA9TeJsHuTLWJPjdRhSnxs9dtroaK0r163yCxKrDgtn1Opbwx8M+3J5iXBxPXdcPYni22/64dANabOdIF/o1/4mriwS2ZvlbExj94UIKIXC4VambdLzkSfRJlBKouTiLbUoLW6L5xiOi7KyeamP1Hoy7IoTB7VaY0/eBWns8/Pu0HHAsuyNXKBeZMtUrCMX2AHEZ/0CNZ5GRySFeTeEkFwXBlSUzjgkyzHZ/FCdRpOehCO0zPdGQDQXI0bwnGPGfyCDsXelNADix0NA/lraJOt3PB1HvVteRVKZu3C+FkYmxhdeUFBxQolyGWCwtpLpZhV8obj92HkADB/19mmSbX4uP/SsWzRaVsan5kugC4RcKXobMtg0y3v0Z7B21y+6gyzmbO4h7RjfJP57n5plP5P00lSo2PkD1OMBC5lOYPDMhOPiIHAWdMP5hq+rZMdz7vOAoA4X1O5yKUSA/biik/yJCEWFEpIleiZNogfgsZ/vCoL7CTOO29u+UsxyO3XbFCeHaUizMdx+sMdelV4Z1+0fkl1icyT8dV9gAAzCjRsNhyLIzxPT3RYCVg1EqQMJqAkAS1McK7Sr8dl1Xl6LuAO2 9cK+eEvE kFuL8lFqGfOt+ME18UUBrUDwQOHnpICDdnDPm3BbiD6iuqh8CyCMiIqxfByJKRj/XQ0O2VMZyTyXuZuNyYT7rTN94LIvhXLi57/6wpMObg+PXIuw4pro7PGFPi0ciOG3//3vMDwzVlCNaLZtFhlrNLTQaGrZH4k8/yaepXUFt1L9x50JjmnkW+Lg+hz5QqQWrLJG4qwaSzOwYRhaZe98vdMDPNfP74qD/n8l9HZH99kTNHcqPx9q6yBIrjahzeo5uG3bdM2uAHygJwXG8/vA+EfhGZQF8DeUKP393vs+pZ+ff8Rz4+IJIk33t/AQ40rNz2qPX5mkKeDEyv8AVPkyvf5hBvQT0fPePJuZNEy6Wx3NOId4= 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: Hi Nadav, Mike, Michał, Can you please share your thoughts at [A] below? On 2/23/23 12:10 AM, Nadav Amit wrote: > > >> On Feb 20, 2023, at 5:24 AM, Muhammad Usama Anjum wrote: >> >>>> +static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start, >>>> + unsigned long end, struct mm_walk *walk) >>>> +{ >>>> + struct pagemap_scan_private *p = walk->private; >>>> + struct vm_area_struct *vma = walk->vma; >>>> + unsigned long addr = end; >>>> + spinlock_t *ptl; >>>> + int ret = 0; >>>> + pte_t *pte; >>>> + >>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>> + ptl = pmd_trans_huge_lock(pmd, vma); >>>> + if (ptl) { >>>> + bool pmd_wt; >>>> + >>>> + pmd_wt = !is_pmd_uffd_wp(*pmd); >>>> + /* >>>> + * Break huge page into small pages if operation needs to be >>>> performed is >>>> + * on a portion of the huge page. >>>> + */ >>>> + if (pmd_wt && IS_WP_ENGAGE_OP(p) && (end - start < HPAGE_SIZE)) { >>>> + spin_unlock(ptl); >>>> + split_huge_pmd(vma, pmd, start); >>>> + goto process_smaller_pages; >>> I think that such goto's are really confusing and should be avoided. And >>> using 'else' (could have easily prevented the need for goto). It is not the >>> best solution though, since I think it would have been better to invert the >>> conditions. >> Yeah, else can be used here. But then we'll have to add a tab to all the >> code after adding else. We have already so many tabs and very less space to >> right code. Not sure which is better. > > goto’s are usually not the right solution. You can extract things into a different > function if you have to. > > I’m not sure why IS_GET_OP(p) might be false and what’s the meaning of taking the > lock and dropping it in such a case. I think that the code can be simplified and > additional condition nesting can be avoided. Lock is taken and we check if pmd has UFFD_WP set or not. In the next version, the GET check has been removed as we have dropped WP_ENGAGE + !GET operation. So get is always specified and condition isn't needed. Please comment on next version if you want anything more optimized. > >>>> --- a/include/uapi/linux/fs.h >>>> +++ b/include/uapi/linux/fs.h >>>> @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t; >>>> #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\ >>>> RWF_APPEND) >>>> +/* Pagemap ioctl */ >>>> +#define PAGEMAP_SCAN _IOWR('f', 16, struct pagemap_scan_arg) >>>> + >>>> +/* Bits are set in the bitmap of the page_region and masks in >>>> pagemap_scan_args */ >>>> +#define PAGE_IS_WRITTEN (1 << 0) >>>> +#define PAGE_IS_FILE (1 << 1) >>>> +#define PAGE_IS_PRESENT (1 << 2) >>>> +#define PAGE_IS_SWAPPED (1 << 3) >>> >>> These names are way too generic and are likely to be misused for the wrong >>> purpose. The "_IS_" part seems confusing as well. So I think the naming >>> needs to be fixed and some new type (using typedef) or enum should be >>> introduced to hold these flags. I understand it is part of uapi and it is >>> less common there, but it is not unheard of and does make things clearer. >> Do you think PM_SCAN_PAGE_IS_* work here? > > Can we lose the IS somehow? [A] Do you think these names would work better: PM_SCAN_WRITTEN_PAGE, PM_SCAN_FILE_PAGE, PM_SCAN_SWAP_PAGE, PM_SCAN_PRESENT_PAGE? > >> >>> >>> >>>> + >>>> +/* >>>> + * struct page_region - Page region with bitmap flags >>>> + * @start: Start of the region >>>> + * @len: Length of the region >>>> + * bitmap: Bits sets for the region >>>> + */ >>>> +struct page_region { >>>> + __u64 start; >>>> + __u64 len; >>> >>> I presume in bytes. Would be useful to mention. >> Length of region in pages. > > Very unintuitive to me I must say. If the start is an address, I would expect > the len to be in bytes. The PAGEMAP_SCAN ioctl is working on page granularity level. We tell the user if a page has certain flags are not. Keeping length in bytes doesn't makes sense. > -- BR, Muhammad Usama Anjum