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

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

[Sachin P Bappalige]

Test Description: 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.

This looping test was added to verify the functionality of
large page backed shared memory segments. A segment is created,
written, verified, and detached a specified number of times.

Signed-off-by: Sachin P Bappalige <sachinpb@linux.vnet.ibm.com>
---
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
---
 runtest/hugetlb                               |  1 +
 testcases/kernel/mem/.gitignore               |  1 +
 .../mem/hugetlb/hugeshmget/hugeshmget06.c     | 93 +++++++++++++++++++
 3 files changed, 95 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget06.c

diff --git a/runtest/hugetlb b/runtest/hugetlb
index 299c07ac9..f294e9aaa 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -55,3 +55,4 @@ hugeshmget01 hugeshmget01 -i 10
 hugeshmget02 hugeshmget02 -i 10
 hugeshmget03 hugeshmget03 -i 10
 hugeshmget05 hugeshmget05 -i 10
+hugeshmget06 hugeshmget06 -i 10
diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
index 7258489ed..e74fd1246 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -47,6 +47,7 @@
 /hugetlb/hugeshmget/hugeshmget02
 /hugetlb/hugeshmget/hugeshmget03
 /hugetlb/hugeshmget/hugeshmget05
+/hugetlb/hugeshmget/hugeshmget06
 /ksm/ksm01
 /ksm/ksm02
 /ksm/ksm03
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 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);
+
+	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);
+	}
+	SAFE_SHMDT((const void *)shmaddr);
+	tst_res(TPASS, "Successfully tested shared memory segment operations "
+			"backed by hugepages");
+}
+
+static void run_test(void)
+{
+	size_t size;
+
+	size = NR_HUGEPAGES * hpage_size;
+
+	do_shmtest(size);
+}
+
+static void setup(void)
+{
+
+	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
+		SAFE_SHMCTL(shmid, IPC_RMID, NULL);
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.mntpoint = MNTPOINT,
+	.needs_hugetlbfs = 1,
+	.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

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

[Cyril Hrubis]

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
> + * 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.

> +	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?

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", ...

> +	}
> +	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");

> +}
> +
> +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); ?

> +}
> +
> +static void setup(void)
> +{
> +

This newline shouldn't be here.

> +	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.

> +		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?

> +	.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

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp