On Fri, May 09, 2014 at 03:17:14PM +0900, Minchan Kim wrote:
> Hello Rik,
>
> On Thu, May 08, 2014 at 04:04:33PM -0400, Rik van Riel wrote:
> > On 04/20/2014 09:56 PM, Minchan Kim wrote:
> >
> > >In summary, MADV_FREE is about 2 time faster than MADV_DONTNEED.
> >
> > This is awesome.
>
> Thanks!
>
> >
> > I have a few nitpicks with the patch, though :)
> >
> > >+static long madvise_lazyfree(struct vm_area_struct *vma,
> > >+ struct vm_area_struct **prev,
> > >+ unsigned long start, unsigned long end)
> > >+{
> > >+ *prev = vma;
> > >+ if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
> > >+ return -EINVAL;
> > >+
> > >+ /* MADV_FREE works for only anon vma at the moment */
> > >+ if (vma->vm_file)
> > >+ return -EINVAL;
> > >+
> > >+ lazyfree_range(vma, start, end - start);
> > >+ return 0;
> > >+}
> >
> > This code checks whether lazyfree_range would work on
> > the VMA...
> >
> > >diff --git a/mm/memory.c b/mm/memory.c
> > >index c4b5bc250820..ca427f258204 100644
> > >--- a/mm/memory.c
> > >+++ b/mm/memory.c
> > >@@ -1270,6 +1270,104 @@ static inline unsigned long zap_pud_range(struct
> > >mmu_gather *tlb,
> > > return addr;
> > > }
> > >
> > >+static unsigned long lazyfree_pte_range(struct mmu_gather *tlb,
> > >+ struct vm_area_struct *vma, pmd_t *pmd,
> > >+ unsigned long addr, unsigned long end)
> > >+{
> > >+ struct mm_struct *mm = tlb->mm;
> > >+ spinlock_t *ptl;
> > >+ pte_t *start_pte;
> > >+ pte_t *pte;
> > >+
> > >+ start_pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> > >+ pte = start_pte;
> > >+ arch_enter_lazy_mmu_mode();
> > >+ do {
> > >+ pte_t ptent = *pte;
> > >+
> > >+ if (pte_none(ptent))
> > >+ continue;
> > >+
> > >+ if (!pte_present(ptent))
> > >+ continue;
> > >+
> > >+ ptent = pte_mkold(ptent);
> > >+ ptent = pte_mkclean(ptent);
> > >+ set_pte_at(mm, addr, pte, ptent);
> > >+ tlb_remove_tlb_entry(tlb, pte, addr);
> >
> > This may not work on PPC, which has a weird hash table for
> > its TLB. You will find that tlb_remove_tlb_entry does
> > nothing for PPC64, and set_pte_at does not remove the hash
> > table entry either.
>
> Hmm, I didn't notice that. Thanks Rik.
>
> Maybe I need this in asm-generic.
>
> static inline void ptep_set_lazyfree(struct mm_struct *mm, unsigned addr,
> pte_t *ptep)
> {
> pte_t ptent = *ptep;
> ptent = pte_mkold(ptent);
> ptent = pte_mkclean(ptent);
> set_pte_at(mm, addr, ptep, ptent);
> }
>
> For arch/powerpc/include/asm/pgtable.h
>
> static inline void ptep_set_lazyfree(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep)
> {
> pte_update(mm, addr, ptep, _PAGE_DIRTY|_PAGE_ACCESSED, 0, 0);
> }
>
> >
> > >@@ -1370,6 +1485,31 @@ void unmap_vmas(struct mmu_gather *tlb,
> > > }
> > >
> > > /**
> > >+ * lazyfree_range - clear dirty bit of pte in a given range
> > >+ * @vma: vm_area_struct holding the applicable pages
> > >+ * @start: starting address of pages
> > >+ * @size: number of bytes to do lazyfree
> > >+ *
> > >+ * Caller must protect the VMA list
> > >+ */
> > >+void lazyfree_range(struct vm_area_struct *vma, unsigned long start,
> > >+ unsigned long size)
> > >+{
> > >+ struct mm_struct *mm = vma->vm_mm;
> > >+ struct mmu_gather tlb;
> > >+ unsigned long end = start + size;
> > >+
> > >+ lru_add_drain();
> > >+ tlb_gather_mmu(&tlb, mm, start, end);
> > >+ update_hiwater_rss(mm);
> > >+ mmu_notifier_invalidate_range_start(mm, start, end);
> > >+ for ( ; vma && vma->vm_start < end; vma = vma->vm_next)
> > >+ lazyfree_single_vma(&tlb, vma, start, end);
> > >+ mmu_notifier_invalidate_range_end(mm, start, end);
> > >+ tlb_finish_mmu(&tlb, start, end);
> > >+}
> >
> > This function, called by madvise_lazyfree, can iterate
> > over multiple VMAs.
> >
> > However, madvise_lazyfree only checked one of them.
>
> Oops, the check should have been lazyfree_range.
> Will fix.
Now that I see the code, madvise_vma always pass *a* vma so madvise_lazyfree
doesn't cover multiple vma all at once so the current sematic is same with
dontneed. So, I don't see any problem. If I miss something, let me know it.
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
