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

[Cyril Hrubis <chrubis@suse.cz>]

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