Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565982 > unrolled thread

[PATCH 00/12] Fix few rmap-related THP bugs

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/12] Fix few rmap-related THP bugs "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:30 +0100
    [PATCH 11/12] mm: drop page_check_address{,_transhuge} "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:30 +0100
    [PATCH 12/12] mm: convert remove_migration_pte() to page_check_walk() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:30 +0100
      Re: [PATCH 12/12] mm: convert remove_migration_pte() to  page_check_walk() kbuild test robot <lkp@intel.com> - 2017-01-25 02:50 +0100
    [PATCH 01/12] uprobes: split THPs before trying replace them "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:40 +0100
      Re: [PATCH 01/12] uprobes: split THPs before trying replace them Rik van Riel <riel@redhat.com> - 2017-01-24 19:10 +0100
      Re: [PATCH 01/12] uprobes: split THPs before trying replace them Andrew Morton <akpm@linux-foundation.org> - 2017-01-24 22:30 +0100
        Re: [PATCH 01/12] uprobes: split THPs before trying replace them "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-24 23:30 +0100
          Re: [PATCH 01/12] uprobes: split THPs before trying replace them Andrew Morton <akpm@linux-foundation.org> - 2017-01-24 23:40 +0100
            Re: [PATCH 01/12] uprobes: split THPs before trying replace them "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-25 00:00 +0100
          Re: [PATCH 01/12] uprobes: split THPs before trying replace them Srikar Dronamraju <srikar@linux.vnet.ibm.com> - 2017-01-25 18:00 +0100
            Re: [PATCH 01/12] uprobes: split THPs before trying replace them Rik van Riel <riel@redhat.com> - 2017-01-25 18:50 +0100
            Re: [PATCH 01/12] uprobes: split THPs before trying replace them "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-25 18:50 +0100
            Re: [PATCH 01/12] uprobes: split THPs before trying replace them Johannes Weiner <hannes@cmpxchg.org> - 2017-01-25 19:40 +0100
              Re: [PATCH 01/12] uprobes: split THPs before trying replace them "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-25 19:40 +0100
              Re: [PATCH 01/12] uprobes: split THPs before trying replace them Srikar Dronamraju <srikar@linux.vnet.ibm.com> - 2017-01-26 04:00 +0100
      Re: [PATCH 01/12] uprobes: split THPs before trying replace them Johannes Weiner <hannes@cmpxchg.org> - 2017-01-25 19:30 +0100
    [PATCH 04/12] mm: fix handling PTE-mapped THPs in page_idle_clear_pte_refs() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:40 +0100
    [PATCH 05/12] mm, rmap: check all VMAs that PTE-mapped THP can be part of "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:40 +0100
    [PATCH 02/12] mm: introduce page_check_walk() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:40 +0100
      Re: [PATCH 02/12] mm: introduce page_check_walk() Andrew Morton <akpm@linux-foundation.org> - 2017-01-24 22:50 +0100
        Re: [PATCH 02/12] mm: introduce page_check_walk() Andrew Morton <akpm@linux-foundation.org> - 2017-01-25 00:00 +0100
          Re: [PATCH 02/12] mm: introduce page_check_walk() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-25 19:00 +0100
        Re: [PATCH 02/12] mm: introduce page_check_walk() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-01-25 00:00 +0100
      Re: [PATCH 02/12] mm: introduce page_check_walk() kbuild test robot <lkp@intel.com> - 2017-01-25 02:30 +0100
      Re: [PATCH 02/12] mm: introduce page_check_walk() kbuild test robot <lkp@intel.com> - 2017-01-25 03:10 +0100
    [PATCH 09/12] mm, uprobes: convert __replace_page() to page_check_walk() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-01-24 17:40 +0100
      Re: [PATCH 09/12] mm, uprobes: convert __replace_page() to  page_check_walk() Srikar Dronamraju <srikar@linux.vnet.ibm.com> - 2017-01-26 04:10 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1566165 — Re: [PATCH 02/12] mm: introduce page_check_walk()

On Tue, 24 Jan 2017 19:28:14 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> The patch introduce new interface to check if a page is mapped into a vma.
> It aims to address shortcomings of page_check_address{,_transhuge}.
> 
> Existing interface is not able to handle PTE-mapped THPs: it only finds
> the first PTE. The rest lefted unnoticed.
> 
> page_check_walk() iterates over all possible mapping of the page in the
> vma.

