On 06/17/2014 05:01 PM, Naoya Horiguchi wrote:
> On Tue, Jun 17, 2014 at 04:27:56PM +0200, Jerome Marchand wrote:
>> On 06/12/2014 11:48 PM, Naoya Horiguchi wrote:
>>> Now all of current users of page table walker are canonicalized, i.e.
>>> pmd_entry() handles only trans_pmd entry, and pte_entry() handles pte entry.
>>> So we can factorize common code more.
>>> This patch moves pmd_trans_huge_lock() in each pmd_entry() to pagewalk core.
>>>
>>> ChangeLog v2:
>>> - add null check walk->vma in walk_pmd_range()
>>
>> An older version of this patch already made it to linux-next (commit
>> b0e08c5) and I've actually hit the NULL pointer dereference.
>>
>> Moreover, that patch (or maybe another recent pagewalk patch) breaks
>> /proc/<pid>/smaps. All fields that should have been filled by
>> smaps_pte() are almost always zero (and when it isn't, it's always a
>> multiple of 2MB). It seems to me that the page walk never goes below
>> pmd level.
>
> Agreed, I'm now thinking that forcing pte_entry() for every user is not
> good idea, so I'll return to the start point and just will do only the
> necessary changes (i.e. only iron out the vma handling problem for hugepage.)
>
> Thanks,
> Naoya Horiguchi
>
>> Jerome
>>
>>> - move comment update into a separate patch
>>>
>>> Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
>>> ---
>>> diff --git mmotm-2014-05-21-16-57.orig/mm/pagewalk.c mmotm-2014-05-21-16-57/mm/pagewalk.c
>>> index 24311d6f5c20..f1a3417d0b51 100644
>>> --- mmotm-2014-05-21-16-57.orig/mm/pagewalk.c
>>> +++ mmotm-2014-05-21-16-57/mm/pagewalk.c
>>> @@ -73,8 +73,22 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr,
>>> continue;
>>> }
>>>
>>> - if (walk->pmd_entry) {
>>> - err = walk->pmd_entry(pmd, addr, next, walk);
>>> + /*
>>> + * We don't take compound_lock() here but no race with splitting
>>> + * thp happens because:
>>> + * - if pmd_trans_huge_lock() returns 1, the relevant thp is
>>> + * not under splitting, which means there's no concurrent
>>> + * thp split,
>>> + * - if another thread runs into split_huge_page() just after
>>> + * we entered this if-block, the thread must wait for page
>>> + * table lock to be unlocked in __split_huge_page_splitting(),
>>> + * where the main part of thp split is not executed yet.
>>> + */
>>> + if (walk->pmd_entry && walk->vma) {
>>> + if (pmd_trans_huge_lock(pmd, walk->vma, &walk->ptl) == 1) {
>>> + err = walk->pmd_entry(pmd, addr, next, walk);
>>> + spin_unlock(walk->ptl);
>>> + }
>>> if (skip_lower_level_walking(walk))
>>> continue;
>>> if (err)
This is the cause of the smaps trouble. This code modifies walk->control
when pmd_entry() is present, even when it is not called. All the control
code should depend on pmd_trans_huge_lock() == 1 too.
Jerome
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>