On Sun 29-01-17 20:38:49, Kirill A. Shutemov wrote: > For PTE-mapped THP page_check_address_transhuge() is not adequate: it > cannot find all relevant PTEs, only the first one. It means we can miss > some references of the page and it can result in suboptimal decisions by > vmscan. > > Let's switch it to page_vma_mapped_walk(). > > I don't think it's subject for stable@: it's not fatal. The only side > effect is that THP can be swapped out when it shouldn't.
Please be more specific about the situation when this happens and how a user can recognize this is going on. In other words when should I consider backporting this series. Also the interface is quite awkward imho. Why cannot we provide a callback into page_vma_mapped_walk and call it for each pte/pmd that matters to the given page? Wouldn't that be much easier than the loop around page_vma_mapped_walk iterator? > Signed-off-by: Kirill A. Shutemov <[email protected]> > --- > mm/rmap.c | 66 > ++++++++++++++++++++++++++++++++------------------------------- > 1 file changed, 34 insertions(+), 32 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 91619fd70939..0dff8accd629 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -886,45 +886,48 @@ struct page_referenced_arg { > static int page_referenced_one(struct page *page, struct vm_area_struct *vma, > unsigned long address, void *arg) > { > - struct mm_struct *mm = vma->vm_mm; > struct page_referenced_arg *pra = arg; > - pmd_t *pmd; > - pte_t *pte; > - spinlock_t *ptl; > + struct page_vma_mapped_walk pvmw = { > + .page = page, > + .vma = vma, > + .address = address, > + }; > int referenced = 0; > > - if (!page_check_address_transhuge(page, mm, address, &pmd, &pte, &ptl)) > - return SWAP_AGAIN; > + while (page_vma_mapped_walk(&pvmw)) { > + address = pvmw.address; > > - if (vma->vm_flags & VM_LOCKED) { > - if (pte) > - pte_unmap(pte); > - spin_unlock(ptl); > - pra->vm_flags |= VM_LOCKED; > - return SWAP_FAIL; /* To break the loop */ > - } > + if (vma->vm_flags & VM_LOCKED) { > + page_vma_mapped_walk_done(&pvmw); > + pra->vm_flags |= VM_LOCKED; > + return SWAP_FAIL; /* To break the loop */ > + } > > - if (pte) { > - if (ptep_clear_flush_young_notify(vma, address, pte)) { > - /* > - * Don't treat a reference through a sequentially read > - * mapping as such. If the page has been used in > - * another mapping, we will catch it; if this other > - * mapping is already gone, the unmap path will have > - * set PG_referenced or activated the page. > - */ > - if (likely(!(vma->vm_flags & VM_SEQ_READ))) > + if (pvmw.pte) { > + if (ptep_clear_flush_young_notify(vma, address, > + pvmw.pte)) { > + /* > + * Don't treat a reference through > + * a sequentially read mapping as such. > + * If the page has been used in another mapping, > + * we will catch it; if this other mapping is > + * already gone, the unmap path will have set > + * PG_referenced or activated the page. > + */ > + if (likely(!(vma->vm_flags & VM_SEQ_READ))) > + referenced++; > + } > + } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) { > + if (pmdp_clear_flush_young_notify(vma, address, > + pvmw.pmd)) > referenced++; > + } else { > + /* unexpected pmd-mapped page? */ > + WARN_ON_ONCE(1); > } > - pte_unmap(pte); > - } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) { > - if (pmdp_clear_flush_young_notify(vma, address, pmd)) > - referenced++; > - } else { > - /* unexpected pmd-mapped page? */ > - WARN_ON_ONCE(1); > + > + pra->mapcount--; > } > - spin_unlock(ptl); > > if (referenced) > clear_page_idle(page); > @@ -936,7 +939,6 @@ static int page_referenced_one(struct page *page, struct > vm_area_struct *vma, > pra->vm_flags |= vma->vm_flags; > } > > - pra->mapcount--; > if (!pra->mapcount) > return SWAP_SUCCESS; /* To break the loop */ > > -- > 2.11.0 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to [email protected]. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"[email protected]";> [email protected] </a> -- Michal Hocko SUSE Labs
