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 <[email protected]>
>>> ---
>>
>> 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)
>>
>
