lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <efe90132-6832-d61a-5d55-d2cc134c7087@linux.alibaba.com>
Date:   Tue, 16 Jul 2019 10:18:26 -0700
From:   Yang Shi <yang.shi@...ux.alibaba.com>
To:     Vlastimil Babka <vbabka@...e.cz>, mhocko@...nel.org,
        mgorman@...hsingularity.net, akpm@...ux-foundation.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when
 MPOL_MF_MOVE* and MPOL_MF_STRICT were specified



On 7/16/19 1:12 AM, Vlastimil Babka wrote:
> On 6/22/19 2:20 AM, Yang Shi wrote:
>> When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should
>> try best to migrate misplaced pages, if some of the pages could not be
>> migrated, then return -EIO.
>>
>> There are three different sub-cases:
>> 1. vma is not migratable
>> 2. vma is migratable, but there are unmovable pages
>> 3. vma is migratable, pages are movable, but migrate_pages() fails
>>
>> If #1 happens, kernel would just abort immediately, then return -EIO,
>> after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm:
>> mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified").
>>
>> If #3 happens, kernel would set policy and migrate pages with best-effort,
>> but won't rollback the migrated pages and reset the policy back.
>>
>> Before that commit, they behaves in the same way.  It'd better to keep
>> their behavior consistent.  But, rolling back the migrated pages and
>> resetting the policy back sounds not feasible, so just make #1 behave as
>> same as #3.
>>
>> Userspace will know that not everything was successfully migrated (via
>> -EIO), and can take whatever steps it deems necessary - attempt rollback,
>> determine which exact page(s) are violating the policy, etc.
>>
>> Make queue_pages_range() return 1 to indicate there are unmovable pages
>> or vma is not migratable.
>>
>> The #2 is not handled correctly in the current kernel, the following
>> patch will fix it.
>>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Mel Gorman <mgorman@...hsingularity.net>
>> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> Agreed with the goal, but I think there's a bug, and room for improvement.
>
>> ---
>>   mm/mempolicy.c | 86 +++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 61 insertions(+), 25 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 01600d8..b50039c 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
>>   }
>>   
>>   /*
>> - * queue_pages_pmd() has three possible return values:
>> + * queue_pages_pmd() has four possible return values:
>> + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
>> + *     specified.
>>    * 1 - pages are placed on the right node or queued successfully.
>>    * 0 - THP was split.
> I think if you renumbered these, it would be more consistent with
> queue_pages_pte_range() and simplify the code there.
> 0 - pages on right node/queued
> 1 - unmovable page with right flags specified
> 2 - THP split
>
>> - * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing
>> - *        page was already on a node that does not follow the policy.
>> + * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
>> + *        existing page was already on a node that does not follow the
>> + *        policy.
>>    */
>>   static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>>   				unsigned long end, struct mm_walk *walk)
>> @@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>>   	/* go to thp migration */
>>   	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>>   		if (!vma_migratable(walk->vma)) {
>> -			ret = -EIO;
>> +			ret = 2;
>>   			goto unlock;
>>   		}
>>   
>> @@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
> Perhaps this function now also deserves a list of possible return values.

Sure, will add some comments to elaborate the return values.

