Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Axel Rasmussen <axelrasmussen@google.com>
Cc: Peter Xu <peterx@redhat.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Hugh Dickins <hughd@google.com>,
	Jerome Glisse <jglisse@redhat.com>, Joe Perches <joe@perches.com>,
	Lokesh Gidra <lokeshgidra@google.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>, Shaohua Li <shli@fb.com>,
	Shuah Khan <shuah@kernel.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Wang Qing <wangqing@vivo.com>,
	linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-mm@kvack.org, Brian Geffon <bgeffon@google.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	Mina Almasry <almasrymina@google.com>,
	Oliver Upton <oupton@google.com>
Subject: Re: [PATCH v4 09/10] userfaultfd/shmem: modify shmem_mcopy_atomic_pte to use install_pte()
Date: Mon, 26 Apr 2021 19:59:04 -0700 (PDT)
Message-ID: <alpine.LSU.2.11.2104261927570.2998@eggly.anvils> (raw)
In-Reply-To: <20210421010205.GH4440@xz-x1>

On Tue, 20 Apr 2021, Peter Xu wrote:
> On Tue, Apr 20, 2021 at 03:08:03PM -0700, Axel Rasmussen wrote:
> > In a previous commit, we added the mcopy_atomic_install_pte() 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_pte() 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.
> 
> Missing to mention the lru_cache_add() replacement comment as Hugh commented on
> this?

Yes, please add that mention.

And personally, I'd much prefer this and the Doc commit,
the non-selftest commits, to be moved up before the selftests.

The selftest situation is confusing at present, since Peter's series
got dropped as [to-be-updated] from mmotm, so we're waiting for those
updates to be added back to mmotm before this series can go in.

(But akpm will be busy with merge window stuff at present:
now is not the time to be adding or re-adding a series.)

> 
> > 
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>

Peter raised a good point below, so no Ack from me yet.

> > ---
> >  include/linux/userfaultfd_k.h |  5 ++++
> >  mm/shmem.c                    | 53 ++++++++---------------------------
> >  mm/userfaultfd.c              | 17 ++++-------
> >  3 files changed, 22 insertions(+), 53 deletions(-)
> > 
> > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > index 794d1538b8ba..39c094cc6641 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_pte(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);
> > +
> >  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 30c0bb501dc9..9bfa80fcd414 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2378,10 +2378,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;
> >  
> > @@ -2391,8 +2389,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);
> > @@ -2432,59 +2432,28 @@ 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);

I think it's best to move that info->locked block down to above the
new SetPageDirty(), where we know we have succeeded, because...

> >  
> > -	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_pte(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);
> > +	SetPageDirty(page);
> >  	unlock_page(page);
> >  	ret = 0;
> >  out:
> >  	return ret;
> > -out_release_unlock:
> > -	pte_unmap_unlock(dst_pte, ptl);
> > -	ClearPageDirty(page);
> > +out_release_uncharge:
> >  	delete_from_page_cache(page);
> > +	shmem_uncharge(inode, 1);
> >  out_release:
> >  	unlock_page(page);
> >  	put_page(page);
> > -out_unacct_blocks:
> 
> Will all the "goto out_release" miss one call to shmem_inode_unacct_blocks()?

Good catch, yes.

Which is why I suggest above to move the info->locked block down:
then you can forget about using shmem_uncharge(), and just use
shmem_inode_unacct_blocks() directly in all cases that need it here.

(But I notice the commit message refers to shmem_uncharge(), so that
will need adjusting. shmem_uncharge() is more of a hack than a proper
interface, introduced to manage certain THP split/collapse cases.)

> 
> > -	shmem_inode_unacct_blocks(inode, 1);
> >  	goto out;
> >  }
> >  #endif /* CONFIG_USERFAULTFD */

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 22:07 [PATCH v4 00/10] userfaultfd: add minor fault handling for shmem Axel Rasmussen
2021-04-20 22:07 ` [PATCH v4 01/10] userfaultfd/hugetlbfs: avoid including userfaultfd_k.h in hugetlb.h Axel Rasmussen
2021-04-27  2:05   ` Hugh Dickins
2021-04-20 22:07 ` [PATCH v4 02/10] userfaultfd/shmem: combine shmem_{mcopy_atomic,mfill_zeropage}_pte Axel Rasmussen
2021-04-20 22:07 ` [PATCH v4 03/10] userfaultfd/shmem: support UFFDIO_CONTINUE for shmem Axel Rasmussen
2021-04-22 20:22   ` Axel Rasmussen
2021-04-22 21:18     ` Peter Xu
2021-04-22 22:05       ` Axel Rasmussen
2021-04-27  2:19   ` Hugh Dickins
2021-04-27 15:54     ` Peter Xu
2021-04-27 16:57       ` Axel Rasmussen
2021-04-27 18:03         ` Peter Xu
2021-04-27 20:29           ` Axel Rasmussen
2021-04-27 20:42             ` Peter Xu
2021-04-27 20:59               ` Hugh Dickins
2021-04-20 22:07 ` [PATCH v4 04/10] userfaultfd/shmem: support minor fault registration " Axel Rasmussen
2021-04-27  2:23   ` Hugh Dickins
2021-04-27 15:57     ` Peter Xu
2021-04-27 17:03       ` Axel Rasmussen
2021-04-20 22:07 ` [PATCH v4 05/10] userfaultfd/selftests: use memfd_create for shmem test type Axel Rasmussen
2021-04-20 22:08 ` [PATCH v4 06/10] userfaultfd/selftests: create alias mappings in the shmem test Axel Rasmussen
2021-04-20 22:08 ` [PATCH v4 07/10] userfaultfd/selftests: reinitialize test context in each test Axel Rasmussen
2021-04-20 22:08 ` [PATCH v4 08/10] userfaultfd/selftests: exercise minor fault handling shmem support Axel Rasmussen
2021-04-20 22:08 ` [PATCH v4 09/10] userfaultfd/shmem: modify shmem_mcopy_atomic_pte to use install_pte() Axel Rasmussen
2021-04-21  1:02   ` Peter Xu
2021-04-27  2:59     ` Hugh Dickins [this message]
2021-04-20 22:08 ` [PATCH v4 10/10] userfaultfd: update documentation to mention shmem minor faults Axel Rasmussen
2021-04-27  2:27   ` Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.11.2104261927570.2998@eggly.anvils \
    --to=hughd@google.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=axelrasmussen@google.com \
    --cc=bgeffon@google.com \
    --cc=dgilbert@redhat.com \
    --cc=jglisse@redhat.com \
    --cc=joe@perches.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=lokeshgidra@google.com \
    --cc=mike.kravetz@oracle.com \
    --cc=oupton@google.com \
    --cc=peterx@redhat.com \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=sfr@canb.auug.org.au \
    --cc=shli@fb.com \
    --cc=shuah@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=wangqing@vivo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git