| Message ID | 20210901205622.6935-2-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: A few cleanup patches around zap, shmem and uffd | expand |
On Wed, Sep 1, 2021 at 1:56 PM Peter Xu <peterx@redhat.com> 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/ Thanks for the cleanup Peter! I think the discussion of whether or not the data can be marked dirty below is correct, and the code change looks good as well. But, I think we're missing an explanation why Hugh's concern is indeed not a problem? Specifically, this question: "Haha: I think Andrea is referring to exactly the dirty_accountable code in change_pte_protection() which worried me above. Now, I think that will turn out okay (shmem does not have a page_mkwrite(), and does not participate in dirty accounting), but you will have to do some work to assure us all of that, before sending in a cleanup patch." Do we have more evidence that this is indeed fine, vs. what we had when discussing this before? If so, we should talk about it explicitly in this commit message, I think. (Sorry if you've covered this and it's just going over my head. ;) ) > > > > 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(-) > > 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); > -- > 2.31.1 >
Hi, Axel, On Wed, Sep 01, 2021 at 02:48:53PM -0700, Axel Rasmussen wrote: > On Wed, Sep 1, 2021 at 1:56 PM Peter Xu <peterx@redhat.com> 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/ > > Thanks for the cleanup Peter! No problem. Obviously that special handling of SetPageDirty is still too tricky to me and I'd love to remove it. > > I think the discussion of whether or not the data can be marked dirty > below is correct, and the code change looks good as well. But, I think > we're missing an explanation why Hugh's concern is indeed not a > problem? > > Specifically, this question: > > "Haha: I think Andrea is referring to exactly the dirty_accountable > code in change_pte_protection() which worried me above. Now, I think > that will turn out okay (shmem does not have a page_mkwrite(), and > does not participate in dirty accounting), but you will have to do > some work to assure us all of that, before sending in a cleanup > patch." > > Do we have more evidence that this is indeed fine, vs. what we had > when discussing this before? If so, we should talk about it explicitly > in this commit message, I think. > > (Sorry if you've covered this and it's just going over my head. ;) ) Thanks for looking into this. I thought Hugh's explanation should mostly have covered that. The previous worry is we may have mprotect() applying write bit errornously if we have some read-only pte marked dirty. But I don't think that'll happen just like Hugh stated in the thread I attached, as the dirty accountable flag is only set if vma_wants_writenotify() returns true. Take the first example within that helper: if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED))) return 0; So firstly it never applies to vma that doesn't have VM_WRITE|VM_SHARED. So far it even doesn't work for anonymous, but logically it may, like: https://github.com/aagit/aa/commit/05dc2c56ef79b3836c75fcf68c5b19b08f4e4c58 Peter Collingbourne originated that patch, due to some reason it didn't land which I forgot, however I still think it's doable even for anonymous. Sorry to have gone off-topic; let me go back to it. It also checks for e.g. page_mkwrite() needs, soft dirty tracking and so on to make sure it's okay to grant write bit when possible. Hugh mentioned "do some work to assure us all of that" - I did firstly went throught the code carefully myself so I'm more certain it's doing the right thing to me, secondly I did run quite some tests on the patch (actually on the whole uffd-wp shmem+hugetlbfs branch). Even if I'm going to switch the uffd-wp series to the pte marker format, this patch won't change. I also analysized three callers that may be affected by this change below, and explaining why it's okay. I hope that can also be counted as part of the "some work" that Hugh asked. Besides all these, I'm pretty happy too if anyone would help me to tell otherwise on whether there's still things missing so we can't do this. That's the "code review" part for every single patch, including this one, isn't it? :) Thanks, > > > > > > > > > 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.
On Wed, Sep 1, 2021 at 4:00 PM Peter Xu <peterx@redhat.com> wrote: > > Hi, Axel, > > On Wed, Sep 01, 2021 at 02:48:53PM -0700, Axel Rasmussen wrote: > > On Wed, Sep 1, 2021 at 1:56 PM Peter Xu <peterx@redhat.com> 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/ > > > > Thanks for the cleanup Peter! > > No problem. Obviously that special handling of SetPageDirty is still too > tricky to me and I'd love to remove it. > > > > > I think the discussion of whether or not the data can be marked dirty > > below is correct, and the code change looks good as well. But, I think > > we're missing an explanation why Hugh's concern is indeed not a > > problem? > > > > Specifically, this question: > > > > "Haha: I think Andrea is referring to exactly the dirty_accountable > > code in change_pte_protection() which worried me above. Now, I think > > that will turn out okay (shmem does not have a page_mkwrite(), and > > does not participate in dirty accounting), but you will have to do > > some work to assure us all of that, before sending in a cleanup > > patch." > > > > Do we have more evidence that this is indeed fine, vs. what we had > > when discussing this before? If so, we should talk about it explicitly > > in this commit message, I think. > > > > (Sorry if you've covered this and it's just going over my head. ;) ) > > Thanks for looking into this. > > I thought Hugh's explanation should mostly have covered that. The previous > worry is we may have mprotect() applying write bit errornously if we have some > read-only pte marked dirty. But I don't think that'll happen just like Hugh > stated in the thread I attached, as the dirty accountable flag is only set if > vma_wants_writenotify() returns true. > > Take the first example within that helper: > > if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED))) > return 0; > > So firstly it never applies to vma that doesn't have VM_WRITE|VM_SHARED. So far > it even doesn't work for anonymous, but logically it may, like: > > https://github.com/aagit/aa/commit/05dc2c56ef79b3836c75fcf68c5b19b08f4e4c58 > > Peter Collingbourne originated that patch, due to some reason it didn't land > which I forgot, however I still think it's doable even for anonymous. > > Sorry to have gone off-topic; let me go back to it. > > It also checks for e.g. page_mkwrite() needs, soft dirty tracking and so on to > make sure it's okay to grant write bit when possible. > > Hugh mentioned "do some work to assure us all of that" - I did firstly went > throught the code carefully myself so I'm more certain it's doing the right > thing to me, secondly I did run quite some tests on the patch (actually on the > whole uffd-wp shmem+hugetlbfs branch). Even if I'm going to switch the uffd-wp > series to the pte marker format, this patch won't change. > > I also analysized three callers that may be affected by this change below, and > explaining why it's okay. I hope that can also be counted as part of the "some > work" that Hugh asked. > > Besides all these, I'm pretty happy too if anyone would help me to tell > otherwise on whether there's still things missing so we can't do this. That's > the "code review" part for every single patch, including this one, isn't it? :) Makes sense. :) Allow me to think out loud for a moment; about the three UFFD cases: I agree case (2) mcopy_atomic_pte has !page_in_cache (by definition, since COPY specifically means inserting a new page), and so is not changing functionally. I'll ignore this case. Case (1) shmem_mfill_atomic_pte is only called for shmem VMAs, with VM_SHARED, and with mode being one of copy (NORMAL) or ZEROPAGE. This case is interesting, because like case (2) we certainly have !page_in_cache, so we were always doing pte_mkdirty(). The difference here is we now no longer *also* SetPageDirty(). What does it mean for a PTE to be dirty, but not the page (or the other way around)? I confess I don't know. But, at least as far as mprotect is concerned, it seems to not care about PageDirty really. It does check it in one place, "if (page_is_file_lru(page) && PageDirty(page))", but "page_is_file_lru" really means "is the page *not* swap backed", so I think shmem pages are never file lru? If true, then this check is never true for shmem anyway, regardless of whether we SetPageDirty or not, and can be ignored. So, for case (1) I think there's no behavior change, at least with regard to mprotect(). Case (3) mcontinue_atomic_pte handles CONTINUE for all shmem VMAs. Meaning, we by definition have page_in_cache. It also seems to me we could have any combination of VM_SHARED or not, VM_WRITE or not, PTE writeable or not. I have to admit, I am a little bit fuzzy on exactly what PageDirty means for shmem (since there is no "filesystem", it's all just RAM). I think being "dirty" in this case means, if we wanted to reclaim the page, we would need to *swap it out*. I.e., shmem pages are always "dirty", as long as they are in the page cache (and have been modified at least once since being allocated? or maybe we don't bother keeping track of zero pages). So, this patch changes the behavior for case (3), when we end up with !writable (i.e., !(VM_WRITE && VM_SHARED) -- note we check VM_SHARED further down). We used to *skip* pte_mkdirty() if !writable, but now we do it after all. But, for mprotect to do its thing, it requires vma_wants_writenotify() -- but if we have !VM_WRITE, or !VM_SHARED, it returns false. So in the only case where case (3) actually does something different, mprotect *doesn't* do anything. Okay, I've convinced myself. :) Sorry for the long e-mail. Feel free to take (for v2 as well, this patch didn't change): Reviewed-by: Axel Rasmussen <axelrasmussen@google.com> > > Thanks, > > > > > > > > > > > > > > > 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. > > -- > Peter Xu >
On Thu, Sep 02, 2021 at 02:54:12PM -0700, Axel Rasmussen wrote: > On Wed, Sep 1, 2021 at 4:00 PM Peter Xu <peterx@redhat.com> wrote: > > > > Hi, Axel, > > > > On Wed, Sep 01, 2021 at 02:48:53PM -0700, Axel Rasmussen wrote: > > > On Wed, Sep 1, 2021 at 1:56 PM Peter Xu <peterx@redhat.com> 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/ > > > > > > Thanks for the cleanup Peter! > > > > No problem. Obviously that special handling of SetPageDirty is still too > > tricky to me and I'd love to remove it. > > > > > > > > I think the discussion of whether or not the data can be marked dirty > > > below is correct, and the code change looks good as well. But, I think > > > we're missing an explanation why Hugh's concern is indeed not a > > > problem? > > > > > > Specifically, this question: > > > > > > "Haha: I think Andrea is referring to exactly the dirty_accountable > > > code in change_pte_protection() which worried me above. Now, I think > > > that will turn out okay (shmem does not have a page_mkwrite(), and > > > does not participate in dirty accounting), but you will have to do > > > some work to assure us all of that, before sending in a cleanup > > > patch." > > > > > > Do we have more evidence that this is indeed fine, vs. what we had > > > when discussing this before? If so, we should talk about it explicitly > > > in this commit message, I think. > > > > > > (Sorry if you've covered this and it's just going over my head. ;) ) > > > > Thanks for looking into this. > > > > I thought Hugh's explanation should mostly have covered that. The previous > > worry is we may have mprotect() applying write bit errornously if we have some > > read-only pte marked dirty. But I don't think that'll happen just like Hugh > > stated in the thread I attached, as the dirty accountable flag is only set if > > vma_wants_writenotify() returns true. > > > > Take the first example within that helper: > > > > if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED))) > > return 0; > > > > So firstly it never applies to vma that doesn't have VM_WRITE|VM_SHARED. So far > > it even doesn't work for anonymous, but logically it may, like: > > > > https://github.com/aagit/aa/commit/05dc2c56ef79b3836c75fcf68c5b19b08f4e4c58 > > > > Peter Collingbourne originated that patch, due to some reason it didn't land > > which I forgot, however I still think it's doable even for anonymous. > > > > Sorry to have gone off-topic; let me go back to it. > > > > It also checks for e.g. page_mkwrite() needs, soft dirty tracking and so on to > > make sure it's okay to grant write bit when possible. > > > > Hugh mentioned "do some work to assure us all of that" - I did firstly went > > throught the code carefully myself so I'm more certain it's doing the right > > thing to me, secondly I did run quite some tests on the patch (actually on the > > whole uffd-wp shmem+hugetlbfs branch). Even if I'm going to switch the uffd-wp > > series to the pte marker format, this patch won't change. > > > > I also analysized three callers that may be affected by this change below, and > > explaining why it's okay. I hope that can also be counted as part of the "some > > work" that Hugh asked. > > > > Besides all these, I'm pretty happy too if anyone would help me to tell > > otherwise on whether there's still things missing so we can't do this. That's > > the "code review" part for every single patch, including this one, isn't it? :) > > Makes sense. :) Allow me to think out loud for a moment; about the > three UFFD cases: > > I agree case (2) mcopy_atomic_pte has !page_in_cache (by definition, > since COPY specifically means inserting a new page), and so is not > changing functionally. I'll ignore this case. > > > > Case (1) shmem_mfill_atomic_pte is only called for shmem VMAs, with > VM_SHARED, and with mode being one of copy (NORMAL) or ZEROPAGE. This > case is interesting, because like case (2) we certainly have > !page_in_cache, so we were always doing pte_mkdirty(). The difference > here is we now no longer *also* SetPageDirty(). What does it mean for > a PTE to be dirty, but not the page (or the other way around)? I > confess I don't know. The PTE being dirty should be fine to cover the PageDirty case. When the PTE is zapped, we'll need to move that pte dirty bit to PageDirty no matter when we zap that PTE. E.g. see zap_pte_range(), here is: if (!PageAnon(page)) { if (pte_dirty(ptent)) { force_flush = 1; set_page_dirty(page); } ... } So IMHO you can see it as a delayed (but unified) version of setting page PG_dirty. > > But, at least as far as mprotect is concerned, it seems to not care > about PageDirty really. It does check it in one place, "if > (page_is_file_lru(page) && PageDirty(page))", but "page_is_file_lru" > really means "is the page *not* swap backed", so I think shmem pages > are never file lru? If true, then this check is never true for shmem > anyway, regardless of whether we SetPageDirty or not, and can be > ignored. > > So, for case (1) I think there's no behavior change, at least with > regard to mprotect(). The behavior change is that the pte does not have the dirty bit set before this patch for this case, now it will have. I think that's also why Hugh explained why it's okay, and what's the source of the discussion that we used to think it might have an impact - that originates from Andrea's patch on the 1st usage of SetPageDirty within UFFDIO_COPY shmem path. > > > > Case (3) mcontinue_atomic_pte handles CONTINUE for all shmem VMAs. > Meaning, we by definition have page_in_cache. It also seems to me we > could have any combination of VM_SHARED or not, VM_WRITE or not, PTE > writeable or not. > > I have to admit, I am a little bit fuzzy on exactly what PageDirty > means for shmem (since there is no "filesystem", it's all just RAM). I > think being "dirty" in this case means, if we wanted to reclaim the > page, we would need to *swap it out*. Yes, at least that's how I understand it too. If the page doesn't have dirty bit set, it means it can be thrown away at any time. See pageout() calling clear_page_dirty_for_io(), and its own stack when doing page reclaim. > I.e., shmem pages are always > "dirty", as long as they are in the page cache (and have been modified > at least once since being allocated? or maybe we don't bother keeping > track of zero pages). > > So, this patch changes the behavior for case (3), when we end up with > !writable (i.e., !(VM_WRITE && VM_SHARED) -- note we check VM_SHARED > further down). We used to *skip* pte_mkdirty() if !writable, but now > we do it after all. But, for mprotect to do its thing, it requires > vma_wants_writenotify() -- but if we have !VM_WRITE, or !VM_SHARED, it > returns false. So in the only case where case (3) actually does > something different, mprotect *doesn't* do anything. Yes, I think that could be a slight difference for !write case but IMHO it's fine too, it's the case when process A maps the shmem with RO but process B maps with RW: process B should be the one that updates page and sends UFFDIO_CONTINUE; and process B mustn't map it RO because otherwise why bother if it can't do anything! It means page cache must be dirty anyway irrelevant of dirty bit in process A. So I think that's not a general use case for minor fault either as I should have expressed in the commit message. Keeping that "optimization on conditionally set dirty bit" is okay but it just makes things complicated and it'll grow more complex when uffd-wp shmem comes. That is not worth it. I figured it's easier to set dirty always - no reason to fight data loss again. > > > > Okay, I've convinced myself. :) Sorry for the long e-mail. Feel free > to take (for v2 as well, this patch didn't change): > > Reviewed-by: Axel Rasmussen <axelrasmussen@google.com> Thank you, Axel. I'll collect that in v3.
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(-)