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 <[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?
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)
>>>>
>>>
>>
>
