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

[David Hildenbrand <david@redhat.com>]

On 30.03.21 18:31, David Hildenbrand wrote:
> 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@redhat.com> 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!

Looking into the details, adjusting the FOLL_TOUCH logic won't make too 
much of a difference for MADV_POPULATE_WRITE I guess. AFAIKs, the 
biggest impact of FOLL_TOUCH is actually with FOLL_FORCE - which we are 
not using, but populate_vma_page_range() is.


If a page was not faulted in yet, 
faultin_page(FOLL_WRITE)->handle_mm_fault(FAULT_FLAG_WRITE) will already 
mark the PTE/PMD/... dirty and accessed. One example is 
handle_pte_fault(). We will mark the page accessed again via FOLL_TOUCH, 
which doesn't seem to be strictly required.


If the page was already faulted in, we have three cases:

1. Page faulted in writable. The page should already be dirty (otherwise 
we would be in trouble I guess). We will mark it accessed.

2. Page faulted in readable. handle_mm_fault() will fault it in writable 
and set the page dirty.

3. Page faulted in readable and we have FOLL_FORCE. We mark the page 
dirty and accessed.


So doing a MADV_POPULATE_WRITE, whereby we prefault page tables 
writable, doesn't seem to fly without marking the pages dirty. That's 
one reason why I included MADV_POPULATE_READ.

We could

a) Drop FOLL_TOUCH. We are not marking the page accessed, which would 
mean it gets evicted rather earlier than later.

b) Introduce FOLL_ACCESSED which won't do the dirtying. But then, the 
pages are already dirty as explained above, so there isn't a real 
observable change.

c) Keep it as is: Mark the page accessed and dirty. As it's already 
dirty, that does not seem to be a real issue.

Am I missing something obvious? Thanks!

-- 
Thanks,

David / dhildenb