Message ID | 20250113033901.68951-2-21cnbao@gmail.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | mm: batched unmap lazyfree large folios during reclamation | expand |
On 13.01.25 04:38, Barry Song wrote: > From: Barry Song <v-songbaohua@oppo.com> > > The refcount may be temporarily or long-term increased, but this does > not change the fundamental nature of the folio already being lazy- > freed. Therefore, we only reset 'swapbacked' when we are certain the > folio is dirty and not droppable. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > --- Acked-by: David Hildenbrand <david@redhat.com>
On 13.01.25 14:19, David Hildenbrand wrote: > On 13.01.25 04:38, Barry Song wrote: >> From: Barry Song <v-songbaohua@oppo.com> >> >> The refcount may be temporarily or long-term increased, but this does >> not change the fundamental nature of the folio already being lazy- >> freed. Therefore, we only reset 'swapbacked' when we are certain the >> folio is dirty and not droppable. >> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Barry Song <v-songbaohua@oppo.com> >> --- > > Acked-by: David Hildenbrand <david@redhat.com> > Ah, should this have a Fixes: ? Because of a speculative reference, we might not reclaim MADV_FREE folios as we silently mark them as swapbacked again, which sounds fairly wrong.
On Tue, Jan 14, 2025 at 2:21 AM David Hildenbrand <david@redhat.com> wrote: > > On 13.01.25 14:19, David Hildenbrand wrote: > > On 13.01.25 04:38, Barry Song wrote: > >> From: Barry Song <v-songbaohua@oppo.com> > >> > >> The refcount may be temporarily or long-term increased, but this does > >> not change the fundamental nature of the folio already being lazy- > >> freed. Therefore, we only reset 'swapbacked' when we are certain the > >> folio is dirty and not droppable. > >> > >> Suggested-by: David Hildenbrand <david@redhat.com> > >> Signed-off-by: Barry Song <v-songbaohua@oppo.com> > >> --- > > > > Acked-by: David Hildenbrand <david@redhat.com> > > Thanks! > > Ah, should this have a Fixes: ? That makes sense to me. > > Because of a speculative reference, we might not reclaim MADV_FREE > folios as we silently mark them as swapbacked again, which sounds fairly > wrong. > I assume the tag should be applied to Mauricio's commit 6c8e2a256915a2 ("mm: fix race between MADV_FREE reclaim and blkdev direct IO read") and also Mauricio is CC'ed. > -- > Cheers, > > David / dhildenb > Thanks Barry
On 2025/1/13 11:38, Barry Song wrote: > From: Barry Song <v-songbaohua@oppo.com> > > The refcount may be temporarily or long-term increased, but this does > not change the fundamental nature of the folio already being lazy- > freed. Therefore, we only reset 'swapbacked' when we are certain the > folio is dirty and not droppable. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Barry Song <v-songbaohua@oppo.com> LGTM. Feel free to add: Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
On Tue, Jan 14, 2025 at 10:56 AM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 2025/1/13 11:38, Barry Song wrote: > > From: Barry Song <v-songbaohua@oppo.com> > > > > The refcount may be temporarily or long-term increased, but this does > > not change the fundamental nature of the folio already being lazy- > > freed. Therefore, we only reset 'swapbacked' when we are certain the > > folio is dirty and not droppable. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > > LGTM. Feel free to add: > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> LGTM! Reviewed-by: Lance Yang <ioworker0@gmail.com> Thanks, Lance
diff --git a/mm/rmap.c b/mm/rmap.c index c6c4d4ea29a7..de6b8c34e98c 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1868,34 +1868,29 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, */ smp_rmb(); - /* - * The only page refs must be one from isolation - * plus the rmap(s) (dropped by discard:). - */ - if (ref_count == 1 + map_count && - (!folio_test_dirty(folio) || - /* - * Unlike MADV_FREE mappings, VM_DROPPABLE - * ones can be dropped even if they've - * been dirtied. - */ - (vma->vm_flags & VM_DROPPABLE))) { - dec_mm_counter(mm, MM_ANONPAGES); - goto discard; - } - - /* - * If the folio was redirtied, it cannot be - * discarded. Remap the page to page table. - */ - set_pte_at(mm, address, pvmw.pte, pteval); - /* - * Unlike MADV_FREE mappings, VM_DROPPABLE ones - * never get swap backed on failure to drop. - */ - if (!(vma->vm_flags & VM_DROPPABLE)) + if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) { + /* + * redirtied either using the page table or a previously + * obtained GUP reference. + */ + set_pte_at(mm, address, pvmw.pte, pteval); folio_set_swapbacked(folio); - goto walk_abort; + goto walk_abort; + } else if (ref_count != 1 + map_count) { + /* + * Additional reference. Could be a GUP reference or any + * speculative reference. GUP users must mark the folio + * dirty if there was a modification. This folio cannot be + * reclaimed right now either way, so act just like nothing + * happened. + * We'll come back here later and detect if the folio was + * dirtied when the additional reference is gone. + */ + set_pte_at(mm, address, pvmw.pte, pteval); + goto walk_abort; + } + dec_mm_counter(mm, MM_ANONPAGES); + goto discard; } if (swap_duplicate(entry) < 0) {