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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=unavailable 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 DBB57C43460 for ; Tue, 20 Apr 2021 18:43:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F17CB613DD for ; Tue, 20 Apr 2021 18:43:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F17CB613DD 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 4B7836B006C; Tue, 20 Apr 2021 14:43:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4417C6B006E; Tue, 20 Apr 2021 14:43:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 293516B0070; Tue, 20 Apr 2021 14:43:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0206.hostedemail.com [216.40.44.206]) by kanga.kvack.org (Postfix) with ESMTP id 048636B006C for ; Tue, 20 Apr 2021 14:43:45 -0400 (EDT) Received: from smtpin40.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 9342D12C7 for ; Tue, 20 Apr 2021 18:43:45 +0000 (UTC) X-FDA: 78053619210.40.226F2EF Received: from mail-il1-f171.google.com (mail-il1-f171.google.com [209.85.166.171]) by imf24.hostedemail.com (Postfix) with ESMTP id D015BA0003A0 for ; Tue, 20 Apr 2021 18:43:36 +0000 (UTC) Received: by mail-il1-f171.google.com with SMTP id v13so16848800ilj.8 for ; Tue, 20 Apr 2021 11:43:44 -0700 (PDT) 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=dKmdwef8NvpmmrAQ7mYZcx9GJzbbsHDuV6WIcarrWis=; b=XovCAIY0tLUPQt4UC6Ic21GdGt0n1X+eLaSd3lNkSwVf+1u+i6ZZLhWexxabjf3Jnj EQqmOWwDqiiJVAKqwIogmBe5SdHl19g94ChBFr117dDiGbyhAtvwuPdSYux0r2tBVefI Hm/vDaCnvKqyYYKlHdLRmcfT1zCJiEy/UNlt/JV4GRcJdwmDmrXLqyGhdNsgpTruIfu9 ha2MMzC5MMAVbiZMwJAwGtNPkozGamXUFFWsnd4mxvAaX+2K+ZXnc/PNHJDz2EjgGwNs 2fL+pQ2Ug8Z2SwRql4Sw4AJ6DyM7LMPDiBT9lAiouZiQ4FW5WsjW5cceFulJwG/YrEbP NBqw== 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=dKmdwef8NvpmmrAQ7mYZcx9GJzbbsHDuV6WIcarrWis=; b=bCYpvBv4BiN6P23GH0sxGkzIBusm3Tfv/U1U/xgC0VKtsPfHLRpzlhu0vmMddR6ObA Wz+dNvwzseHipA8qID9anQQoqDmzu3sDFq6WJKqmsac3ljobgZfWlYi4wbKii/30f5s6 Rv1RhH0LtISIsQ0NbBmLMdLxR1g6wtuFOYo0sbo4b4odI2m0Clss5+BtqespruyOTV0N OqhYa3q5mCed2v7HrriIbdqdRVHFEOKHOWcjHCGSnm0J8Lg+5IXN9WK5yked0kt4uZCt FVc0D4qchS1YXIPAkjbtVECHFEZ9OQgWGD2PLJ5lcmtP0QM1YzNCWVw8ZO9YKEhRwWVq WfPA== X-Gm-Message-State: AOAM533ghxmp64XSKeSSfIgLgddhtDi7UoeBcN89+Sy/Zow1Ij+46TmQ nDxzqZT4bbIcMo63NKy3mIS6bMlpRtrDx8n7YuWnIQ== X-Google-Smtp-Source: ABdhPJwcblTHChLirRlY5MfaJNxZFzYm2yu+MafO+vAOL0cPEAh7XqrjEUEoeolpoRqRpBC8WLa9S2V2Gdgq4ufuKws= X-Received: by 2002:a05:6e02:1d06:: with SMTP id i6mr20526041ila.165.1618944224270; Tue, 20 Apr 2021 11:43:44 -0700 (PDT) MIME-Version: 1.0 References: <20210413051721.2896915-1-axelrasmussen@google.com> <20210413051721.2896915-10-axelrasmussen@google.com> In-Reply-To: From: Axel Rasmussen Date: Tue, 20 Apr 2021 11:43:06 -0700 Message-ID: Subject: Re: [PATCH v2 9/9] userfaultfd/shmem: modify shmem_mcopy_atomic_pte to use install_ptes To: Hugh Dickins Cc: Alexander Viro , Andrea Arcangeli , Andrew Morton , Jerome Glisse , Joe Perches , Lokesh Gidra , Mike Kravetz , Mike Rapoport , Peter Xu , Shaohua Li , Shuah Khan , Stephen Rothwell , Wang Qing , linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org, LKML , linux-kselftest@vger.kernel.org, Linux MM , Brian Geffon , "Dr . David Alan Gilbert" , Mina Almasry , Oliver Upton Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: qwqtce9e1qw47tr1ks7qkspeiyrrf7c4 X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: D015BA0003A0 Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf24; identity=mailfrom; envelope-from=""; helo=mail-il1-f171.google.com; client-ip=209.85.166.171 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1618944216-705202 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 Fri, Apr 16, 2021 at 5:34 PM Hugh Dickins wrote: > > On Mon, 12 Apr 2021, Axel Rasmussen wrote: > > > In a previous commit, we added the mcopy_atomic_install_ptes() helper. > > This helper does the job of setting up PTEs for an existing page, to map > > it into a given VMA. It deals with both the anon and shmem cases, as > > well as the shared and private cases. > > > > In other words, shmem_mcopy_atomic_pte() duplicates a case it already > > handles. So, expose it, and let shmem_mcopy_atomic_pte() use it > > directly, to reduce code duplication. > > > > This requires that we refactor shmem_mcopy_atomic-pte() a bit: > > > > Instead of doing accounting (shmem_recalc_inode() et al) part-way > > through the PTE setup, do it beforehand. This frees up > > mcopy_atomic_install_ptes() from having to care about this accounting, > > but it does mean we need to clean it up if we get a failure afterwards > > (shmem_uncharge()). > > > > We can *almost* use shmem_charge() to do this, reducing code > > duplication. But, it does `inode->i_mapping->nrpages++`, which would > > double-count since shmem_add_to_page_cache() also does this. > > > > Signed-off-by: Axel Rasmussen > > --- > > include/linux/userfaultfd_k.h | 5 ++++ > > mm/shmem.c | 52 +++++++---------------------------- > > mm/userfaultfd.c | 25 ++++++++--------- > > 3 files changed, 27 insertions(+), 55 deletions(-) > > Very nice, and it gets better. > > > > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h > > index 794d1538b8ba..3e20bfa9ef80 100644 > > --- a/include/linux/userfaultfd_k.h > > +++ b/include/linux/userfaultfd_k.h > > @@ -53,6 +53,11 @@ enum mcopy_atomic_mode { > > MCOPY_ATOMIC_CONTINUE, > > }; > > > > +extern int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd, > > mcopy_atomic_install_pte throughout as before. > > > + struct vm_area_struct *dst_vma, > > + unsigned long dst_addr, struct page *page, > > + bool newly_allocated, bool wp_copy); > > + > > extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start, > > unsigned long src_start, unsigned long len, > > bool *mmap_changing, __u64 mode); > > diff --git a/mm/shmem.c b/mm/shmem.c > > index 3f48cb5e8404..9b12298405a4 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -2376,10 +2376,8 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, > > struct address_space *mapping = inode->i_mapping; > > gfp_t gfp = mapping_gfp_mask(mapping); > > pgoff_t pgoff = linear_page_index(dst_vma, dst_addr); > > - spinlock_t *ptl; > > void *page_kaddr; > > struct page *page; > > - pte_t _dst_pte, *dst_pte; > > int ret; > > pgoff_t max_off; > > > > @@ -2389,8 +2387,10 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, > > > > if (!*pagep) { > > page = shmem_alloc_page(gfp, info, pgoff); > > - if (!page) > > - goto out_unacct_blocks; > > + if (!page) { > > + shmem_inode_unacct_blocks(inode, 1); > > + goto out; > > + } > > > > if (!zeropage) { /* COPY */ > > page_kaddr = kmap_atomic(page); > > @@ -2430,59 +2430,27 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, > > if (ret) > > goto out_release; > > > > - _dst_pte = mk_pte(page, dst_vma->vm_page_prot); > > - if (dst_vma->vm_flags & VM_WRITE) > > - _dst_pte = pte_mkwrite(pte_mkdirty(_dst_pte)); > > - else { > > - /* > > - * We don't set the pte dirty if the vma has no > > - * VM_WRITE permission, so mark the page dirty or it > > - * could be freed from under us. We could do it > > - * unconditionally before unlock_page(), but doing it > > - * only if VM_WRITE is not set is faster. > > - */ > > - set_page_dirty(page); > > - } > > - > > - dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl); > > - > > - ret = -EFAULT; > > - max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); > > - if (unlikely(pgoff >= max_off)) > > - goto out_release_unlock; > > - > > - ret = -EEXIST; > > - if (!pte_none(*dst_pte)) > > - goto out_release_unlock; > > - > > - lru_cache_add(page); > > - > > spin_lock_irq(&info->lock); > > info->alloced++; > > inode->i_blocks += BLOCKS_PER_PAGE; > > shmem_recalc_inode(inode); > > spin_unlock_irq(&info->lock); > > > > - inc_mm_counter(dst_mm, mm_counter_file(page)); > > - page_add_file_rmap(page, false); > > - set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte); > > + ret = mcopy_atomic_install_ptes(dst_mm, dst_pmd, dst_vma, dst_addr, > > + page, true, false); > > + if (ret) > > + goto out_release_uncharge; > > > > - /* No need to invalidate - it was non-present before */ > > - update_mmu_cache(dst_vma, dst_addr, dst_pte); > > - pte_unmap_unlock(dst_pte, ptl); > > In reviewing 4/9, I said to take out mcopy_atomic_install_pte()'s > set_page_dirty(). Just call it here instead, before the unlock_page(). > You have an array of choices for how to write it: > > if (!(dst_vma->vm_flags & VM_WRITE)) > set_page_dirty(page); > or > if (!(dst_vma->vm_flags & VM_WRITE)) > SetPageDirty(page); > or > set_page_dirty(page); > or > SetPageDirty(page); > > Personally, I'd go for the last: this function has just modified the > page, so it ought to mark it dirty: without second-guessing what > mcopy_atomic_install_pte() might do to make that redundant in the > VM_WRITE case. set_page_dirty() or SetPageDirty()? Some years ago > I tended to favour the former for its preparatory PageDirty check; > nowadays (on shmem) I favour the latter, to avoid the function call > indirection which became more expensive with spectre+retpoline. Now reaching this comment after looking through patch 4: Ah, this makes sense. I agree this is clearer too, as it doesn't split the concern across two files / functions. > > > unlock_page(page); > > ret = 0; > > out: > > return ret; > > -out_release_unlock: > > - pte_unmap_unlock(dst_pte, ptl); > > - ClearPageDirty(page); > > +out_release_uncharge: > > Given how 4/9 was, this did still need the ClearPageDirty() > before delete_from_page_cache(), to prevent a warning. > But not needed here if SPD is done just before unlock_page(). > > > delete_from_page_cache(page); > > + shmem_uncharge(inode, 1); > > out_release: > > unlock_page(page); > > put_page(page); > > -out_unacct_blocks: > > - shmem_inode_unacct_blocks(inode, 1); > > goto out; > > } > > #endif /* CONFIG_USERFAULTFD */ > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 8df0438f5d6a..3f73ba0b99f0 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -51,18 +51,13 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm, > > /* > > * Install PTEs, to map dst_addr (within dst_vma) to page. > > * > > - * This function handles MCOPY_ATOMIC_CONTINUE (which is always file-backed), > > - * whether or not dst_vma is VM_SHARED. It also handles the more general > > - * MCOPY_ATOMIC_NORMAL case, when dst_vma is *not* VM_SHARED (it may be file > > - * backed, or not). > > - * > > - * Note that MCOPY_ATOMIC_NORMAL for a VM_SHARED dst_vma is handled by > > - * shmem_mcopy_atomic_pte instead. > > + * This function handles both MCOPY_ATOMIC_NORMAL and _CONTINUE for both shmem > > + * and anon, and for both shared and private VMAs. > > */ > > -static int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd, > > - struct vm_area_struct *dst_vma, > > - unsigned long dst_addr, struct page *page, > > - bool newly_allocated, bool wp_copy) > > +int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd, > > + struct vm_area_struct *dst_vma, > > + unsigned long dst_addr, struct page *page, > > + bool newly_allocated, bool wp_copy) > > { > > int ret; > > pte_t _dst_pte, *dst_pte; > > @@ -116,8 +111,12 @@ static int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd, > > else > > page_add_new_anon_rmap(page, dst_vma, dst_addr, false); > > > > - if (newly_allocated) > > - lru_cache_add_inactive_or_unevictable(page, dst_vma); > > + if (newly_allocated) { > > + if (vma_is_shmem(dst_vma) && vm_shared) > > + lru_cache_add(page); > > + else > > + lru_cache_add_inactive_or_unevictable(page, dst_vma); > > + } > > This change is not required, you'll be glad to hear. Take a look at > lru_cache_add_inactive_or_unevictable() in mm/swap.c: it's a wrapper > to set PageMlocked and do associated accounting (if appropriate) before > doing the lru_cache_add(page). And, strictly speaking, should have been > used in shmem_mcopy_atomic_pte() all along. > > I say "strictly speaking" because it would be exceptional to find a page > needing Mlock there, and not an error to delay that Mlock until sometime > later, before reclaim. Worth a sentence in the commit message, > but not worth a "Fixes:" tag. This makes sense. I had considered doing this before, but took a brief look at the functions and saw they were indeed different. But looking more closely, you're right that the wrapper is only different if some particular conditions are true, and so is fine to use in other cases. > > > > > set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte); > > > > -- > > 2.31.1.295.g9ea45b61b8-goog