I really don't like the name page_check_walk().  "check" could mean any
damn thing.  Something like page_vma_mapped_walk() has meaning.  We
could omit the "_walk" for brevity.


> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  include/linux/rmap.h |  65 ++++++++++++++++++++++
>  mm/Makefile          |   6 ++-
>  mm/huge_memory.c     |   9 ++--
>  mm/page_check.c      | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 223 insertions(+), 5 deletions(-)
>  create mode 100644 mm/page_check.c
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 15321fb1df6b..474279810742 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -232,6 +232,71 @@ static inline bool page_check_address_transhuge(struct page *page,
>  }
>  #endif
>  
> +/* Avoid racy checks */
> +#define PAGE_CHECK_WALK_SYNC		(1 << 0)
> +/* Look for migarion entries rather than present ptes */
> +#define PAGE_CHECK_WALK_MIGRATION	(1 << 1)
> +
> +struct page_check_walk {
> +	struct page *page;
> +	struct vm_area_struct *vma;
> +	unsigned long address;
> +	pmd_t *pmd;
> +	pte_t *pte;
> +	spinlock_t *ptl;
> +	unsigned int flags;
> +};

One thing which I don't think was documented is that it is the caller's
responsibility to initialize this appropriately before calling
page_check_walk().  At least, .pte and .ptl must be NULL, for
page_check_walk_done().

> +static inline void page_check_walk_done(struct page_check_walk *pcw)
> +{
> +	if (pcw->pte)
> +		pte_unmap(pcw->pte);
> +	if (pcw->ptl)
> +		spin_unlock(pcw->ptl);
> +}
> +
> +bool __page_check_walk(struct page_check_walk *pcw);
> +
> +/**
> + * page_check_walk - check if @pcw->page is mapped in @pcw->vma at @pcw->address
> + * @pcw: pointer to struce page_check_walk. page, vma and address must be set.

"struct"

> + *
> + * Returns true, if the page is mapped in the vma. @pcw->pmd and @pcw->pte point

"Returns true if"

> + * to relevant page table entries. @pcw->ptl is locked. @pcw->address is
> + * adjusted if needed (for PTE-mapped THPs).
> + *
> + * If @pcw->pmd is set, but @pcw->pte is not, you have found PMD-mapped page

"is set but"

> + * (usually THP). For PTE-mapped THP, you should run page_check_walk() in 
> + * a loop to find all PTEs that maps the THP.

"that map"

> + *
> + * For HugeTLB pages, @pcw->pte is set to relevant page table entry regardless

"set to the relevant", "regardless of"

> + * which page table level the page mapped at. @pcw->pmd is NULL.

"the page is"

> + *
> + * Retruns false, if there's no more page table entries for the page in the vma.

"Returns false if there are"

> + * @pcw->ptl is unlocked and @pcw->pte is unmapped.
> + *
> + * If you need to stop the walk before page_check_walk() returned false, use
> + * page_check_walk_done(). It will do the housekeeping.
> + */
> +static inline bool page_check_walk(struct page_check_walk *pcw)
> +{
> +	/* The only possible pmd mapping has been handled on last iteration */
> +	if (pcw->pmd && !pcw->pte) {
> +		page_check_walk_done(pcw);
> +		return false;
> +	}
> +
> +	/* Only for THP, seek to next pte entry makes sense */
> +	if (pcw->pte) {
> +		if (!PageTransHuge(pcw->page) || PageHuge(pcw->page)) {
> +			page_check_walk_done(pcw);
> +			return false;
> +		}
> +	}
> +
> +	return __page_check_walk(pcw);
> +}

Was the decision to inline this a correct one?

> --- /dev/null
> +++ b/mm/page_check.c
> @@ -0,0 +1,148 @@
> +#include <linux/mm.h>
> +#include <linux/rmap.h>
> +#include <linux/hugetlb.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
> +
> +#include "internal.h"
> +
> +static inline bool check_pmd(struct page_check_walk *pcw)
> +{
> +	pmd_t pmde = *pcw->pmd;
> +	barrier();
> +	return pmd_present(pmde) && !pmd_trans_huge(pmde);
> +}

Can we please have a comment explaining what the barrier() does?

