Re: [LTP] [PATCH] Migrating the libhugetlbfs/testcases/alloc-instantiate-race.c test

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
First of all there are style problems, please run 'make check' and fix
all problems before resubmitting.

> +/*\
> + * [Description]
> + *
> + * Test Name: alloc-instantiate-race.c
          ^
	  This is no longer true and should be removed.
> + * This test is designed to detect a kernel allocation race introduced
> + * with hugepage demand-faulting.  The problem is that no lock is held
> + * between allocating a hugepage and instantiating it in the
> + * pagetables or page cache index.  In between the two, the (huge)
> + * page is cleared, so there's substantial time.  Thus two processes
> + * can race instantiating the (same) last available hugepage - one
> + * will fail on the allocation, and thus cause an OOM fault even
> + * though the page it actually wants is being instantiated by the
> + * other racing process.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <pthread.h>
> +#include "tst_safe_pthread.h"
> +#include "hugetlb.h"
> +#define SYSFS_CPU_ONLINE_FMT	"/sys/devices/system/cpu/cpu%d/online"
> +#define MNTPOINT "hugetlbfs/"

> +unsigned long totpages;
> +static long hpage_size;
> +char *str_op;
> +int race_type;

There are some missing static keywors here.

> +static int child1, child2;
> +static pthread_t thread1, thread2;
> +
> +struct racer_info {
> +	void *p; /* instantiation address */
> +	int cpu;
> +	int race_type;

Why do we have race type duplicated here? We can just use the global
variable race_type instead I suppose.

