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 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)
> 
> 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
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.

> 
>> +			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;
>>  }
>>  
>>  void *access_mem(void *ptr)
> 

-- 
Thanks,
Usama