Re: [PATCH v1 2/5] mm/madvise: introduce MADV_POPULATE_(READ|WRITE) to prefault/prealloc memory

[David Hildenbrand]

On 30.03.21 18:30, David Hildenbrand wrote:
> On 30.03.21 18:21, Jann Horn wrote:
> > On Tue, Mar 30, 2021 at 5:01 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > > > > +long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start,
> > > > > +                           unsigned long end, bool write, int *locked)
> > > > > +{
> > > > > +       struct mm_struct *mm = vma->vm_mm;
> > > > > +       unsigned long nr_pages = (end - start) / PAGE_SIZE;
> > > > > +       int gup_flags;
> > > > > +
> > > > > +       VM_BUG_ON(!PAGE_ALIGNED(start));
> > > > > +       VM_BUG_ON(!PAGE_ALIGNED(end));
> > > > > +       VM_BUG_ON_VMA(start < vma->vm_start, vma);
> > > > > +       VM_BUG_ON_VMA(end > vma->vm_end, vma);
> > > > > +       mmap_assert_locked(mm);
> > > > > +
> > > > > +       /*
> > > > > +        * FOLL_HWPOISON: Return -EHWPOISON instead of -EFAULT when we hit
> > > > > +        *                a poisoned page.
> > > > > +        * FOLL_POPULATE: Always populate memory with VM_LOCKONFAULT.
> > > > > +        * !FOLL_FORCE: Require proper access permissions.
> > > > > +        */
> > > > > +       gup_flags = FOLL_TOUCH | FOLL_POPULATE | FOLL_MLOCK | FOLL_HWPOISON;
> > > > > +       if (write)
> > > > > +               gup_flags |= FOLL_WRITE;
> > > > > +
> > > > > +       /*
> > > > > +        * See check_vma_flags(): Will return -EFAULT on incompatible mappings
> > > > > +        * or with insufficient permissions.
> > > > > +        */
> > > > > +       return __get_user_pages(mm, start, nr_pages, gup_flags,
> > > > > +                               NULL, NULL, locked);
> > > >
> > > > You mentioned in the commit message that you don't want to actually
> > > > dirty all the file pages and force writeback; but doesn't
> > > > POPULATE_WRITE still do exactly that? In follow_page_pte(), if
> > > > FOLL_TOUCH and FOLL_WRITE are set, we mark the page as dirty:
> > >
> > > Well, I mention that POPULATE_READ explicitly doesn't do that. I
> > > primarily set it because populate_vma_page_range() also sets it.
> > >
> > > Is it safe to *not* set it? IOW, fault something writable into a page
> > > table (where the CPU could dirty it without additional page faults)
> > > without marking it accessed? For me, this made logically sense. Thus I
> > > also understood why populate_vma_page_range() set it.
> >
> > FOLL_TOUCH doesn't have anything to do with installing the PTE - it
> > essentially means "the caller of get_user_pages wants to read/write
> > the contents of the returned page, so please do the same things you
> > would do if userspace was accessing the page". So in particular, if
> > you look up a page via get_user_pages() with FOLL_WRITE|FOLL_TOUCH,
> > that tells the MM subsystem "I will be writing into this page directly
> > from the kernel, bypassing the userspace page tables, so please mark
> > it as dirty now so that it will be properly written back later". Part
> > of that is that it marks the page as recently used, which has an
> > effect on LRU pageout behavior, I think - as far as I understand, that
> > is why populate_vma_page_range() uses FOLL_TOUCH.
> >
> > If you look at __get_user_pages(), you can see that it is split up
> > into two major parts: faultin_page() for creating PTEs, and
> > follow_page_mask() for grabbing pages from PTEs. faultin_page()
> > ignores FOLL_TOUCH completely; only follow_page_mask() uses it.
> >
> > In a way I guess maybe you do want the "mark as recently accessed"
> > part that FOLL_TOUCH would give you without FOLL_WRITE? But I think
> > you very much don't want the dirtying that FOLL_TOUCH|FOLL_WRITE leads
> > to. Maybe the ideal approach would be to add a new FOLL flag to say "I
> > only want to mark as recently used, I don't want to dirty". Or maybe
> > it's enough to just leave out the FOLL_TOUCH entirely, I don't know.
>
> Any thoughts why populate_vma_page_range() does it?

Sorry, I missed the explanation above - thanks!


--
Thanks,

David / dhildenb