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

[Dev Jain <dev.jain@arm.com>]

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.


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

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