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 X-Spam-Level: X-Spam-Status: No, score=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6D7FBC433E0 for ; Wed, 3 Feb 2021 18:21:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B3AF964F7C for ; Wed, 3 Feb 2021 18:21:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B3AF964F7C Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id E82F76B0005; Wed, 3 Feb 2021 13:20:59 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E32F66B0006; Wed, 3 Feb 2021 13:20:59 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CD3366B006E; Wed, 3 Feb 2021 13:20:59 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0157.hostedemail.com [216.40.44.157]) by kanga.kvack.org (Postfix) with ESMTP id B8D5E6B0005 for ; Wed, 3 Feb 2021 13:20:59 -0500 (EST) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 79657181AEF1D for ; Wed, 3 Feb 2021 18:20:59 +0000 (UTC) X-FDA: 77777773038.24.noise49_0709231275d5 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin24.hostedemail.com (Postfix) with ESMTP id 54FB61A4A0 for ; Wed, 3 Feb 2021 18:20:59 +0000 (UTC) X-HE-Tag: noise49_0709231275d5 X-Filterd-Recvd-Size: 9188 Received: from mail-il1-f178.google.com (mail-il1-f178.google.com [209.85.166.178]) by imf14.hostedemail.com (Postfix) with ESMTP for ; Wed, 3 Feb 2021 18:20:58 +0000 (UTC) Received: by mail-il1-f178.google.com with SMTP id q5so115621ilc.10 for ; Wed, 03 Feb 2021 10:20:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=t15/2iQDwVaTBOunEsPSucc/nRrzJFxeeAfzfc0DJNE=; b=wOW8ILuVngAUBSdPy/ecmjBNJOfiMVtVA9nyDK8/THmrxZvlGUv/rVjlZsCQpAin3H +28x+whbEIs/N5KX70/rzYdBxSHXSgc7EECh63rgsTZZaT/2P89+yBaEGZEqhWUeNCdE rEuYf8O+4OksgXgQjVngIY7ZvfK3bo7M8VP5hcEuEhfKfnaZdLJNCmPBkv4MRvE08aTX 9MAftkVyMN+16pvN8SUua7qKGEwKZkmQHOX3H6aGTVqxTu4jnOk/g6I6jNOcHkrq5VEa a0q1sUmYU+v/8wywE82tZx9VBkVGaJC3KUN4NLRUb2v1ER98XP/oFSveycRGmbQnchEi GPng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=t15/2iQDwVaTBOunEsPSucc/nRrzJFxeeAfzfc0DJNE=; b=sxQOfYvos64uIfUTxoQMQI848HIlIz5C7wFzs9KLq+pj6dgMNbn82UDVq0Bib5qmWZ qAynKDYbl0KL9LhX99J7Hkhfm/Yz301brKEplN8TvKT462zQw+VBDeEOYEE+ZXiaedlH 6hhxdtRML4V6Yp5dT7O+U1qbC2wqIWNFhhIBTKe9qjqw6fZrBrzBXjaUhLM5vAk+5Xzp kjsIFfHarmamGF+JTcRDRBUlez/eeIfzDLL/EtX1aTxuLyvbBLywSIQSVkrsrQvSa1z1 sFCqweuKJw6FHl+9u7Yl+U0iF1U22BsJ9LFVOTp3OyhDJlSkdXYhcWQYoCDS7ou5y9gV 4BBQ== X-Gm-Message-State: AOAM533Pm5kgQ0zO2WESUmLtRHd4BhLb/H7Mr4OYZOm19aQdmprZ4V3X W9ruJQ8cwexrziTla0e0Qn6sPYFMB3ynDB0ywzmXug== X-Google-Smtp-Source: ABdhPJwlFItdhRlFkSlxPRvvmGgnRhMv5qySj2o0FluBkMhPuLOsQ5+2Asugr+9haQYOMzj1LN0n2dIbGV9moq6orGc= X-Received: by 2002:a05:6e02:1c8d:: with SMTP id w13mr3621359ill.301.1612376458087; Wed, 03 Feb 2021 10:20:58 -0800 (PST) MIME-Version: 1.0 References: <20210128224819.2651899-1-axelrasmussen@google.com> <20210128224819.2651899-6-axelrasmussen@google.com> <20210201183159.GF260413@xz-x1> <20210202171515.GF6468@xz-x1> In-Reply-To: <20210202171515.GF6468@xz-x1> From: Axel Rasmussen Date: Wed, 3 Feb 2021 10:20:21 -0800 Message-ID: Subject: Re: [PATCH v3 5/9] userfaultfd: add minor fault registration mode To: Peter Xu Cc: Alexander Viro , Alexey Dobriyan , Andrea Arcangeli , Andrew Morton , Anshuman Khandual , Catalin Marinas , Chinwen Chang , Huang Ying , Ingo Molnar , Jann Horn , Jerome Glisse , Lokesh Gidra , "Matthew Wilcox (Oracle)" , Michael Ellerman , =?UTF-8?Q?Michal_Koutn=C3=BD?= , Michel Lespinasse , Mike Kravetz , Mike Rapoport , Nicholas Piggin , Shaohua Li , Shawn Anastasio , Steven Rostedt , Steven Price , Vlastimil Babka , LKML , linux-fsdevel@vger.kernel.org, Linux MM , Adam Ruprecht , Cannon Matthews , "Dr . David Alan Gilbert" , David Rientjes , Oliver Upton Content-Type: text/plain; charset="UTF-8" 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 Tue, Feb 2, 2021 at 9:15 AM Peter Xu wrote: > > On Mon, Feb 01, 2021 at 01:31:59PM -0500, Peter Xu wrote: > > On Thu, Jan 28, 2021 at 02:48:15PM -0800, Axel Rasmussen wrote: > > > This feature allows userspace to intercept "minor" faults. By "minor" > > > faults, I mean the following situation: > > > > > > Let there exist two mappings (i.e., VMAs) to the same page(s) (shared > > > memory). One of the mappings is registered with userfaultfd (in minor > > > mode), and the other is not. Via the non-UFFD mapping, the underlying > > > pages have already been allocated & filled with some contents. The UFFD > > > mapping has not yet been faulted in; when it is touched for the first > > > time, this results in what I'm calling a "minor" fault. As a concrete > > > example, when working with hugetlbfs, we have huge_pte_none(), but > > > find_lock_page() finds an existing page. > > > > > > This commit adds the new registration mode, and sets the relevant flag > > > on the VMAs being registered. In the hugetlb fault path, if we find > > > that we have huge_pte_none(), but find_lock_page() does indeed find an > > > existing page, then we have a "minor" fault, and if the VMA has the > > > userfaultfd registration flag, we call into userfaultfd to handle it. > > > > When re-read, now I'm thinking whether we should restrict the minor fault > > scenario with shared mappings always, assuming there's one mapping with uffd > > and the other one without, while the non-uffd can modify the data before an > > UFFDIO_CONTINUE kicking the uffd process. > > > > To me, it's really more about page cache and that's all.. > > > > So I'm wondering whether below would be simpler and actually clearer on > > defining minor faults, comparing to the above whole two paragraphs. For > > example, the scemantics do not actually need two mappings: > > > > For shared memory, userfaultfd missing fault used to only report the event > > if the page cache does not exist for the current fault process. Here we > > define userfaultfd minor fault as the case where the missing page fault > > does have a backing page cache (so only the pgtable entry is missing). > > > > It should not affect most of your code, but only one below [1]. > > OK it could be slightly more than that... > > E.g. we'd need to make UFFDIO_COPY to not install the write bit if it's > UFFDIO_CONTINUE and if it's private mappings. In hugetlb_mcopy_atomic_pte() now > we apply the write bit unconditionally: > > _dst_pte = make_huge_pte(dst_vma, page, dst_vma->vm_flags & VM_WRITE); > > That'll need a touch-up otherwise. > > It's just the change seems still very small so I'd slightly prefer to support > it all. However I don't want to make your series complicated and blocking it, > so please feel free to still make it shared memory if that's your preference. > The worst case is if someone would like to enable this (if with a valid user > scenario) we'd export a new uffd feature flag. > > > > > [...] > > > > > @@ -1302,9 +1301,26 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma, > > > unsigned long vm_flags) > > > { > > > /* FIXME: add WP support to hugetlbfs and shmem */ > > > - return vma_is_anonymous(vma) || > > > - ((is_vm_hugetlb_page(vma) || vma_is_shmem(vma)) && > > > - !(vm_flags & VM_UFFD_WP)); > > > + if (vm_flags & VM_UFFD_WP) { > > > + if (is_vm_hugetlb_page(vma) || vma_is_shmem(vma)) > > > + return false; > > > + } > > > + > > > + if (vm_flags & VM_UFFD_MINOR) { > > > + /* > > > + * The use case for minor registration (intercepting minor > > > + * faults) is to handle the case where a page is present, but > > > + * needs to be modified before it can be used. This requires > > > + * two mappings: one with UFFD registration, and one without. > > > + * So, it only makes sense to do this with shared memory. > > > + */ > > > + /* FIXME: Add minor fault interception for shmem. */ > > > + if (!(is_vm_hugetlb_page(vma) && (vma->vm_flags & VM_SHARED))) > > > + return false; > > > > [1] > > > > So here we also restrict the mapping be shared. My above comment on the commit > > message is also another way to ask whether we could also allow it to happen > > with non-shared mappings as long as there's a page cache. If so, we could drop > > the VM_SHARED check here. It won't affect your existing use case for sure, it > > just gives more possibility that maybe it could also be used on non-shared > > mappings due to some reason in the future. > > > > What do you think? Agreed, I don't see any reason why it can't work. The only requirement for it to be useful is, the UFFD-registered area needs to be able to "see" writes from the non-UFFD-registered area. Whether or not the UFFD-registered half is shared or not doesn't affect this. I'll include this change (and the VM_WRITE touchup described above) in a v4. > > > > The rest looks good to me. > > > > Thanks, > > > > -- > > Peter Xu > > -- > Peter Xu >