lkml.org 
[lkml]   [2019]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified
    From
    Date


    On 3/20/19 1:16 AM, Oscar Salvador wrote:
    > On Wed, Mar 20, 2019 at 02:35:56AM +0800, Yang Shi wrote:
    >> Fixes: 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()")
    >> Reported-by: Cyril Hrubis <chrubis@suse.cz>
    >> Cc: Vlastimil Babka <vbabka@suse.cz>
    >> Cc: stable@vger.kernel.org
    >> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
    >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
    >> Signed-off-by: Oscar Salvador <osalvador@suse.de>
    > Hi Yang, thanks for the patch.
    >
    > Some observations below.
    >
    >> }
    >> page = pmd_page(*pmd);
    >> @@ -473,8 +480,15 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
    >> ret = 1;
    >> flags = qp->flags;
    >> /* go to thp migration */
    >> - if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
    >> + if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
    >> + if (!vma_migratable(walk->vma)) {
    >> + ret = -EIO;
    >> + goto unlock;
    >> + }
    >> +
    >> migrate_page_add(page, qp->pagelist, flags);
    >> + } else
    >> + ret = -EIO;
    > if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
    > !vma_migratable(walk->vma)) {
    > ret = -EIO;
    > goto unlock;
    > }
    >
    > migrate_page_add(page, qp->pagelist, flags);
    > unlock:
    > spin_unlock(ptl);
    > out:
    > return ret;
    >
    > seems more clean to me?

    Yes, it sounds so.

    >
    >
    >> unlock:
    >> spin_unlock(ptl);
    >> out:
    >> @@ -499,8 +513,10 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
    >> ptl = pmd_trans_huge_lock(pmd, vma);
    >> if (ptl) {
    >> ret = queue_pages_pmd(pmd, ptl, addr, end, walk);
    >> - if (ret)
    >> + if (ret > 0)
    >> return 0;
    >> + else if (ret < 0)
    >> + return ret;
    > I would go with the following, but that's a matter of taste I guess.
    >
    > if (ret < 0)
    > return ret;
    > else
    > return 0;

    No, this is not correct. queue_pages_pmd() may return 0, which means THP
    gets split. If it returns 0 the code should just fall through instead of
    returning.

    >
    >> }
    >>
    >> if (pmd_trans_unstable(pmd))
    >> @@ -521,11 +537,16 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
    >> continue;
    >> if (!queue_pages_required(page, qp))
    >> continue;
    >> - migrate_page_add(page, qp->pagelist, flags);
    >> + if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
    >> + if (!vma_migratable(vma))
    >> + break;
    >> + migrate_page_add(page, qp->pagelist, flags);
    >> + } else
    >> + break;
    > I might be missing something, but AFAICS neither vma nor flags is going to change
    > while we are in queue_pages_pte_range(), so, could not we move the check just
    > above the loop?
    > In that way, 1) we only perform the check once and 2) if we enter the loop
    > we know that we are going to do some work, so, something like:
    >
    > index af171ccb56a2..7c0e44389826 100644
    > --- a/mm/mempolicy.c
    > +++ b/mm/mempolicy.c
    > @@ -487,6 +487,9 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
    > if (pmd_trans_unstable(pmd))
    > return 0;
    >
    > + if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) || !vma_migratable(vma))
    > + return -EIO;

    It sounds not correct to me. We need check if there is existing page on
    the node which is not allowed by the policy. This is what
    queue_pages_required() does.

    Thanks,
    Yang

    > +
    > pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl);
    > for (; addr != end; pte++, addr += PAGE_SIZE) {
    > if (!pte_present(*pte))
    >
    >
    >> }
    >> pte_unmap_unlock(pte - 1, ptl);
    >> cond_resched();
    >> - return 0;
    >> + return addr != end ? -EIO : 0;
    > If we can do the above, we can leave the return value as it was.
    >

    \
     
     \ /
      Last update: 2019-03-20 19:33    [W:6.655 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site