On Wed, Jun 18, 2014 at 05:13:53PM +0200, Jerome Marchand wrote:
> 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.
Thank you for pointing out, I have a few objection about doing aggressive
cleanup around this code, and now I'm preparing the next version which
does minimum cleanup without walk->control stuff, so I hope your concern
will be gone in it.
Thanks,
Naoya Horiguchi
--
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>