Received-SPF: pass (google.com: domain of linux-kernel+bounces-102988-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <07e2e91b-7b0d-4810-a2a1-ed4654ecafb4@collabora.com>
Date: Thu, 14 Mar 2024 13:46:22 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>, kernel@collabora.com,
 linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org, Anshuman.Khandual@arm.com
Subject: Re: [PATCH v2 12/12] selftests/mm: virtual_address_range: conform to
 TAP format output
To: Dev Jain <dev.jain@arm.com>, Andrew Morton <akpm@linux-foundation.org>,
 Shuah Khan <shuah@kernel.org>
References: <20240202113119.2047740-1-usama.anjum@collabora.com>
 <20240202113119.2047740-13-usama.anjum@collabora.com>
 <12b0aa90-3b4f-4fb0-b4df-c677f496cb09@arm.com>
Content-Language: en-US
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <12b0aa90-3b4f-4fb0-b4df-c677f496cb09@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 3/14/24 10:00 AM, Dev Jain wrote:
> 
> On 2/2/24 17:01, Muhammad Usama Anjum wrote:
>> Conform the layout, informational and status messages to TAP. No
>> functional change is intended other than the layout of output messages.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>   .../selftests/mm/virtual_address_range.c      | 44 +++++++++----------
>>   1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/virtual_address_range.c
>> b/tools/testing/selftests/mm/virtual_address_range.c
>> index bae0ceaf95b13..7bcf8d48256a6 100644
>> --- a/tools/testing/selftests/mm/virtual_address_range.c
>> +++ b/tools/testing/selftests/mm/virtual_address_range.c
>> @@ -12,6 +12,7 @@
>>   #include <errno.h>
>>   #include <sys/mman.h>
>>   #include <sys/time.h>
>> +#include "../kselftest.h"
>>     /*
>>    * Maximum address range mapped with a single mmap()
>> @@ -68,23 +69,15 @@ static char *hind_addr(void)
>>       return (char *) (1UL << bits);
>>   }
>>   -static int validate_addr(char *ptr, int high_addr)
>> +static void validate_addr(char *ptr, int high_addr)
>>   {
>>       unsigned long addr = (unsigned long) ptr;
>>   -    if (high_addr) {
>> -        if (addr < HIGH_ADDR_MARK) {
>> -            printf("Bad address %lx\n", addr);
>> -            return 1;
>> -        }
>> -        return 0;
>> -    }
>> +    if (high_addr && addr < HIGH_ADDR_MARK)
>> +        ksft_exit_fail_msg("Bad address %lx\n", addr);
>>   -    if (addr > HIGH_ADDR_MARK) {
>> -        printf("Bad address %lx\n", addr);
>> -        return 1;
>> -    }
>> -    return 0;
>> +    if (addr > HIGH_ADDR_MARK)
>> +        ksft_exit_fail_msg("Bad address %lx\n", addr);
>>   }
>>     static int validate_lower_address_hint(void)
>> @@ -107,23 +100,29 @@ int main(int argc, char *argv[])
>>       char *hint;
>>       unsigned long i, lchunks, hchunks;
>>   +    ksft_print_header();
>> +    ksft_set_plan(1);
>> +
>>       for (i = 0; i < NR_CHUNKS_LOW; i++) {
>>           ptr[i] = mmap(NULL, MAP_CHUNK_SIZE, PROT_READ | PROT_WRITE,
>>                       MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>             if (ptr[i] == MAP_FAILED) {
>> -            if (validate_lower_address_hint())
>> -                return 1;
>> +            if (validate_lower_address_hint()) {
>> +                ksft_test_result_skip("Memory constraint not fulfilled\n");
>> +                ksft_finished();
>> +            }
> 
> Hi,
> 
> When validate_lower_address_hint() returns 1, it implies that despite
> filling the lower
> 
> range, mmap succeeded. IMHO, ksft_exit_fail_msg() should be used instead,
> with a
> 
> more descriptive message indicating that the memory was unexpectedly
> allocated.
Can you fire up a patch for this? Otherwise I'll get back to it next week.

> 
> 
> Regards
> 
> Dev
> 
>>               break;
>>           }
>>   -        if (validate_addr(ptr[i], 0))
>> -            return 1;
>> +        validate_addr(ptr[i], 0);
>>       }
>>       lchunks = i;
>>       hptr = (char **) calloc(NR_CHUNKS_HIGH, sizeof(char *));
>> -    if (hptr == NULL)
>> -        return 1;
>> +    if (hptr == NULL) {
>> +        ksft_test_result_skip("Memory constraint not fulfilled\n");
>> +        ksft_finished();
>> +    }
>>         for (i = 0; i < NR_CHUNKS_HIGH; i++) {
>>           hint = hind_addr();
>> @@ -133,8 +132,7 @@ int main(int argc, char *argv[])
>>           if (hptr[i] == MAP_FAILED)
>>               break;
>>   -        if (validate_addr(hptr[i], 1))
>> -            return 1;
>> +        validate_addr(hptr[i], 1);
>>       }
>>       hchunks = i;
>>   @@ -145,5 +143,7 @@ int main(int argc, char *argv[])
>>           munmap(hptr[i], MAP_CHUNK_SIZE);
>>         free(hptr);
>> -    return 0;
>> +
>> +    ksft_test_result_pass("Test\n");
>> +    ksft_finished();
>>   }

-- 
BR,
Muhammad Usama Anjum