[LTP] [PATCH] Add hugeshmmisc01, migrated shm-getraw.c from libhugetlbfs

[Cyril Hrubis]

Hi!
>  runtest/hugetlb                               |   2 +
>  testcases/kernel/mem/.gitignore               |   1 +
>  .../kernel/mem/hugetlb/hugeshmmisc/Makefile   |   9 ++
>  .../mem/hugetlb/hugeshmmisc/hugeshmmisc01.c   | 134 ++++++++++++++++++
>  4 files changed, 146 insertions(+)
>  create mode 100644 testcases/kernel/mem/hugetlb/hugeshmmisc/Makefile
>  create mode 100644 testcases/kernel/mem/hugetlb/hugeshmmisc/hugeshmmisc01.c
> 
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 299c07ac9..ea18f6ef6 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -55,3 +55,5 @@ hugeshmget01 hugeshmget01 -i 10
>  hugeshmget02 hugeshmget02 -i 10
>  hugeshmget03 hugeshmget03 -i 10
>  hugeshmget05 hugeshmget05 -i 10
> +
> +hugeshmmisc01 hugeshmmisc01
> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index c96fe8bfc..493679ae6 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/hugeshmmisc/hugeshmmisc01
>  /ksm/ksm01
>  /ksm/ksm02
>  /ksm/ksm03
> diff --git a/testcases/kernel/mem/hugetlb/hugeshmmisc/Makefile b/testcases/kernel/mem/hugetlb/hugeshmmisc/Makefile
> new file mode 100644
> index 000000000..84715c7b5
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugeshmmisc/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2009, Cisco Systems Inc.
> +# Ngie Cooper, July 2009
> +
> +top_srcdir              ?= ../../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(abs_srcdir)/../Makefile.inc
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/mem/hugetlb/hugeshmmisc/hugeshmmisc01.c b/testcases/kernel/mem/hugetlb/hugeshmmisc/hugeshmmisc01.c
> new file mode 100644
> index 000000000..15867f251
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugeshmmisc/hugeshmmisc01.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
> + */
> +
> +/*\
> + *[Descripiton]
> + *
> + * Origin: https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/shm-getraw.c
> + *
> + * The test creates a shared memory segment, then attaches it to the process’s address space.
> + * It writes a string to the shared memory from raw device and detaches the shared memory
> + * segment and finally removes it.
> + * The purpose of this test is to ensure that the shared memory subsystem is working correctly
> + * with hugepages. It checks that shared memory segments can be created, attached, written to,
> + * read from, detached, and removed without errors
> + *
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +#include <assert.h>
> +#include <sys/types.h>
> +#include <sys/shm.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +
> +#include "hugetlb.h"
> +#include "tst_safe_sysv_ipc.h"
> +
> +#define MNTPOINT "hugetlbfs/"
> +#define SAFE_FREE(p) { if (p) { free(p); (p) = NULL; } }

Please do not do this.

> +#define NR_HUGEPAGES 2
> +
> +static int shmid = -1;
> +static size_t size;
> +static size_t i;
> +static size_t ret;
> +
> +static volatile char *shmaddr;
> +static char *buffer;
> +static int raw_fd;
> +static long hpage_size;
> +static int fd;
> +
> +static void setup(void)
> +{
> +	hpage_size = tst_get_hugepage_size();
> +	fd = tst_creat_unlinked(MNTPOINT, 0);

We do not use this fd for anything in the test since we are getting
hugepages from SHM instead. There is no point in mounting the hugetlbfs
either.

> +}
> +
> +static void cleanup(void)
> +{
> +	if (shmid >= 0)
> +		SAFE_SHMCTL(shmid, IPC_RMID, NULL);
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +}
> +
> +static void check_hugetlb_shm_group(void)
> +{
> +	char gid_buffer[64] = {0};
> +	gid_t hugetlb_shm_group;
> +	gid_t gid = getgid();
> +	uid_t uid = getuid();
> +
> +	/* root is an exception */
> +	if (uid == 0)
> +		return;

The test has .needs_root = 1 in tst_test so this is always true.

> +	fd = SAFE_OPEN("/proc/sys/vm/hugetlb_shm_group", O_RDONLY);
> +	ret = SAFE_READ(0, fd, &gid_buffer, sizeof(gid_buffer));
> +	hugetlb_shm_group = atoi(gid_buffer);
> +	SAFE_CLOSE(fd);
> +	if (hugetlb_shm_group != gid)
> +		tst_brk(TCONF, "Do not have permission to use SHM_HUGETLB");
> +}
> +
> +static void run_test(void)
> +{
> +	check_hugetlb_shm_group();
> +	buffer = SAFE_MALLOC(hpage_size*sizeof(char));
                                        ^
					This is 1 by definition

And there is no point in multiplying a number by 1...

Also the buffer should be allocated once in the test setup and freed
once in the test cleanup.


> +	raw_fd = SAFE_OPEN("/dev/full", O_RDONLY);
> +	size = hpage_size * NR_HUGEPAGES;
> +	tst_res(TINFO, "Requesting %zu bytes\n", size);
> +
> +	shmid = shmget(2, size, SHM_HUGETLB|IPC_CREAT|SHM_R|SHM_W);

You shouldn't hardcode keys in the tests like this.

I guess that since we are not actually sharing the memory with other
processes we can as well pass IPC_PRIVATE as a key instead.

> +	if (shmid < 0) {
> +		tst_res(TFAIL | TERRNO, "shmget() failed with error ");
> +		goto windup;
> +	} else
> +		tst_res(TINFO, "shmid: 0x%x\n", shmid);

You should just use SAFE_SHMGET() instead this.

> +	shmaddr = shmat(shmid, 0, SHM_RND);
> +	if (shmaddr == MAP_FAILED) {
> +		tst_res(TFAIL | TERRNO, "shmat() failed with error ");
> +		goto windup;
> +	} else
> +		tst_res(TINFO, "shmaddr: %p\n", shmaddr);

And we have SAFE_SHMAT() as well.

> +	/* Read a page from device and write to shm segment */
> +	for (i = 0; i < size; i += hpage_size) {
> +		if (!read(raw_fd, buffer, hpage_size)) {
> +			tst_res(TFAIL | TERRNO, "Can't read from raw device!");
> +			goto windup;
> +		}
> +		memcpy((char *)(shmaddr + i), buffer, hpage_size);

I do not see the point of this. We read from a device into a regular
buffer we got from malloc() and then copy it to the SHM memory.

As far as I can tell from the point of this test this is no different
than actually doing memset() to zero the buffer instead.

It would be a different story if we passed the SHM buffer to the read
syscal, that would mean tha the shm buffer is actually passed down to
the kernel. And this seems to be written in the test description too:

"It writes a string to the shared memory from raw device..."

> +	}
> +
> +	if (shmdt((const void *)shmaddr) != 0) {
> +		tst_res(TFAIL | TERRNO, "shmdt() failed");
> +		goto windup;
> +	}

And we have SAFE_SHMDT() too.

> +	tst_res(TPASS, "Test Passed!");
> +windup:
> +	SAFE_FREE(buffer);
> +}
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.needs_hugetlbfs = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {2, TST_NEEDS},
> +};
> -- 
> 2.39.3 (Apple Git-146)
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz