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

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

On 24/07/26 9:27 pm, Muhammad Usama Anjum wrote:
> 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?

I meant it can happen that ret is 0, status is 0. Then you will pass
"status != n2" and the function will return 0.

So probably just return -2 instead of status?

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

Okay.


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