> +	volatile int *mytrigger;
> +	volatile int *othertrigger;
> +	int status;
> +};
> +
> +static int one_racer(void *p, int cpu,
> +		     volatile int *mytrigger, volatile int *othertrigger)
> +{
> +	volatile int *pi = p;
> +	cpu_set_t cpuset;
> +	int err;
> +
> +	/* Split onto different cpus to encourage the race */
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(cpu, &cpuset);

This is the old API, the new API that should be used instead allocates
the mask dynamically with CPU_ALLOC().

> +	err = sched_setaffinity(gettid(), CPU_SETSIZE/8, &cpuset);
> +	if (err != 0)
> +		tst_res(TFAIL,"sched_setaffinity(cpu%d): %s", cpu, strerror(errno));
                                                                    ^
								    Use
								    TERRNO
								    instead.

Also what are we supposed to do here if the call fails?

Should we stop the test? If so this should be tst_brk(TBROK, ...).

If we should continue it should be warning, i.e. tst_res(TWARN, ...);

> +
> +	/* Ready.. */
> +	*mytrigger = 1;
> +	/* Set.. */
> +	while (! *othertrigger)
> +		;
> +
> +	/* Instantiate! */
> +	*pi = 1;
> +
> +	return 0;
> +}
> +
> +static void proc_racer(void *p, int cpu,
> +		       volatile int *mytrigger, volatile int *othertrigger)
> +{
> +	exit(one_racer(p, cpu, mytrigger, othertrigger));

There is no need to pass the return value from one_racer() to exit here
since one_racer() returns always 0, we should just do exit(0);
explicitly instead.

> +}
> +
> +static void *thread_racer(void *info)
> +{
> +	struct racer_info *ri = info;
> +	one_racer(ri->p, ri->cpu, ri->mytrigger, ri->othertrigger);
> +	return ri;
> +}
> +
> +void check_online_cpus(int online_cpus[], int nr_cpus_needed)
> +{
> +	char cpu_state, path_buf[64];
> +	int total_cpus, cpu_idx, fd, ret, i;
> +
> +	total_cpus = get_nprocs_conf();
> +	cpu_idx = 0;
> +
> +	if (get_nprocs() < nr_cpus_needed)
> +		tst_res(TFAIL,"Atleast online %d cpus are required", nr_cpus_needed);

This should be .min_cpus = 2 in tst_test.

> +	for (i = 0; i < total_cpus && cpu_idx < nr_cpus_needed; i++) {
> +		errno = 0;
> +		sprintf(path_buf, SYSFS_CPU_ONLINE_FMT, i);
> +		fd = open(path_buf, O_RDONLY);
> +		if (fd < 0) {
> +			/* If 'online' is absent, the cpu cannot be offlined */
> +			if (errno == ENOENT) {
> +				online_cpus[cpu_idx] = i;
> +				cpu_idx++;
> +				continue;
> +			} else {
> +				tst_res(TFAIL,"Unable to open %s: %s", path_buf,
> +				     strerror(errno));
> +			}
> +		}
> +
> +		ret = read(fd, &cpu_state, 1);
> +		if (ret < 1)
> +			tst_res(TFAIL,"Unable to read %s: %s", path_buf,
> +			     strerror(errno));
> +
> +		if (cpu_state == '1') {
> +			online_cpus[cpu_idx] = i;
> +			cpu_idx++;
> +		}
> +
> +		if (fd >= 0)
> +			SAFE_CLOSE(fd);

So all this is done to get two different CPU ids that could be used for
the affinity call.

I guess that it would be much easier to call sched_getaffinity() which
should fill the cpuset mask with cpus the thread is eligible to run on,
then we can simply find two non-zero bits in the mask.

> +	}
> +
> +	if (cpu_idx < nr_cpus_needed)
> +		tst_res(TFAIL,"Atleast %d online cpus were not found", nr_cpus_needed);
> +}
> +
> +static void run_race(void *syncarea, int race_type)
> +{
> +	volatile int *trigger1, *trigger2;
> +	int fd;
> +	void *p, *tret1, *tret2;
> +	int status1, status2;
> +	int online_cpus[2];
> +
> +	check_online_cpus(online_cpus, 2);
> +	memset(syncarea, 0, sizeof(*trigger1) + sizeof(*trigger2));
> +	trigger1 = syncarea;
> +	trigger2 = trigger1 + 1;
> +
> +	/* Get a new file for the final page */
> +	fd = tst_creat_unlinked(MNTPOINT, 0);
> +	tst_res(TINFO,"Mapping final page.. ");
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, race_type, fd, 0);
> +	if (race_type == MAP_SHARED) {
> +		child1 = SAFE_FORK();
> +		if (child1 == 0)
> +			proc_racer(p, online_cpus[0], trigger1, trigger2);
> +
> +		child2 = SAFE_FORK();
> +		if (child2 == 0)
> +			proc_racer(p, online_cpus[1], trigger2, trigger1);
> +
> +		/* wait() calls */
> +		SAFE_WAITPID(child1, &status1, 0);
> +		tst_res(TINFO,"Child 1 status: %x\n", status1);
> +
> +
> +		SAFE_WAITPID(child2, &status2, 0);
> +		tst_res(TINFO,"Child 2 status: %x\n", status2);
> +
> +		if (WIFSIGNALED(status1))
> +			tst_res(TFAIL,"Child 1 killed by signal %s",
> +			strsignal(WTERMSIG(status1)));
> +		if (WIFSIGNALED(status2))
> +			tst_res(TFAIL,"Child 2 killed by signal %s",
> +			strsignal(WTERMSIG(status2)));
> +
> +		status1 = WEXITSTATUS(status1);
> +		status2 = WEXITSTATUS(status2);

We do always exit(0) in the proc_racer() this is useless.

> +	} else {
> +		struct racer_info ri1 = {
> +			.p = p,
> +			.cpu = online_cpus[0],
> +			.mytrigger = trigger1,
> +			.othertrigger = trigger2,
> +		};
> +		struct racer_info ri2 = {
> +			.p = p,
> +			.cpu = online_cpus[1],
> +			.mytrigger = trigger2,
> +			.othertrigger = trigger1,
> +		};
> +		
> +		SAFE_PTHREAD_CREATE(&thread1, NULL, thread_racer, &ri1);
> +		SAFE_PTHREAD_CREATE(&thread2, NULL, thread_racer, &ri2);
> +		SAFE_PTHREAD_JOIN(thread1, &tret1);
> +		if (tret1 != &ri1)
> +			tst_res(TFAIL,"Thread 1 returned %p not %p, killed?\n",
> +			     tret1, &ri1);
> +		SAFE_PTHREAD_JOIN(thread2, &tret2);
> +		if (tret2 != &ri2)
> +			tst_res(TFAIL,"Thread 2 returned %p not %p, killed?\n",
> +			     tret2, &ri2);
> +		status1 = ri1.status;
> +		status2 = ri2.status;

Here as well I do not see the status set anywhere.

> +	}
> +
> +	if (status1 != 0)
> +		tst_res(TFAIL,"Racer 1 terminated with code %d", status1);
> +
> +	if (status2 != 0)
> +		tst_res(TFAIL,"Racer 2 terminated with code %d", status2);
> +
> +	if (fd >= 0)
> +		SAFE_CLOSE(fd);
> +}
> +
> +
> +static void run_test(void)
> +{
> +	int fd;
> +	long unsigned int i;
> +	void *p, *q;
> +	fd = tst_creat_unlinked(MNTPOINT, 0);
> +	/* Get a shared normal page for synchronization */
> +	tst_res(TINFO, "Mapping synchronization area..");

Useless comment and message.

> +	q = SAFE_MMAP(NULL, getpagesize(), PROT_READ|PROT_WRITE,
> +		 MAP_SHARED|MAP_ANONYMOUS, -1, 0);

This is never unmapped and should be done in the test setup instead.

> +	tst_res(TINFO,"done\n");

This is useless and also includes one more newline.

> +	tst_res(TINFO,"Mapping %ld/%ld pages.. ", totpages-1, totpages);
> +	
> +	p = SAFE_MMAP(NULL, (totpages-1)*hpage_size, PROT_READ|PROT_WRITE,
> +		 MAP_SHARED, fd, 0);
> +
> +	/* Allocate all save one of the pages up front */
> +	tst_res(TINFO,"instantiating.. ");
> +	for (i = 0; i < (totpages - 1); i++)
> +		memset(p + (i * hpage_size), 0, sizeof(int));
> +
> +	tst_res(TINFO,"done\n");

Here as well.

> +	run_race(q, race_type);
> +	tst_res(TPASS,"Test passed..!!");

This is useless, since we print the TPASS unconditionally.

> +	if (fd >= 0)

This is useless, as long as we got here the fd is >= 0.

> +		SAFE_CLOSE(fd);
> +}
> +
> +void setup(void)

