Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE

[Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org>]

On 05/24/2017 09:50 AM, Mike Rapoport wrote:
> On Mon, May 22, 2017 at 05:52:47PM +0200, Vlastimil Babka wrote:
>> On 05/22/2017 04:29 PM, Mike Rapoport wrote:
>>> On Mon, May 22, 2017 at 03:55:48PM +0200, Michal Hocko wrote:
>>>> On Mon 22-05-17 16:36:00, Mike Rapoport wrote:
>>>>> On Mon, May 22, 2017 at 02:42:43PM +0300, Kirill A. Shutemov wrote:
>>>>>> On Mon, May 22, 2017 at 09:12:42AM +0300, Mike Rapoport wrote:
>>>>>>> Currently applications can explicitly enable or disable THP for a memory
>>>>>>> region using MADV_HUGEPAGE or MADV_NOHUGEPAGE. However, once either of
>>>>>>> these advises is used, the region will always have
>>>>>>> VM_HUGEPAGE/VM_NOHUGEPAGE flag set in vma->vm_flags.
>>>>>>> The MADV_CLR_HUGEPAGE resets both these flags and allows managing THP in
>>>>>>> the region according to system-wide settings.
>>>>>>
>>>>>> Seems reasonable. But could you describe an use-case when it's useful in
>>>>>> real world.
>>>>>
>>>>> My use-case was combination of pre- and post-copy migration of containers
>>>>> with CRIU.
>>>>> In this case we populate a part of a memory region with data that was saved
>>>>> during the pre-copy stage. Afterwards, the region is registered with
>>>>> userfaultfd and we expect to get page faults for the parts of the region
>>>>> that were not yet populated. However, khugepaged collapses the pages and
>>>>> the page faults we would expect do not occur.
>>>>
>>>> I am not sure I undestand the problem. Do I get it right that the
>>>> khugepaged will effectivelly corrupt the memory by collapsing a range
>>>> which is not yet fully populated? If yes shouldn't that be fixed in
>>>> khugepaged rather than adding yet another madvise command? Also how do
>>>> you prevent on races? (say you VM_NOHUGEPAGE, khugepaged would be in the
>>>> middle of the operation and sees a collapsable vma and you get the same
>>>> result)
>>>
>>> Probably I didn't explained it too well.
>>>
>>> The range is intentionally not populated. When we combine pre- and
>>> post-copy for process migration, we create memory pre-dump without stopping
>>> the process, then we freeze the process without dumping the pages it has
>>> dirtied between pre-dump and freeze, and then, during restore, we populate
>>> the dirtied pages using userfaultfd.
>>>
>>> When CRIU restores a process in such scenario, it does something like:
>>>
>>> * mmap() memory region
>>> * fill in the pages that were collected during the pre-dump
>>> * do some other stuff
>>> * register memory region with userfaultfd
>>> * populate the missing memory on demand
>>>
>>> khugepaged collapses the pages in the partially populated regions before we
>>> have a chance to register these regions with userfaultfd, which would
>>> prevent the collapse.
>>>
>>> We could have used MADV_NOHUGEPAGE right after the mmap() call, and then
>>> there would be no race because there would be nothing for khugepaged to
>>> collapse at that point. But the problem is that we have no way to reset
>>> *HUGEPAGE flags after the memory restore is complete.
>>
>> Hmm, I wouldn't be that sure if this is indeed race-free. Check that
>> this scenario is indeed impossible?
>>
>> - you do the mmap
>> - khugepaged will choose the process' mm to scan
>> - khugepaged will get to the vma in question, it doesn't have
>> MADV_NOHUGEPAGE yet
>> - you set MADV_NOHUGEPAGE on the vma
>> - you start populating the vma
>> - khugepaged sees the vma is non-empty, collapses
>>
>> unless I'm wrong, the racers will have mmap_sem for reading only when
>> setting/checking the MADV_NOHUGEPAGE? Might be actually considered a bug.
>>
>> However, can't you use prctl(PR_SET_THP_DISABLE) instead? "If arg2 has a
>> nonzero value, the flag is set, otherwise it is cleared." says the
>> manpage. Do it before the mmap and you avoid the race as well?
> 
> Unfortunately, prctl(PR_SET_THP_DISABLE) didn't help :(
> When I've tried to use it, I've ended up with VM_NOHUGEPAGE set on all VMAs
> created after prctl(). This returns me to the state when checkpoint-restore
> alters the application vma->vm_flags although it shouldn't and I do not see
> a way to fix it using existing interfaces.

[CC linux-api, should have been done in the initial posting already]

Hm so the prctl does:

                if (arg2)
                        me->mm->def_flags |= VM_NOHUGEPAGE;
                else
                        me->mm->def_flags &= ~VM_NOHUGEPAGE;

That's rather lazy implementation IMHO. Could we change it so the flag
is stored elsewhere in the mm, and the code that decides to (not) use
THP will check both the per-vma flag and the per-mm flag?

> --
> Sincerely yours,
> Mike. 
>