> +static inline bool not_found(struct page_check_walk *pcw)
> +{
> +	page_check_walk_done(pcw);
> +	return false;
> +}
> +
> +static inline bool map_pte(struct page_check_walk *pcw)
> +{
> +	pcw->pte = pte_offset_map(pcw->pmd, pcw->address);
> +	if (!(pcw->flags & PAGE_CHECK_WALK_SYNC)) {
> +		if (pcw->flags & PAGE_CHECK_WALK_MIGRATION) {
> +			if (!is_swap_pte(*pcw->pte))
> +				return false;
> +		} else {
> +			if (!pte_present(*pcw->pte))
> +				return false;
> +		}
> +	}
> +	pcw->ptl = pte_lockptr(pcw->vma->vm_mm, pcw->pmd);
> +	spin_lock(pcw->ptl);
> +	return true;
> +}

The compiler will just ignore all these "inline" statements.

> +static inline bool check_pte(struct page_check_walk *pcw)
> +{
> +	if (pcw->flags & PAGE_CHECK_WALK_MIGRATION) {
> +		swp_entry_t entry;
> +		if (!is_swap_pte(*pcw->pte))
> +			return false;
> +		entry = pte_to_swp_entry(*pcw->pte);
> +		if (!is_migration_entry(entry))
> +			return false;
> +		if (migration_entry_to_page(entry) - pcw->page >=
> +				hpage_nr_pages(pcw->page)) {
> +			return false;
> +		}
> +		if (migration_entry_to_page(entry) < pcw->page)
> +			return false;
> +	} else {
> +		if (!pte_present(*pcw->pte))
> +			return false;
> +
> +		/* THP can be referenced by any subpage */
> +		if (pte_page(*pcw->pte) - pcw->page >=
> +				hpage_nr_pages(pcw->page)) {
> +			return false;
> +		}
> +		if (pte_page(*pcw->pte) < pcw->page)
> +			return false;
> +	}
> +
> +	return true;
> +}

Thankfully, because inlining this one does seem inappropriate - it's
enormous!

> +bool __page_check_walk(struct page_check_walk *pcw)
> +{
> +	struct mm_struct *mm = pcw->vma->vm_mm;
> +	struct page *page = pcw->page;
> +	pgd_t *pgd;
> +	pud_t *pud;
> +
> +	/* For THP, seek to next pte entry */
> +	if (pcw->pte)
> +		goto next_pte;
> +
> +	if (unlikely(PageHuge(pcw->page))) {
> +		/* when pud is not present, pte will be NULL */
> +		pcw->pte = huge_pte_offset(mm, pcw->address);
> +		if (!pcw->pte)
> +			return false;
> +
> +		pcw->ptl = huge_pte_lockptr(page_hstate(page), mm, pcw->pte);
> +		spin_lock(pcw->ptl);
> +		if (!check_pte(pcw))
> +			return not_found(pcw);
> +		return true;
> +	}
> +restart:
> +	pgd = pgd_offset(mm, pcw->address);
> +	if (!pgd_present(*pgd))
> +		return false;
> +	pud = pud_offset(pgd, pcw->address);
> +	if (!pud_present(*pud))
> +		return false;
> +	pcw->pmd = pmd_offset(pud, pcw->address);
> +	if (pmd_trans_huge(*pcw->pmd)) {
> +		pcw->ptl = pmd_lock(mm, pcw->pmd);
> +		if (!pmd_present(*pcw->pmd))
> +			return not_found(pcw);
> +		if (likely(pmd_trans_huge(*pcw->pmd))) {
> +			if (pcw->flags & PAGE_CHECK_WALK_MIGRATION)
> +				return not_found(pcw);
> +			if (pmd_page(*pcw->pmd) != page)
> +				return not_found(pcw);
> +			return true;
> +		} else {
> +			/* THP pmd was split under us: handle on pte level */
> +			spin_unlock(pcw->ptl);
> +			pcw->ptl = NULL;
> +		}
> +	} else {
> +		if (!check_pmd(pcw))
> +			return false;
> +	}
> +	if (!map_pte(pcw))
> +		goto next_pte;
> +	while (1) {
> +		if (check_pte(pcw))
> +			return true;
> +next_pte:	do {
> +			pcw->address += PAGE_SIZE;
> +			if (pcw->address >= __vma_address(pcw->page, pcw->vma) +
> +					hpage_nr_pages(pcw->page) * PAGE_SIZE)
> +				return not_found(pcw);
> +			/* Did we cross page table boundary? */
> +			if (pcw->address % PMD_SIZE == 0) {
> +				pte_unmap(pcw->pte);
> +				if (pcw->ptl) {
> +					spin_unlock(pcw->ptl);
> +					pcw->ptl = NULL;
> +				}
> +				goto restart;
> +			} else {
> +				pcw->pte++;
> +			}
> +		} while (pte_none(*pcw->pte));
> +
> +		if (!pcw->ptl) {
> +			pcw->ptl = pte_lockptr(mm, pcw->pmd);
> +			spin_lock(pcw->ptl);
> +		}
> +	}
> +}

