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

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

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?

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