| Message ID | 20210902201721.52796-2-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: A few cleanup patches around zap, shmem and uffd | expand |
On 02.09.21 22:17, Peter Xu wrote: > It was conditionally done previously, as there's one shmem special case that we > use SetPageDirty() instead. However that's not necessary and it should be > easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). > > The most recent discussion about this is here, where Hugh explained the history > of SetPageDirty() and why it's possible that it's not required at all: > > https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ > > Currently mfill_atomic_install_pte() has three callers: > > 1. shmem_mfill_atomic_pte > 2. mcopy_atomic_pte > 3. mcontinue_atomic_pte > > After the change: case (1) should have its SetPageDirty replaced by the dirty > bit on pte (so we unify them together, finally), case (2) should have no > functional change at all as it has page_in_cache==false, case (3) may add a > dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, > it's merely 100% sure the page is dirty after all, so should not make a real > difference either. Would it be worth adding VM_BUG_ON() to make sure that "100%" is really the case? > > This should make it much easier to follow on which case will set dirty for > uffd, as we'll simply set it all now for all uffd related ioctls. Meanwhile, > no special handling of SetPageDirty() if there's no need. To me this all sounds sane, but I'm certainly not an expert on that code, so ...
On Fri, Sep 03, 2021 at 09:42:34AM +0200, David Hildenbrand wrote: > On 02.09.21 22:17, Peter Xu wrote: > > It was conditionally done previously, as there's one shmem special case that we > > use SetPageDirty() instead. However that's not necessary and it should be > > easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). > > > > The most recent discussion about this is here, where Hugh explained the history > > of SetPageDirty() and why it's possible that it's not required at all: > > > > https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ > > > > Currently mfill_atomic_install_pte() has three callers: > > > > 1. shmem_mfill_atomic_pte > > 2. mcopy_atomic_pte > > 3. mcontinue_atomic_pte > > > > After the change: case (1) should have its SetPageDirty replaced by the dirty > > bit on pte (so we unify them together, finally), case (2) should have no > > functional change at all as it has page_in_cache==false, case (3) may add a > > dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, > > it's merely 100% sure the page is dirty after all, so should not make a real > > difference either. > > Would it be worth adding VM_BUG_ON() to make sure that "100%" is really the > case? I won't be able to make it 100% sure (and that's where I put it "merely"). The example discussed between Axel and me in the other thread could be an outlier (when two processes, uffd target, and uffd minor resolver, map the region as RO), it's just that neither do I think that's a great matter, nor do I think it would be worth a BUG_ON(), not to mention we use BUG_ON so carefully. > > > > > This should make it much easier to follow on which case will set dirty for > > uffd, as we'll simply set it all now for all uffd related ioctls. Meanwhile, > > no special handling of SetPageDirty() if there's no need. > > To me this all sounds sane, but I'm certainly not an expert on that code, so > ... No problem. I hope this patch didn't bring much headache to a lot of people. It's just that I do think this is the right thing to do so I will insist until someone says no to me. Already appreciate a lot for all the comments and r-bs!
On 03.09.21 22:00, Peter Xu wrote: > On Fri, Sep 03, 2021 at 09:42:34AM +0200, David Hildenbrand wrote: >> On 02.09.21 22:17, Peter Xu wrote: >>> It was conditionally done previously, as there's one shmem special case that we >>> use SetPageDirty() instead. However that's not necessary and it should be >>> easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). >>> >>> The most recent discussion about this is here, where Hugh explained the history >>> of SetPageDirty() and why it's possible that it's not required at all: >>> >>> https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ >>> >>> Currently mfill_atomic_install_pte() has three callers: >>> >>> 1. shmem_mfill_atomic_pte >>> 2. mcopy_atomic_pte >>> 3. mcontinue_atomic_pte >>> >>> After the change: case (1) should have its SetPageDirty replaced by the dirty >>> bit on pte (so we unify them together, finally), case (2) should have no >>> functional change at all as it has page_in_cache==false, case (3) may add a >>> dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, >>> it's merely 100% sure the page is dirty after all, so should not make a real >>> difference either. >> >> Would it be worth adding VM_BUG_ON() to make sure that "100%" is really the >> case? > > I won't be able to make it 100% sure (and that's where I put it "merely"). The > example discussed between Axel and me in the other thread could be an outlier > (when two processes, uffd target, and uffd minor resolver, map the region as > RO), it's just that neither do I think that's a great matter, nor do I think it > would be worth a BUG_ON(), not to mention we use BUG_ON so carefully. Agreed then, if we really expect there are corner cases and that the corner cases are fine! (VM_BUG_ON() could have helped to catch these while testing)
diff --git a/mm/shmem.c b/mm/shmem.c index dacda7463d54..3f91c8ce4d02 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2437,7 +2437,6 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, shmem_recalc_inode(inode); spin_unlock_irq(&info->lock); - SetPageDirty(page); unlock_page(page); return 0; out_delete_from_cache: diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 0e2132834bc7..b30a3724c701 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -69,10 +69,9 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, pgoff_t offset, max_off; _dst_pte = mk_pte(page, dst_vma->vm_page_prot); + _dst_pte = pte_mkdirty(_dst_pte); if (page_in_cache && !vm_shared) writable = false; - if (writable || !page_in_cache) - _dst_pte = pte_mkdirty(_dst_pte); if (writable) { if (wp_copy) _dst_pte = pte_mkuffd_wp(_dst_pte);
It was conditionally done previously, as there's one shmem special case that we use SetPageDirty() instead. However that's not necessary and it should be easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). The most recent discussion about this is here, where Hugh explained the history of SetPageDirty() and why it's possible that it's not required at all: https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ Currently mfill_atomic_install_pte() has three callers: 1. shmem_mfill_atomic_pte 2. mcopy_atomic_pte 3. mcontinue_atomic_pte After the change: case (1) should have its SetPageDirty replaced by the dirty bit on pte (so we unify them together, finally), case (2) should have no functional change at all as it has page_in_cache==false, case (3) may add a dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, it's merely 100% sure the page is dirty after all, so should not make a real difference either. This should make it much easier to follow on which case will set dirty for uffd, as we'll simply set it all now for all uffd related ioctls. Meanwhile, no special handling of SetPageDirty() if there's no need. Cc: Hugh Dickins <hughd@google.com> Cc: Axel Rasmussen <axelrasmussen@google.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/shmem.c | 1 - mm/userfaultfd.c | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-)