Re: [v2 PATCH] mm: thp: fix false negative of shmem vma's THP eligibility

From: Vlastimil Babka <vbabka@suse.cz>
To: Yang Shi <yang.shi@linux.alibaba.com>,
	mhocko@suse.com, rientjes@google.com, kirill@shutemov.name,
	akpm@linux-foundation.org
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org__handle_mm_fault
Subject: Re: [v2 PATCH] mm: thp: fix false negative of shmem vma's THP eligibility
Date: Wed, 24 Apr 2019 18:17:43 +0200	[thread overview]
Message-ID: <e9feaee4-4276-e672-c852-b64fd8965838@suse.cz> (raw)
In-Reply-To: <cca0cab8-c1a5-2ea5-0433-964b8166f54a@linux.alibaba.com>

On 4/24/19 5:47 PM, Yang Shi wrote:
> 
> 
> On 4/24/19 6:10 AM, Vlastimil Babka wrote:
>> On 4/23/19 6:43 PM, Yang Shi wrote:
>>> The commit 7635d9cbe832 ("mm, thp, proc: report THP eligibility for each
>>> vma") introduced THPeligible bit for processes' smaps. But, when checking
>>> the eligibility for shmem vma, __transparent_hugepage_enabled() is
>>> called to override the result from shmem_huge_enabled().  It may result
>>> in the anonymous vma's THP flag override shmem's.  For example, running a
>>> simple test which create THP for shmem, but with anonymous THP disabled,
>>> when reading the process's smaps, it may show:
>>>
>>> 7fc92ec00000-7fc92f000000 rw-s 00000000 00:14 27764 /dev/shm/test
>>> Size:               4096 kB
>>> ...
>>> [snip]
>>> ...
>>> ShmemPmdMapped:     4096 kB
>> But how does this happen in the first place?
>> In __handle_mm_fault() we do:
>>
>>          if (pmd_none(*vmf.pmd) && __transparent_hugepage_enabled(vma)) {
>>                  ret = create_huge_pmd(&vmf);
>>                  if (!(ret & VM_FAULT_FALLBACK))
>>                          return ret;
>>
>> And __transparent_hugepage_enabled() checks the global THP settings.
>> If THP is not enabled / is only for madvise and the vma is not madvised,
>> then this should fail, and also khugepaged shouldn't either run at all,
>> or don't do its job for such non-madvised vma.
> 
> If __transparent_hugepage_enabled() returns false, the code will not 
> reach create_huge_pmd() at all. If it returns true, create_huge_pmd() 
> actually will return VM_FAULT_FALLBACK for shmem since shmem doesn't 
> have huge_fault (or pmd_fault in earlier versions) method.
> 
> Then it will get into handle_pte_fault(), finally shmem_fault() is 
> called, which allocates THP by checking some global flag (i.e. 
> VM_NOHUGEPAGE and MMF_DISABLE_THP) and  shmem THP knobs.

Aha, thanks! What a mess...

> 
> 4.8 (the first version has shmem THP merged) behaves exactly in the same 
> way. So, I suspect this may be intended behavior.

Still looks like an oversight to me. And it's inconsistent... it might
fault huge shmem pages when THPs are globally disabled, but khugepaged
is still not running. I think it should just check the global THP flags
as well...