Re: [PATCH v6] hugetlb: don't delete vma_lock in hugetlb MADV_DONTNEED processing

From: Mike Kravetz
Date: Mon Oct 31 2022 - 21:51:16 EST


On 10/31/22 17:54, Nadav Amit wrote:
> On Oct 31, 2022, at 3:34 PM, Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> > madvise(MADV_DONTNEED) ends up calling zap_page_range() to clear the page
> > tables associated with the address range. For hugetlb vmas,
> > zap_page_range will call __unmap_hugepage_range_final. However,
> > __unmap_hugepage_range_final assumes the passed vma is about to be removed
> > and deletes the vma_lock to prevent pmd sharing as the vma is on the way
> > out. In the case of madvise(MADV_DONTNEED) the vma remains, but the
> > missing vma_lock prevents pmd sharing and could potentially lead to issues
> > with truncation/fault races.
> >
>
> [snip]
>
> > index 978c17df053e..517c8cc8ccb9 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -3464,4 +3464,7 @@ madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > */
> > #define ZAP_FLAG_DROP_MARKER ((__force zap_flags_t) BIT(0))
> >
> > +/* Set in unmap_vmas() to indicate an unmap call. Only used by hugetlb */
> > +#define ZAP_FLAG_UNMAP ((__force zap_flags_t) BIT(1))
>
> PeterZ wants to add ZAP_FLAG_FORCE_FLUSH that would be set on
> zap_pte_range(). Not sure you would want to combine them both together, but
> at least be aware of potential conflict.
>
> https://lore.kernel.org/all/Y1f7YvKuwOl1XEwU@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>

Ok, noted

> [snip]
>
> > +#ifdef CONFIG_ADVISE_SYSCALLS
> > +/*
> > + * Similar setup as in zap_page_range(). madvise(MADV_DONTNEED) can not call
> > + * zap_page_range for hugetlb vmas as __unmap_hugepage_range_final will delete
> > + * the associated vma_lock.
> > + */
> > +void clear_hugetlb_page_range(struct vm_area_struct *vma, unsigned long start,
> > + unsigned long end)
> > +{
> > + struct mmu_notifier_range range;
> > + struct mmu_gather tlb;
> > +
> > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
> > + start, end);
> > + adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
> > + tlb_gather_mmu(&tlb, vma->vm_mm);
> > + update_hiwater_rss(vma->vm_mm);
> > + mmu_notifier_invalidate_range_start(&range);
> > +
> > + __unmap_hugepage_range_locking(&tlb, vma, start, end, NULL, 0);
> > +
> > + mmu_notifier_invalidate_range_end(&range);
> > + tlb_finish_mmu(&tlb);
> > }
> > +#endif
>
> I hate ifdef’s. And the second definition of clear_hugetlb_page_range() is
> confusing since it does not have an ifdef at all. . How about moving the
> ifdef’s into the function like being done in io_madvise_prep()? I think it
> is less confusing.

Dang!!! I sent the wrong version of the patch. clear_hugetlb_page_range
is not even used here. I used Peter's idea to eliminate the need for
this separate routine. And, this was a development version.

Really sorry about that.

Andrew, if you can please drop the version of the patch.
--
Mike Kravetz