>
>>   	struct queue_pages *qp = walk->private;
>>   	unsigned long flags = qp->flags;
>>   	int ret;
>> +	bool has_unmovable = false;
>>   	pte_t *pte;
>>   	spinlock_t *ptl;
>>   
>>   	ptl = pmd_trans_huge_lock(pmd, vma);
>>   	if (ptl) {
>>   		ret = queue_pages_pmd(pmd, ptl, addr, end, walk);
>> -		if (ret > 0)
>> +		switch (ret) {
> With renumbering suggested above, this could be:
> if (ret != 2)
>      return ret;
>
>> +		/* THP was split, fall through to pte walk */
>> +		case 0:
>> +			break;
>> +		/* Pages are placed on the right node or queued successfully */
>> +		case 1:
>>   			return 0;
>> -		else if (ret < 0)
>> +		/*
>> +		 * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT
>> +		 * were specified.
>> +		 */
>> +		case 2:
>> +			return 1;
>> +		case -EIO:
>>   			return ret;
>> +		}
>>   	}
>>   
>>   	if (pmd_trans_unstable(pmd))
>> @@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
>>   		if (!queue_pages_required(page, qp))
>>   			continue;
>>   		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>> -			if (!vma_migratable(vma))
>> +			/* MPOL_MF_STRICT must be specified if we get here */
>> +			if (!vma_migratable(vma)) {
>> +				has_unmovable |= true;
>>   				break;
>> +			}
>>   			migrate_page_add(page, qp->pagelist, flags);
>>   		} else
>>   			break;
>>   	}
>>   	pte_unmap_unlock(pte - 1, ptl);
>>   	cond_resched();
>> +
>> +	if (has_unmovable)
>> +		return 1;
>> +
>>   	return addr != end ? -EIO : 0;
>>   }
>>   
>> @@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
>>    *
>>    * If pages found in a given range are on a set of nodes (determined by
>>    * @nodes and @flags,) it's isolated and queued to the pagelist which is
>> - * passed via @private.)
>> + * passed via @private.
>> + *
>> + * queue_pages_range() has three possible return values:
>> + * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were
>> + *     specified.
>> + * 0 - queue pages successfully or no misplaced page.
>> + * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified.
>>    */
>>   static int
>>   queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
>> @@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start, unsigned long len,
>>   	struct mempolicy *new;
>>   	unsigned long end;
>>   	int err;
>> +	int ret;
>>   	LIST_HEAD(pagelist);
>>   
>>   	if (flags & ~(unsigned long)MPOL_MF_VALID)
>> @@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start, unsigned long len,
>>   	if (err)
>>   		goto mpol_out;
>>   
>> -	err = queue_pages_range(mm, start, end, nmask,
>> +	ret = queue_pages_range(mm, start, end, nmask,
>>   			  flags | MPOL_MF_INVERT, &pagelist);
>> -	if (!err)
>> -		err = mbind_range(mm, start, end, new);
>> -
>> -	if (!err) {
>> -		int nr_failed = 0;
>>   
>> -		if (!list_empty(&pagelist)) {
>> -			WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>> -			nr_failed = migrate_pages(&pagelist, new_page, NULL,
>> -				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
>> -			if (nr_failed)
>> -				putback_movable_pages(&pagelist);
>> -		}
>> +	if (ret < 0)
>> +		err = -EIO;
> I think after your patch, you miss putback_movable_pages() in cases
> where some were queued, and later the walk returned -EIO. The previous
> code doesn't miss it, but it's also not obvious due to the multiple if
> (!err) checks. I would rewrite it some thing like this:
>
> if (ret < 0) {
>      putback_movable_pages(&pagelist);
>      err = ret;
>      goto mmap_out; // a new label above up_write()
> }

Yes, the old code had putback_movable_pages called when !err. But, I 
think that is for error handling of mbind_range() if I understand it 
correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT 
was specified and there was misplaced page) that page list should be 
empty . The old code should checked whether that list is empty or not.

So, in the new code I just removed that.

>
> The rest can have reduced identation now.

Yes, the goto does eliminate the extra indentation.

>
>> +	else {
>> +		err = mbind_range(mm, start, end, new);
>>   
>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>> -			err = -EIO;
>> -	} else
>> -		putback_movable_pages(&pagelist);
>> +		if (!err) {
>> +			int nr_failed = 0;
>> +
>> +			if (!list_empty(&pagelist)) {
>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>> +				nr_failed = migrate_pages(&pagelist, new_page,
>> +					NULL, start, MIGRATE_SYNC,
>> +					MR_MEMPOLICY_MBIND);
>> +				if (nr_failed)
>> +					putback_movable_pages(&pagelist);
>> +			}
>> +
>> +			if ((ret > 0) ||
>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>> +				err = -EIO;
>> +		} else
>> +			putback_movable_pages(&pagelist);
> While at it, IIRC the kernel style says that when the 'if' part uses
> '{ }' then the 'else' part should as well, and it shouldn't be mixed.

Really? The old code doesn't have '{ }' for else, and checkpatch doesn't 
report any error or warning.

Thanks,
Yang

>
> Thanks,
> Vlastimil
>
>> +	}
>>   
>>   	up_write(&mm->mmap_sem);
>>    mpol_out:
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