Re: [PATCH] mm: thp: passing correct vm_flags to hugepage_vma_check

[Song Liu <songliubraving@fb.com>]

> On Jun 29, 2018, at 7:25 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Fri, 29 Jun 2018 11:17:52 -0700 Song Liu <songliubraving@fb.com> wrote:
> 
>> Back in May, I sent patch similar to 02b75dc8160d:
>> 
>> https://patchwork.kernel.org/patch/10416233/  (v1)
>> 
>> This patch got positive feedback. However, I realized there is a problem,
>> that vma->vm_flags in khugepaged_enter_vma_merge() is stale. The separate
>> argument vm_flags contains the latest value. Therefore, it is
>> necessary to pass this vm_flags into hugepage_vma_check(). To fix this
>> problem,  I resent v2 and v3 of the work:
>> 
>> https://patchwork.kernel.org/patch/10419527/   (v2)
>> https://patchwork.kernel.org/patch/10433937/   (v3)
>> 
>> To my surprise, after I thought we all agreed on v3 of the work. Yang's
>> patch, which is similar to correct looking (but wrong) v1, got applied.
>> So we still have the issue of stale vma->vm_flags. This patch fixes this
>> issue. Please apply.
> 
> That's a ueful history lesson but most of it isn't relevant to this
> change.  So I'd suggest this changelog:
> 
> : khugepaged_enter_vma_merge() passes a stale vma->vm_flags to
> : hugepage_vma_check().  The argument vm_flags contains the latest value. 
> : Therefore, it is necessary to pass this vm_flags into
> : hugepage_vma_check().

This looks good. Thanks!

> Also, please (as always) tell us the user-visible runtime effects of
> this bug so that others can decide which kernel(s) need the fix?

With this bug, madvise(MADV_HUGEPAGE) for mmap files in shmem fails to
put memory in huge pages. Here is an example of failed madvise():

   /* mount /dev/shm with huge=advise:
    *     mount -o remount,huge=advise /dev/shm */
   /* create file /dev/shm/huge */
   #define HUGE_FILE "/dev/shm/huge"

   fd = open(HUGE_FILE, O_RDONLY);
   ptr = mmap(NULL, FILE_SIZE, PROT_READ, MAP_PRIVATE, fd, 0);
   ret = madvise(ptr, FILE_SIZE, MADV_HUGEPAGE);

madvise() will return 0, but this memory region is never put in huge
page (check from /proc/meminfo: ShmemHugePages).

Thanks,
Song