Re: [PATCH v2 5/5] selftests/mm: retry migration failures for the full runtime

[Muhammad Usama Anjum <usama.anjum@arm.com>]

On 24/07/2026 4:23 pm, Dev Jain wrote:
> 
> 
> On 24/07/26 5:23 pm, Muhammad Usama Anjum wrote:
>> On 24/07/2026 12:46 pm, Dev Jain wrote:
>>>
>>>
>>> On 24/07/26 3:54 pm, Muhammad Usama Anjum wrote:
>>>> move_pages() is best effort and can temporarily fail when concurrent
>>>> faults race with page unmapping. A busy shared-anon workload can exhaust
>>>> the current 100 retries long before the intended 20-second runtime and
>>>> produce a false failure.
>>>>
>>>> Use the full runtime as the retry window. Since the initial page location
>>>> is unknown, require it to reach both alternating NUMA targets to confirm
>>>> that cross-node migration made progress despite transient contention.
>>>>
>>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
>>>> ---
>>>
>>> Makes sense, but see below.
>>>
>>>
>>>> Changes since v1:
>>>> - Retry per-page failures for the full runtime
>>>> - Verify that both alternating NUMA targets are reached
>>>> ---
>>>>  tools/testing/selftests/mm/migration.c | 39 ++++++++++++++------------
>>>>  1 file changed, 21 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
>>>> index 29f7492453d43..4d55a424058a9 100644
>>>> --- a/tools/testing/selftests/mm/migration.c
>>>> +++ b/tools/testing/selftests/mm/migration.c
>>>> @@ -7,7 +7,7 @@
>>>>  #include "kselftest_harness.h"
>>>>  #include "hugepage_settings.h"
>>>>  
>>>> -#include <strings.h>
>>>> +#include <string.h>
>>>>  #include <pthread.h>
>>>>  #include <numa.h>
>>>>  #include <numaif.h>
>>>> @@ -20,7 +20,6 @@
>>>>  
>>>>  #define TWOMEG		(2<<20)
>>>>  #define RUNTIME		(20)
>>>> -#define MAX_RETRIES	100
>>>>  #define ALIGN(x, a)	(((x) + (a - 1)) & (~((a) - 1)))
>>>>  
>>>>  HUGETLB_SETUP_DEFAULT_PAGES(1)
>>>> @@ -110,7 +109,7 @@ int migrate(uint64_t *ptr, int n1, int n2)
>>>>  	int ret, tmp;
>>>>  	int status = 0;
>>>>  	struct timespec ts1, ts2;
>>>> -	int failures = 0;
>>>> +	int success = 0;
>>>>  
>>>>  	if (clock_gettime(CLOCK_MONOTONIC, &ts1))
>>>>  		return -1;
>>>> @@ -119,29 +118,33 @@ int migrate(uint64_t *ptr, int n1, int n2)
>>>>  		if (clock_gettime(CLOCK_MONOTONIC, &ts2))
>>>>  			return -1;
>>>>  
>>>> -		if (ts2.tv_sec - ts1.tv_sec >= RUNTIME)
>>>> -			return 0;
>>>> +		if (ts2.tv_sec - ts1.tv_sec >= RUNTIME) {
>>>> +			/* Reaching both targets verifies a cross-node move. */
>>>> +			if (success >= 2)
>>>> +				return 0;
>>>> +			else
>>>> +				return -2;
>>>> +		}
>>>>  
>>>>  		ret = move_pages(0, 1, (void **) &ptr, &n2, &status,
>>>>  				MPOL_MF_MOVE_ALL);
>>>> -		if (ret) {
>>>> -			if (ret > 0) {
>>>> -				/* Migration is best effort; try again */
>>>> -				if (++failures < MAX_RETRIES)
>>>> -					continue;
>>>> -				printf("Didn't migrate %d pages\n", ret);
>>>> -			}
>>>> -			else
>>>> -				perror("Couldn't migrate pages");
>>>> -			return -2;
>>>> +		if (ret < 0) {
>>>> +			perror("Couldn't migrate pages");
>>>> +			return ret;
>>>>  		}
>>>> -		failures = 0;
>>>> +		/* Migration is best effort. Try again */
>>>> +		if (ret > 0 || status < 0)
> 
> Also what if you migrated accidentally to node 0? In which case you
> will return a false success below.
What do you mean by "migrated accidentally to node 0"? Can you give an
example?

> 
> 
>>>
>>> old code wasn't using status, so why now?
>> The old code not using it does not mean we cannot use it now. ret gives the
> 
> I never implied that :) The observation was because I didn't see it mentioned
> in the commit message, this change is orthogonal to the description.
> 
> 
>> aggregate result, while status gives the per-page result: the destination
>> node on success or a negative errno explaining the failure. In particular,
>> move_pages() can return 0 with a negative status, so checking it prevents a
>> failed move from being counted as successful.
> 
> Crazy stuff. So move_pages() can return zero but that return value is not
> really the return value and you have to check status.
> 
> The man page reads:
> 
> "status is an array of integers that return the status of each page.
> The array contains valid values only if move_pages() did not return
> an error."
> 
> But then says
> 
> "On success move_pages() returns zero.  On error, it returns -1, and sets
> errno to indicate the  error. If positive value is returned, it is the
> number of nonmigrated pages."
> 
> I don't know which one to fix, the manpage or the ABI, I suspect the latter,
> storing error codes in a variable and then returning zero sounds wrong.
> 
>>
>>>
>>>> +			continue;
>>>> +		if (status != n2) {
>>>> +			printf("Page is on node %d instead of target node %d\n",
>>>> +			       status, n2);
>>>> +			return status;
>>>> +		}
>>>> +		success++;
>>>>  		tmp = n2;
>>>>  		n2 = n1;
>>>>  		n1 = tmp;
>>>>  	}
>>>> -
>>>> -	return 0;
> 
> There is no return statement after the while loop. I suspect on some compiler
> config it is gonna complain about this, or someone inevitably will run the
> selftests with an old compiler and complain about this :)
The loop cannot fall through, and the minimum supported GCC and Clang versions
recognize that. No trailing return is needed.

> 
>>>>  }
>>>>  
>>>>  void *access_mem(void *ptr)
>>>
>>
> 

-- 
Thanks,
Usama