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 <[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.
What do you mean by "migrated accidentally to node 0"? Can you give an
example?
>
>
>>>
>>> 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.
>
>>>> }
>>>>
>>>> void *access_mem(void *ptr)
>>>
>>
>
--
Thanks,
Usama