Should be static.

> +{
> +	totpages = SAFE_READ_MEMINFO(MEMINFO_HPAGE_FREE);
> +	hpage_size = tst_get_hugepage_size();
> +	if (str_op)
> +	{	
> +		if (strcmp(str_op, "shared") == 0) {
> +			race_type = MAP_SHARED;
> +			} else if (strcmp(str_op, "private") == 0) {
> +				race_type = MAP_PRIVATE;
> +				} else {
> +					tst_res(TFAIL,"Usage: alloc-instantiate-race <private|shared>");
> +		}
> +	}
> +	else{
> +		tst_res(TFAIL,"Usage: alloc-instantiate-race <private|shared>");
> +	}	

The indentation here is broken here.

> +}
> +
> +void cleanup(void)

Here as well, static.

> +{
> +	if (child1)
> +		SAFE_KILL(child1, SIGKILL);
> +	if (child2)
> +		SAFE_KILL(child2, SIGKILL);
> +}
> +
> +
> +static struct tst_test test = {
> +	.options = (struct  tst_option[]){
> +		{"m:", &str_op, "Usage: alloc-instantiate-race <private|shared>"},
                                             ^
					     This is also wrong.
> +		{}
> +	},
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.needs_hugetlbfs = 1,
> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {2, TST_NEEDS},
> +	.forks_child = 1,
> +};
> -- 
> 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