lkml.org 
[lkml]   [2017]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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);
    > + }
    > + }
    > +}

    \
     
     \ /
      Last update: 2017-01-24 22:42    [W:4.146 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site