[LTP] [PATCH v2] Migrating the libhugetlbfs/testcases/shm-gettest.c test

[Sachin Bappalige]

Hi Cyril Hrubis,

Thanks for the review comments. Please check the reply in line

Also , Please check the v3 patch

On 10/30/2023 9:19 PM, Cyril Hrubis wrote:
> Hi!
>> v2:
>>   -Fixed coding style errors as per 'make check'
>>   -Reporting TPASS moved inside do_shmtest() function
>>   -Moved testcase file from folder hugemmap to hugeshmget
>>   -Renamed testcase 'hugepage35.c' to hugeshmget06.c
>>   -Updated 'kernel/mem/.gitignore'
>>   -Updated 'runtest/hugetlb' for number of iterations with -i 10
> This version looks much better, a few comments below.
>
>> diff --git a/testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget06.c b/testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget06.c
>> new file mode 100644
>> index 000000000..5b0c2ec23
>> --- /dev/null
>> +++ b/testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget06.c
>> @@ -0,0 +1,93 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2005-2006, IBM Corporation.
>> + * Author: David Gibson & Adam Litke
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * Test Name: shm-gettest.c
>                  ^
> 		This is no longer true and should be removed

Sachin >> This line is removed. And the DESCRIPTION similar to matched 
with other testcases in

>> + * This testcase creates shared memory segments backed by hugepages,
>> + * writes specific patterns to each segment, verifies pattern,
>> + * and detaches a shared memory segments in a loop.
>> + * It ensures that the hugepage backed shared memory functionalities
>> + * works correctly by validating the data written to segment.
>> + */
>> +
>> +#include "hugetlb.h"
>> +#include "tst_safe_sysv_ipc.h"
>> +
>> +#define MNTPOINT "hugetlbfs/"
>> +#define NR_HUGEPAGES 4
>> +
>> +static long hpage_size;
>> +static int shmid = -1, key = -1;
>> +
>> +static void do_shmtest(size_t size)
>> +{
>> +	size_t i, j;
>> +	char pattern;
>> +	char *shmaddr;
>> +
>> +	shmid = SAFE_SHMGET(key, size, SHM_HUGETLB|IPC_CREAT|SHM_R|SHM_W);
>> +	tst_res(TINFO, "shmid: 0x%x\n", shmid);
> THis should be moved into setup, since otherwise we would create one id
> per iteration (run test with -i 2) and the test will leak shm ids.
Sachin >> That's correct  and moved that code  to setup.
>
>> +	shmaddr = SAFE_SHMAT(shmid, 0, SHM_RND);
>> +	tst_res(TINFO, "shmaddr: %p\n", shmaddr);
>> +
>> +	for (i = 0; i < NR_HUGEPAGES; i++) {
>> +		pattern = 65 + (i % 26);
>> +		tst_res(TINFO, "Touching %p with %c\n",
>> +				shmaddr + (i * hpage_size), pattern);
>> +		memset(shmaddr + (i * hpage_size), pattern, hpage_size);
>> +	}
>> +
>> +	for (i = 0; i < NR_HUGEPAGES; i++) {
>> +		pattern = 65 + (i % 26);
>> +		tst_res(TINFO, "Verifying %p\n", (shmaddr + (i * hpage_size)));
>> +		for (j = 0; j < (size_t)hpage_size; j++)
>> +			if (*(shmaddr + (i * hpage_size) + j) != pattern)
>> +				tst_res(TFAIL, "Verifying the segment failed."
>> +						"Got %c, expected %c",
>> +						*(shmaddr + (i * hpage_size) + j),
>> +						pattern);
> If we print fail here we will still continue and print the TPASS at the
> end of the function. Should we instead do shmdt() and return here?


Sachin >>  Yes . After Failure, it should return. That part added to v3 
patch

>
> Also the message can be shorter while keeping the information in there
> and there is no guarantee that the corrupted byte would be printable, so
> I would print hex instead, something as:
>
> 	tst_res(TFAIL, "Got wrong byte 0x%02x expected 0x%02x", ...


Sachin >> Modified as per suggestion

>
>> +	}
>> +	SAFE_SHMDT((const void *)shmaddr);
>> +	tst_res(TPASS, "Successfully tested shared memory segment operations "
>> +			"backed by hugepages");
> Can we be shorter and to the point? Something as:
>
> tst_res(TPASS, "shm hugepages works correctly");
Sachin >> Done
>
>> +}
>> +
>> +static void run_test(void)
>> +{
>> +	size_t size;
>> +
>> +	size = NR_HUGEPAGES * hpage_size;
>> +
>> +	do_shmtest(size);
> Is there a reason why this isn't simply do_shmtest(NR_HUGEPAGES * hpage_size); ?
Sachin >>  yes .I modified as per suggestion
>
>> +}
>> +
>> +static void setup(void)
>> +{
>> +
> This newline shouldn't be here.
Sachin >> Remove this line
>
>> +	hpage_size = tst_get_hugepage_size();
>> +	tst_res(TINFO, "hugepage size is  %ld", hpage_size);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (shmid >= 0)
>> +		// Remove the shared memory segment
> Please do not comment the obvious.
Sachin >> Removed this comment
>
>> +		SAFE_SHMCTL(shmid, IPC_RMID, NULL);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.needs_root = 1,
>> +	.mntpoint = MNTPOINT,
>> +	.needs_hugetlbfs = 1,
> Do we actually need to mount the hugetlbfs?


Sachin >> If  I remove this line from code , it gives this error

tst_test.c:1294: TBROK: tst_test->mntpoint must be set!


>
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = run_test,
>> +	.hugepages = {NR_HUGEPAGES, TST_NEEDS},
>> +};
>> -- 
>> 2.39.3
>>
>>
>> -- 
>> Mailing list info:https://lists.linux.it/listinfo/ltp