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 ED625C54EED for ; Mon, 30 Jan 2023 08:38:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 51C728E0001; Mon, 30 Jan 2023 03:38:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4CC156B0073; Mon, 30 Jan 2023 03:38:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3936F8E0001; Mon, 30 Jan 2023 03:38:32 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 2A3A36B0072 for ; Mon, 30 Jan 2023 03:38:32 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id F2AF1140564 for ; Mon, 30 Jan 2023 08:38:31 +0000 (UTC) X-FDA: 80410814022.10.6856EF0 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf16.hostedemail.com (Postfix) with ESMTP id 092D6180015 for ; Mon, 30 Jan 2023 08:38:28 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=D9ech3Ye; dmarc=pass (policy=reject) header.from=collabora.com; spf=pass (imf16.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=1675067909; 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=DZ0s/9pZZWD9iEb3W+UKA/r/M13cVXrj6XA6IZgbM20=; b=pPccup1QD7VHQ2k2Gabu6f8TmEZ3KdSV8AlifAW/52z6rXai3R6MYisnuqK+LOXZi2VGWw 0STCefVyezMZMEHbDWe8PIz5qkOeWSnOYth1jUVhxeDnEl6jsJyUmNZhYegvT+HaP0PIN0 GimQyTiaLILjD1IVaZPlTGyD9xJCTjU= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=D9ech3Ye; dmarc=pass (policy=reject) header.from=collabora.com; spf=pass (imf16.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=1675067909; a=rsa-sha256; cv=none; b=Ummxv4q1J2FfWgFmQdBiEfqk95TpyX9HNvhTwAIeUnd4V1SCmx4Po8FEg78sm+0K78Knwk LMARTcGL/EPpiUfC7gkcFAFbSNS42DmWLDimucC/8yB4ViZzxADkIiQYyzsdFGsyBWdtOg cnvP6r338172l4UxEarvg7Y9W/OTc3c= Received: from [192.168.10.12] (unknown [39.45.165.226]) (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 1D6C86602DE5; Mon, 30 Jan 2023 08:38:19 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1675067907; bh=5cZ1uwdlD2VMh9wC/4HNBJIXQ/pBEtuV1lN6pYKGkqw=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=D9ech3Yelwdi1yWeENtGt+6695vw+jM0hsWDm+SIrFi8jKGgW/+GkGjHcKwz8ClPa MxtKClHXZyTOY6OTHN4n3d1EPAZN2jT/UgRekHhzWtvotYG2fJeojV4JWR+U6htUpj N0Siwvij8LVP2P6T5CfQRfCHpkX15+EJsAVinoFjVBNMVqtxfpeuahfAhLx9AY8uEq tQ+Sfq7FqCAKS1tb9fUYM2EPF+A0Vv74V84XpbdaxhbVyzy0NrieOp0ThMM6tpo0lx WZ3iB7w9gdYQT71mBmEcdJ/gVWRBXRbb4ZLfPL+HMSskYISlcUN9LoydkrkYNJux9+ EwNw5pWcSkiKA== Message-ID: Date: Mon, 30 Jan 2023 13:38:16 +0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 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 v8 1/4] userfaultfd: Add UFFD WP Async support To: Peter Xu References: <20230124084323.1363825-1-usama.anjum@collabora.com> <20230124084323.1363825-2-usama.anjum@collabora.com> <1968dff9-f48a-3290-a15b-a8b739f31ed2@collabora.com> Content-Language: en-US From: Muhammad Usama Anjum In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 092D6180015 X-Stat-Signature: fytawya7w7j68jct4zjtru914gdx1na4 X-HE-Tag: 1675067908-958796 X-HE-Meta: U2FsdGVkX18wcAxx1E4rD4jbufmZPYrtjaLmNpgOORJeMuB7v9nbdjMf4/hyQPgrDTdNyG/AXCZN60H8EnnFyvGAarciU46LSXu7/mjNmE//afuSUolD9JJI5bsoGQjNJijMUhuSJKM3h2YEvEXKeviqzoWB7z8Fb/OOTHqMm4sZoyDAxj/jXpKV/9F37sdbBDh+aN0QFuAAHjzfcXZd5VDm4YMhQxhvtobcppbsNt8v3qML8CmSUAvSgu21xZyD/+6fob6NBsFcYaKzXT48z1t47cWLpI6vRd3uZW3LAzsa/PRAIcG14BQ4sJzy+X38SGVAAbqDIBbwdBsDYGS4JaI8SKgZsSUnD3eR+e14sySu4XpHU4Tx+Ep2skHvxwOC95T9LYiB7zPQoSxsC5sXTJKufqvfpV6mI9OY2osIVguHXg6Qt5icbGj3dsuFI2Uz4ZwEcwTcgQ5rcK7zbTL367V/N399JMQt116LODUr60PtgzU1+XyTvZ73URzJz5VUF8UF+8o+r4zNS3dmXL1YSNUXDcL1LCjzC6gBEW48i5Pjc31F/0WgebNJXzBcrZck9SdyThyBH9FUljULuYKrtfqTEUQE6CxWHUlNovEPo91xWZZxOPheIbj6IEOW0F1sFsHgvrx5VodXxaCi4k2Dg0tQU2HifJxUeNr+2I4oZ+wg+jnV7s40lttkOwFIOk+IAHRjbI4YMz4pPw+kiNItti1MA4l1J+tRUsszCTCJU6Wc6eK2BuZnoI0BzFS3gjuu/xMxN/QHo5kVBEpJep/xra5S7xGsx5AQS/4rN5yDaytVC652PdV9lyiWvdGbhGb8E8oUsU3Lb85XKNwqRm5JUnuWp8Y64TVF2vOGWIRfhKO7VG0p7sbrSyAxvP+siUbP/Xqbn86xiR22MjiUbasxmq/Sh06dePDgSiAMq9PjfvErj5yvxC2dGE1SAQrvelrkm84q3LS8On3v9aAV9Jv RBoa75ei YLR4tmHamaFTwPtX7+4sQL9hrpNA3LhW2IfSlw3hit6UTOS3GLFeX3/3yMOxwL8moLACwQYaa7H1ZkUMzh94VxxSH6qAixRJ0CkIN6B7a90sJFsIFX4p4XSSkIYXJlhBhGRA47ajO2XQG6u2eg0XiU+3Yk11GMmGLlzT7tXa3UN0w77vgOPtRdiREcCLE1t/N70UZyBCP86piFVOQdfUfuW5brXBn2NCKQdYwriKAlkerDziv/wsFwaTFZ03gp8StjU8gebcqXqCa+o7Z9lTRzYMcDQ== 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 1/27/23 8:32 PM, Peter Xu wrote: > On Fri, Jan 27, 2023 at 11:47:14AM +0500, Muhammad Usama Anjum wrote: >>>> diff --git a/mm/memory.c b/mm/memory.c >>>> index 4000e9f017e0..8c03b133d483 100644 >>>> --- a/mm/memory.c >>>> +++ b/mm/memory.c >>>> @@ -3351,6 +3351,18 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) >>>> >>>> if (likely(!unshare)) { >>>> if (userfaultfd_pte_wp(vma, *vmf->pte)) { >>>> + if (userfaultfd_wp_async(vma)) { >>>> + /* >>>> + * Nothing needed (cache flush, TLB invalidations, >>>> + * etc.) because we're only removing the uffd-wp bit, >>>> + * which is completely invisible to the user. This >>>> + * falls through to possible CoW. >>> >>> Here it says it falls through to CoW, but.. >>> >>>> + */ >>>> + pte_unmap_unlock(vmf->pte, vmf->ptl); >>>> + set_pte_at(vma->vm_mm, vmf->address, vmf->pte, >>>> + pte_clear_uffd_wp(*vmf->pte)); >>>> + return 0; >>> >>> ... it's not doing so. The original lines should do: >>> >>> https://lore.kernel.org/all/Y8qq0dKIJBshua+X@x1n/ > > [1] > >>> >>> Side note: you cannot modify pgtable after releasing the pgtable lock. >>> It's racy. >> If I don't unlock and return after removing the UFFD_WP flag in case of >> async wp, the target just gets stuck. Maybe the pte lock is not unlocked in >> some path. >> >> If I unlock and don't return, the crash happens. >> >> So I'd put unlock and return from here. Please comment on the below patch >> and what do you think should be done. I've missed something. > > Have you tried to just use exactly what I suggested in [1]? I'll paste > again: > > ---8<--- > diff --git a/mm/memory.c b/mm/memory.c > index 4000e9f017e0..09aab434654c 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -3351,8 +3351,20 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) > > if (likely(!unshare)) { > if (userfaultfd_pte_wp(vma, *vmf->pte)) { > - pte_unmap_unlock(vmf->pte, vmf->ptl); > - return handle_userfault(vmf, VM_UFFD_WP); > + if (userfaultfd_uffd_wp_async(vma)) { > + /* > + * Nothing needed (cache flush, TLB > + * invalidations, etc.) because we're only > + * removing the uffd-wp bit, which is > + * completely invisible to the user. > + * This falls through to possible CoW. > + */ > + set_pte_at(vma->vm_mm, vmf->address, vmf->pte, > + pte_clear_uffd_wp(*vmf->pte)); > + } else { > + pte_unmap_unlock(vmf->pte, vmf->ptl); > + return handle_userfault(vmf, VM_UFFD_WP); > + } > } > ---8<--- > > Note that there's no "return", neither the unlock. The lock is used in the > follow up write fault resolution and it's released later. I've tried out the exact patch above. This doesn't work. The pages keep their WP flag even after being resolved in do_wp_page() while is written on the page. So I'd added pte_unmap_unlock() and return 0 from here. This makes the patch to work. Maybe you can try this on your end to see what I'm seeing here? > > Meanwhile please fully digest how pgtable lock is used in this path before > moving forward on any of such changes. > >> >>> >>>> + } >>>> pte_unmap_unlock(vmf->pte, vmf->ptl); >>>> return handle_userfault(vmf, VM_UFFD_WP); >>>> } >>>> @@ -4812,8 +4824,21 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf) >>>> >>>> if (vma_is_anonymous(vmf->vma)) { >>>> if (likely(!unshare) && >>>> - userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) >>>> - return handle_userfault(vmf, VM_UFFD_WP); >>>> + userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) { >>>> + if (userfaultfd_wp_async(vmf->vma)) { >>>> + /* >>>> + * Nothing needed (cache flush, TLB invalidations, >>>> + * etc.) because we're only removing the uffd-wp bit, >>>> + * which is completely invisible to the user. This >>>> + * falls through to possible CoW. >>>> + */ >>>> + set_pmd_at(vmf->vma->vm_mm, vmf->address, vmf->pmd, >>>> + pmd_clear_uffd_wp(*vmf->pmd)); >>> >>> This is for THP, not hugetlb. >>> >>> Clearing uffd-wp bit here for the whole pmd is wrong to me, because we >>> track writes in small page sizes only. We should just split. >> By detecting if the fault is async wp, just splitting the PMD doesn't work. >> The below given snippit is working right now. But definately, the fault of >> the whole PMD is being resolved which if we can bypass by correctly >> splitting would be highly desirable. Can you please take a look on UFFD >> side and suggest the changes? It would be much appreciated. I'm attaching >> WIP v9 patches for you to apply on next(next-20230105) and pagemap_ioctl >> selftest can be ran to test things after making changes. > > Can you elaborate why thp split didn't work? Or if you want, I can look > into this and provide the patch to enable uffd async mode. Sorry, I was doing the wrong way. Splitting the page does work. What do you think about the following: --- a/mm/memory.c +++ b/mm/memory.c @@ -3351,6 +3351,17 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) if (likely(!unshare)) { if (userfaultfd_pte_wp(vma, *vmf->pte)) { + if (userfaultfd_wp_async(vma)) { + /* + * Nothing needed (cache flush, TLB invalidations, + * etc.) because we're only removing the uffd-wp bit, + * which is completely invisible to the user. + */ + set_pte_at(vma->vm_mm, vmf->address, vmf->pte, + pte_clear_uffd_wp(*vmf->pte)); + pte_unmap_unlock(vmf->pte, vmf->ptl); + return 0; + } pte_unmap_unlock(vmf->pte, vmf->ptl); return handle_userfault(vmf, VM_UFFD_WP); } @@ -4812,8 +4823,13 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf) if (vma_is_anonymous(vmf->vma)) { if (likely(!unshare) && - userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) + userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) { + if (userfaultfd_wp_async(vmf->vma)) { + __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL); + return 0; + } return handle_userfault(vmf, VM_UFFD_WP); + } return do_huge_pmd_wp_page(vmf); } > > Thanks, > -- BR, Muhammad Usama Anjum