[toc] | [prev] | [next] | [standalone]

#1566218 — Re: [PATCH 02/12] mm: introduce page_check_walk()

On Wed, 25 Jan 2017 01:50:30 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> > > + * @pcw->ptl is unlocked and @pcw->pte is unmapped.
> > > + *
> > > + * If you need to stop the walk before page_check_walk() returned false, use
> > > + * page_check_walk_done(). It will do the housekeeping.
> > > + */
> > > +static inline bool page_check_walk(struct page_check_walk *pcw)
> > > +{
> > > +	/* The only possible pmd mapping has been handled on last iteration */
> > > +	if (pcw->pmd && !pcw->pte) {
> > > +		page_check_walk_done(pcw);
> > > +		return false;
> > > +	}
> > > +
> > > +	/* Only for THP, seek to next pte entry makes sense */
> > > +	if (pcw->pte) {
> > > +		if (!PageTransHuge(pcw->page) || PageHuge(pcw->page)) {
> > > +			page_check_walk_done(pcw);
> > > +			return false;
> > > +		}
> > > +	}
> > > +
> > > +	return __page_check_walk(pcw);
> > > +}
> > 
> > Was the decision to inline this a correct one?
> 
> Well, my logic was that in most cases we would have exactly one iteration.
> The only case when we need more than one iteration is PTE-mapped THP which
> is rare.
> I hoped to avoid additional function call. Not sure if it worth it.
> 
> Should I move it inside the function?

I suggest building a kernel with it uninlined, take a look at the bloat
factor then make a seat-of-the pants decision about "is it worth it". 
With quite a few callsites the saving from uninlining may be
significant.

[toc] | [prev] | [next] | [standalone]

#1566797 — Re: [PATCH 02/12] mm: introduce page_check_walk()

