Re: [PATCH v3 11/13] selftests/mm: fix double increment in linked list cleanup in compaction_test

[Sayali Patil <sayalip@linux.ibm.com>]

On 01/04/26 20:09, David Hildenbrand (Arm) wrote:
> On 4/1/26 16:32, Sayali Patil wrote:
>>
>> On 27/03/26 12:46, Sayali Patil wrote:
>>> The cleanup loop of allocated memory currently uses:
>>>
>>>       for (entry = list; entry != NULL; entry = entry->next) {
>>>           munmap(entry->map, MAP_SIZE);
>>>           if (!entry->next)
>>>               break;
>>>           entry = entry->next;
>>>       }
>>>
>>> The inner entry = entry->next causes the loop to skip every
>>> other node, resulting in only half of the mapped regions being
>>> unmapped.
>>>
>>> Remove the redundant increment to ensure every entry is visited
>>> and unmapped during cleanup.
>>>
>>> Fixes: bd67d5c15cc1 ("Test compaction of mlocked memory")
>>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>>> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>>> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
>>> Signed-off-by: Sayali Patil <sayalip@linux.ibm.com>
>>> ---
>>>    tools/testing/selftests/mm/compaction_test.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/
>>> testing/selftests/mm/compaction_test.c
>>> index 30209c40b697..f73930706bd0 100644
>>> --- a/tools/testing/selftests/mm/compaction_test.c
>>> +++ b/tools/testing/selftests/mm/compaction_test.c
>>> @@ -263,9 +263,6 @@ int main(int argc, char **argv)
>>>          for (entry = list; entry != NULL; entry = entry->next) {
>>>            munmap(entry->map, MAP_SIZE);
>>> -        if (!entry->next)
>>> -            break;
>>> -        entry = entry->next;
>>>        }
>>>          if (check_compaction(mem_free, hugepage_size,
>>
>> Sorry, this change is not valid.
>>
>> The goal of this test is to verify the kernel’s ability to compact
>> unevictable (MAP_LOCKED) pages. The loop is intentionally written to
>> unmap every other chunk, thereby creating fragmentation with locked pages
>> before check_compaction() is invoked.
>>
>> With the proposed change (removing the double increment), the loop ends up
>> unmapping all allocated locked pages instead of leaving a fragmented
>> pattern. This results in memory being effectively unfragmented.
> 
> Ahhh, we should really make that clearer in a comment. I missed it myself :(
> 
yes, let me add a comment to clarify this and send it in v4.