On Thu, Apr 08, 2021 at 04:43:20PM -0700, Axel Rasmussen wrote:
Previously, we did a dance where we had one calling path in userfaultfd.c (mfill_atomic_pte), but then we split it into two in shmem_fs.h (shmem_{mcopy_atomic,mfill_zeropage}_pte), and then rejoined into a single shared function in shmem.c (shmem_mfill_atomic_pte).
This is all a bit overly complex. Just call the single combined shmem function directly, allowing us to clean up various branches, boilerplate, etc.
While we're touching this function, two other small cleanup changes:
- offset is equivalent to pgoff, so we can get rid of offset entirely.
- Split two VM_BUG_ON cases into two statements. This means the line number reported when the BUG is hit specifies exactly which condition was true.
(For my own preference, I'll avoid touching the latter one)
Signed-off-by: Axel Rasmussen axelrasmussen@google.com
include/linux/shmem_fs.h | 15 +++++------- mm/shmem.c | 52 +++++++++++++--------------------------- mm/userfaultfd.c | 10 +++----- 3 files changed, 25 insertions(+), 52 deletions(-)
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index d82b6f396588..919e36671fe6 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -122,21 +122,18 @@ static inline bool shmem_file(struct file *file) extern bool shmem_charge(struct inode *inode, long pages); extern void shmem_uncharge(struct inode *inode, long pages); +#ifdef CONFIG_USERFAULTFD #ifdef CONFIG_SHMEM extern int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, struct vm_area_struct *dst_vma, unsigned long dst_addr, unsigned long src_addr,
Not a problem of your patch, but it's just that we passed in odd src_addr values into mfill_atomic_pte() for zeropage case because we loop on src_addr in __mcopy_atomic()... Then it'll further passed into shmem_mcopy_atomic_pte() now after this patch (as shmem_mfill_zeropage_pte() probably only did one thing good which is to clear src_addr). Not a big deal, though.
All the rest looks sane to me.
Reviewed-by: Peter Xu peterx@redhat.com
I'll wait to look at the selftests since in all cases they should be prone to rebase (either based on the v2 cleanup I posted, or you'd need to post without err() - then I can rebase again), so I figured maybe I just read the new version.
Thanks,