On Tue, Jan 24, 2017 at 02:55:13PM -0800, Andrew Morton wrote:
> On Wed, 25 Jan 2017 01:50:30 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > > > + * @pcw->ptl is unlocked and @pcw->pte is unmapped.
> > > > + *
> > > > + * If you need to stop the walk before page_check_walk() returned false, use
> > > > + * page_check_walk_done(). It will do the housekeeping.
> > > > + */
> > > > +static inline bool page_check_walk(struct page_check_walk *pcw)
> > > > +{
> > > > +	/* The only possible pmd mapping has been handled on last iteration */
> > > > +	if (pcw->pmd && !pcw->pte) {
> > > > +		page_check_walk_done(pcw);
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	/* Only for THP, seek to next pte entry makes sense */
> > > > +	if (pcw->pte) {
> > > > +		if (!PageTransHuge(pcw->page) || PageHuge(pcw->page)) {
> > > > +			page_check_walk_done(pcw);
> > > > +			return false;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return __page_check_walk(pcw);
> > > > +}
> > > 
> > > Was the decision to inline this a correct one?
> > 
> > Well, my logic was that in most cases we would have exactly one iteration.
> > The only case when we need more than one iteration is PTE-mapped THP which
> > is rare.
> > I hoped to avoid additional function call. Not sure if it worth it.
> > 
> > Should I move it inside the function?
> 
> I suggest building a kernel with it uninlined, take a look at the bloat
> factor then make a seat-of-the pants decision about "is it worth it". 
> With quite a few callsites the saving from uninlining may be
> significant.

add/remove: 1/2 grow/shrink: 8/0 up/down: 5089/-2954 (2135)
function                                     old     new   delta
__page_vma_mapped_walk                         -    2928   +2928
try_to_unmap_one                            2916    3218    +302
page_mkclean_one                             513     802    +289
__replace_page                              1439    1719    +280
page_referenced_one                          753    1030    +277
page_mapped_in_vma                           799    1059    +260
remove_migration_pte                        1129    1388    +259
page_idle_clear_pte_refs_one                 197     456    +259
write_protect_page                          1210    1445    +235
page_idle_clear_pte_refs_one.part             26       -     -26
page_vma_mapped_walk                        2928       -   -2928
Total: Before=37784555, After=37786690, chg +0.01%

I'll drop inlining. It would save ~2k.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1566219 — Re: [PATCH 02/12] mm: introduce page_check_walk()

On Tue, Jan 24, 2017 at 01:41:22PM -0800, Andrew Morton wrote:
> On Tue, 24 Jan 2017 19:28:14 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > The patch introduce new interface to check if a page is mapped into a vma.
> > It aims to address shortcomings of page_check_address{,_transhuge}.
> > 
> > Existing interface is not able to handle PTE-mapped THPs: it only finds
> > the first PTE. The rest lefted unnoticed.
> > 
> > page_check_walk() iterates over all possible mapping of the page in the
> > vma.
> 
> I really don't like the name page_check_walk().  "check" could mean any
> damn thing.  Something like page_vma_mapped_walk() has meaning.  We
> could omit the "_walk" for brevity.

page_vma_mapped() would sound as predicate.
I'll rename to page_vma_mapped_walk().

> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  include/linux/rmap.h |  65 ++++++++++++++++++++++
> >  mm/Makefile          |   6 ++-
> >  mm/huge_memory.c     |   9 ++--
> >  mm/page_check.c      | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 223 insertions(+), 5 deletions(-)
> >  create mode 100644 mm/page_check.c
> > 
> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index 15321fb1df6b..474279810742 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -232,6 +232,71 @@ static inline bool page_check_address_transhuge(struct page *page,
> >  }
> >  #endif
> >  
> > +/* Avoid racy checks */
> > +#define PAGE_CHECK_WALK_SYNC		(1 << 0)
> > +/* Look for migarion entries rather than present ptes */
> > +#define PAGE_CHECK_WALK_MIGRATION	(1 << 1)
> > +
> > +struct page_check_walk {
> > +	struct page *page;
> > +	struct vm_area_struct *vma;
> > +	unsigned long address;
> > +	pmd_t *pmd;
> > +	pte_t *pte;
> > +	spinlock_t *ptl;
> > +	unsigned int flags;
> > +};
> 
> One thing which I don't think was documented is that it is the caller's
> responsibility to initialize this appropriately before calling
> page_check_walk().  At least, .pte and .ptl must be NULL, for
> page_check_walk_done().

Okay, I'll write it down.

> > +static inline void page_check_walk_done(struct page_check_walk *pcw)
> > +{
> > +	if (pcw->pte)
> > +		pte_unmap(pcw->pte);
> > +	if (pcw->ptl)
> > +		spin_unlock(pcw->ptl);
> > +}
> > +
> > +bool __page_check_walk(struct page_check_walk *pcw);
> > +
> > +/**
> > + * page_check_walk - check if @pcw->page is mapped in @pcw->vma at @pcw->address
> > + * @pcw: pointer to struce page_check_walk. page, vma and address must be set.
> 
> "struct"
> 
> > + *
> > + * Returns true, if the page is mapped in the vma. @pcw->pmd and @pcw->pte point
> 
> "Returns true if"
> 
> > + * to relevant page table entries. @pcw->ptl is locked. @pcw->address is
> > + * adjusted if needed (for PTE-mapped THPs).
> > + *
> > + * If @pcw->pmd is set, but @pcw->pte is not, you have found PMD-mapped page
> 
> "is set but"
> 
> > + * (usually THP). For PTE-mapped THP, you should run page_check_walk() in 
> > + * a loop to find all PTEs that maps the THP.
> 
> "that map"
> 
> > + *
> > + * For HugeTLB pages, @pcw->pte is set to relevant page table entry regardless
> 
> "set to the relevant", "regardless of"
> 
> > + * which page table level the page mapped at. @pcw->pmd is NULL.
> 
> "the page is"
> 
> > + *
> > + * Retruns false, if there's no more page table entries for the page in the vma.
> 
> "Returns false if there are"

Thanks for corrections. Applied.

> > + * @pcw->ptl is unlocked and @pcw->pte is unmapped.
> > + *
> > + * If you need to stop the walk before page_check_walk() returned false, use
> > + * page_check_walk_done(). It will do the housekeeping.
> > + */
> > +static inline bool page_check_walk(struct page_check_walk *pcw)
> > +{
> > +	/* The only possible pmd mapping has been handled on last iteration */
> > +	if (pcw->pmd && !pcw->pte) {
> > +		page_check_walk_done(pcw);
> > +		return false;
> > +	}
> > +
> > +	/* Only for THP, seek to next pte entry makes sense */
> > +	if (pcw->pte) {
> > +		if (!PageTransHuge(pcw->page) || PageHuge(pcw->page)) {
> > +			page_check_walk_done(pcw);
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return __page_check_walk(pcw);
> > +}
> 
> Was the decision to inline this a correct one?

Well, my logic was that in most cases we would have exactly one iteration.
The only case when we need more than one iteration is PTE-mapped THP which
is rare.
I hoped to avoid additional function call. Not sure if it worth it.

Should I move it inside the function?

> > --- /dev/null
> > +++ b/mm/page_check.c
> > @@ -0,0 +1,148 @@
> > +#include <linux/mm.h>
> > +#include <linux/rmap.h>
> > +#include <linux/hugetlb.h>
> > +#include <linux/swap.h>
> > +#include <linux/swapops.h>
> > +
> > +#include "internal.h"
> > +
> > +static inline bool check_pmd(struct page_check_walk *pcw)
> > +{
> > +	pmd_t pmde = *pcw->pmd;
> > +	barrier();
> > +	return pmd_present(pmde) && !pmd_trans_huge(pmde);
> > +}
> 
> Can we please have a comment explaining what the barrier() does?

I copied it from page_check_address_transhuge().

I think we can get aways with READ_ONCE() instead.
I'll add a comment.

> > +static inline bool not_found(struct page_check_walk *pcw)
> > +{
> > +	page_check_walk_done(pcw);
> > +	return false;
> > +}
> > +
> > +static inline bool map_pte(struct page_check_walk *pcw)
> > +{
> > +	pcw->pte = pte_offset_map(pcw->pmd, pcw->address);
> > +	if (!(pcw->flags & PAGE_CHECK_WALK_SYNC)) {
> > +		if (pcw->flags & PAGE_CHECK_WALK_MIGRATION) {
> > +			if (!is_swap_pte(*pcw->pte))
> > +				return false;
> > +		} else {
> > +			if (!pte_present(*pcw->pte))
> > +				return false;
> > +		}
> > +	}
> > +	pcw->ptl = pte_lockptr(pcw->vma->vm_mm, pcw->pmd);
> > +	spin_lock(pcw->ptl);
> > +	return true;
> > +}
> 
> The compiler will just ignore all these "inline" statements.

These helpers grew during development. I forgot to drop "inline".

> > +static inline bool check_pte(struct page_check_walk *pcw)
> > +{
> > +	if (pcw->flags & PAGE_CHECK_WALK_MIGRATION) {
> > +		swp_entry_t entry;
> > +		if (!is_swap_pte(*pcw->pte))
> > +			return false;
> > +		entry = pte_to_swp_entry(*pcw->pte);
> > +		if (!is_migration_entry(entry))
> > +			return false;
> > +		if (migration_entry_to_page(entry) - pcw->page >=
> > +				hpage_nr_pages(pcw->page)) {
> > +			return false;
> > +		}
> > +		if (migration_entry_to_page(entry) < pcw->page)
> > +			return false;
> > +	} else {
> > +		if (!pte_present(*pcw->pte))
> > +			return false;
> > +
> > +		/* THP can be referenced by any subpage */
> > +		if (pte_page(*pcw->pte) - pcw->page >=
> > +				hpage_nr_pages(pcw->page)) {
> > +			return false;
> > +		}
> > +		if (pte_page(*pcw->pte) < pcw->page)
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> 
> Thankfully, because inlining this one does seem inappropriate - it's
> enormous!
> 
> > +bool __page_check_walk(struct page_check_walk *pcw)
> > +{
> > +	struct mm_struct *mm = pcw->vma->vm_mm;
> > +	struct page *page = pcw->page;
> > +	pgd_t *pgd;
> > +	pud_t *pud;
> > +
> > +	/* For THP, seek to next pte entry */
> > +	if (pcw->pte)
> > +		goto next_pte;
> > +
> > +	if (unlikely(PageHuge(pcw->page))) {
> > +		/* when pud is not present, pte will be NULL */
> > +		pcw->pte = huge_pte_offset(mm, pcw->address);
> > +		if (!pcw->pte)
> > +			return false;
> > +
> > +		pcw->ptl = huge_pte_lockptr(page_hstate(page), mm, pcw->pte);
> > +		spin_lock(pcw->ptl);
> > +		if (!check_pte(pcw))
> > +			return not_found(pcw);
> > +		return true;
> > +	}
> > +restart:
> > +	pgd = pgd_offset(mm, pcw->address);
> > +	if (!pgd_present(*pgd))
> > +		return false;
> > +	pud = pud_offset(pgd, pcw->address);
> > +	if (!pud_present(*pud))
> > +		return false;
> > +	pcw->pmd = pmd_offset(pud, pcw->address);
> > +	if (pmd_trans_huge(*pcw->pmd)) {
> > +		pcw->ptl = pmd_lock(mm, pcw->pmd);
> > +		if (!pmd_present(*pcw->pmd))
> > +			return not_found(pcw);
> > +		if (likely(pmd_trans_huge(*pcw->pmd))) {
> > +			if (pcw->flags & PAGE_CHECK_WALK_MIGRATION)
> > +				return not_found(pcw);
> > +			if (pmd_page(*pcw->pmd) != page)
> > +				return not_found(pcw);
> > +			return true;
> > +		} else {
> > +			/* THP pmd was split under us: handle on pte level */
> > +			spin_unlock(pcw->ptl);
> > +			pcw->ptl = NULL;
> > +		}
> > +	} else {
> > +		if (!check_pmd(pcw))
> > +			return false;
> > +	}
> > +	if (!map_pte(pcw))
> > +		goto next_pte;
> > +	while (1) {
> > +		if (check_pte(pcw))
> > +			return true;
> > +next_pte:	do {
> > +			pcw->address += PAGE_SIZE;
> > +			if (pcw->address >= __vma_address(pcw->page, pcw->vma) +
> > +					hpage_nr_pages(pcw->page) * PAGE_SIZE)
> > +				return not_found(pcw);
> > +			/* Did we cross page table boundary? */
> > +			if (pcw->address % PMD_SIZE == 0) {
> > +				pte_unmap(pcw->pte);
> > +				if (pcw->ptl) {
> > +					spin_unlock(pcw->ptl);
> > +					pcw->ptl = NULL;
> > +				}
> > +				goto restart;
> > +			} else {
> > +				pcw->pte++;
> > +			}
> > +		} while (pte_none(*pcw->pte));
> > +
> > +		if (!pcw->ptl) {
> > +			pcw->ptl = pte_lockptr(mm, pcw->pmd);
> > +			spin_lock(pcw->ptl);
> > +		}
> > +	}
> > +}
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1566266 — Re: [PATCH 02/12] mm: introduce page_check_walk()

[Multipart message — attachments visible in raw view] — view raw

Hi Kirill,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.10-rc5 next-20170124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-A-Shutemov/Fix-few-rmap-related-THP-bugs/20170125-081918
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: i386-defconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/pgtable.h:471:0,
                    from include/linux/mm.h:68,
                    from include/linux/ring_buffer.h:5,
                    from include/linux/trace_events.h:5,
                    from include/trace/syscall.h:6,
                    from include/linux/syscalls.h:81,
                    from init/main.c:18:
   include/linux/rmap.h: In function 'page_check_walk_done':
>> arch/x86/include/asm/pgtable_32.h:53:24: error: implicit declaration of function 'kunmap_atomic' [-Werror=implicit-function-declaration]
    #define pte_unmap(pte) kunmap_atomic((pte))
                           ^
>> include/linux/rmap.h:253:3: note: in expansion of macro 'pte_unmap'
      pte_unmap(pcw->pte);
      ^~~~~~~~~
   cc1: some warnings being treated as errors

vim +/pte_unmap +253 include/linux/rmap.h

   237	/* Look for migarion entries rather than present ptes */
   238	#define PAGE_CHECK_WALK_MIGRATION	(1 << 1)
   239	
   240	struct page_check_walk {
   241		struct page *page;
   242		struct vm_area_struct *vma;
   243		unsigned long address;
   244		pmd_t *pmd;
   245		pte_t *pte;
   246		spinlock_t *ptl;
   247		unsigned int flags;
   248	};
   249	
   250	static inline void page_check_walk_done(struct page_check_walk *pcw)
   251	{
   252		if (pcw->pte)
 > 253			pte_unmap(pcw->pte);
   254		if (pcw->ptl)
   255			spin_unlock(pcw->ptl);
   256	}
   257	
   258	bool __page_check_walk(struct page_check_walk *pcw);
   259	
   260	/**
   261	 * page_check_walk - check if @pcw->page is mapped in @pcw->vma at @pcw->address

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1566277 — Re: [PATCH 02/12] mm: introduce page_check_walk()

[Multipart message — attachments visible in raw view] — view raw

Hi Kirill,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.10-rc5 next-20170124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-A-Shutemov/Fix-few-rmap-related-THP-bugs/20170125-081918
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: sparc64-allnoconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   mm/page_check.c: In function 'check_pte':
>> mm/page_check.c:48:38: error: invalid operands to binary - (have 'void *' and 'struct page *')
      if (migration_entry_to_page(entry) - pcw->page >=
                                         ^ ~~~~~~~~~
>> mm/page_check.c:52:38: warning: comparison of distinct pointer types lacks a cast
      if (migration_entry_to_page(entry) < pcw->page)
                                         ^

vim +48 mm/page_check.c

    42			swp_entry_t entry;
    43			if (!is_swap_pte(*pcw->pte))
    44				return false;
    45			entry = pte_to_swp_entry(*pcw->pte);
    46			if (!is_migration_entry(entry))
    47				return false;
  > 48			if (migration_entry_to_page(entry) - pcw->page >=
    49					hpage_nr_pages(pcw->page)) {
    50				return false;
    51			}
  > 52			if (migration_entry_to_page(entry) < pcw->page)
    53				return false;
    54		} else {
    55			if (!pte_present(*pcw->pte))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1565993 — [PATCH 09/12] mm, uprobes: convert __replace_page() to page_check_walk()

For consistency, it worth converting all page_check_address() to
page_check_walk(), so we could drop the former.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 kernel/events/uprobes.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 1e65c79e52a6..6dbaa93b22fa 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -153,14 +153,19 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 				struct page *old_page, struct page *new_page)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	spinlock_t *ptl;
-	pte_t *ptep;
+	struct page_check_walk pcw = {
+		.page = old_page,
+		.vma = vma,
+		.address = addr,
+	};
 	int err;
 	/* For mmu_notifiers */
 	const unsigned long mmun_start = addr;
 	const unsigned long mmun_end   = addr + PAGE_SIZE;
 	struct mem_cgroup *memcg;
 
+	VM_BUG_ON_PAGE(PageTransHuge(old_page), old_page);
+
 	err = mem_cgroup_try_charge(new_page, vma->vm_mm, GFP_KERNEL, &memcg,
 			false);
 	if (err)
@@ -171,11 +176,11 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 
 	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end);
 	err = -EAGAIN;
-	ptep = page_check_address(old_page, mm, addr, &ptl, 0);
-	if (!ptep) {
+	if (!page_check_walk(&pcw)) {
 		mem_cgroup_cancel_charge(new_page, memcg, false);
 		goto unlock;
 	}
+	VM_BUG_ON_PAGE(addr != pcw.address, old_page);
 
 	get_page(new_page);
 	page_add_new_anon_rmap(new_page, vma, addr, false);
@@ -187,14 +192,15 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 		inc_mm_counter(mm, MM_ANONPAGES);
 	}
 
-	flush_cache_page(vma, addr, pte_pfn(*ptep));
-	ptep_clear_flush_notify(vma, addr, ptep);
-	set_pte_at_notify(mm, addr, ptep, mk_pte(new_page, vma->vm_page_prot));
+	flush_cache_page(vma, addr, pte_pfn(*pcw.pte));
+	ptep_clear_flush_notify(vma, addr, pcw.pte);
+	set_pte_at_notify(mm, addr, pcw.pte,
+			mk_pte(new_page, vma->vm_page_prot));
 
 	page_remove_rmap(old_page, false);
 	if (!page_mapped(old_page))
 		try_to_free_swap(old_page);
-	pte_unmap_unlock(ptep, ptl);
+	page_check_walk_done(&pcw);
 
 	if (vma->vm_flags & VM_LOCKED)
 		munlock_vma_page(old_page);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1567092 — Re: [PATCH 09/12] mm, uprobes: convert __replace_page() to page_check_walk()

* Kirill A. Shutemov <kirill.shutemov@linux.intel.com> [2017-01-24 19:28:21]:

> For consistency, it worth converting all page_check_address() to
> page_check_walk(), so we could drop the former.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  kernel/events/uprobes.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 1e65c79e52a6..6dbaa93b22fa 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -153,14 +153,19 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
>  				struct page *old_page, struct page *new_page)
>  {
>  	struct mm_struct *mm = vma->vm_mm;

I thought the subject is a bit misleading, it looks as if we are
replacing __replace_page. Can it be changed to
"Convert __replace_page() to use page_check_walk()" ?

Otherwise looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web