[LTP] [PATCH 2/2] pthread_create-14-1: avoid threads sharing stack

[Jan Stancek]

----- Original Message -----
> Hi!
> > +/*
> > + * Copyright (c) 2004, Bull S.A..  All rights reserved.
> > + * Copyright (c) 2017, Linux Test Project
> > + * Created by: Sebastien Decugis
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it would be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write the Free Software Foundation, Inc.,
> > + *
> > + * This sample test aims to check the following assertion:
> > + * The function does not return EINTR
> > + *
> > + * The steps are:
> > + * -> continuously send SIGUSR1 to a thread which runs pthread_create()
> > + * -> check that EINTR is never returned
> > + */
> > +
> > +/* We are testing conformance to IEEE Std 1003.1, 2003 Edition */
> > +#define _POSIX_C_SOURCE 200112L
> > +
> > +/* Some routines are part of the XSI Extensions */
> > +#ifndef WITHOUT_XOPEN
> > +#define _XOPEN_SOURCE	600
> > +#endif
> > +
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <pthread.h>
> > +#include <semaphore.h>
> > +#include <signal.h>
> > +#include <sys/wait.h>
> > +#include <sys/time.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +
> > +#include "../testfrmw/testfrmw.h"
> > +#include "../testfrmw/testfrmw.c"
> > +#include "../testfrmw/threads_scenarii.c"
> > +#include "safe_helpers.h"
> > +
> > +#define RUN_TIME_USEC (2*1000*1000)
> > +#define SIGNALS_WITHOUT_DELAY 100
> > +
> > +/* total number of signals sent */
> > +static unsigned long count_sig;
> > +/* sleep [us] in between signals */
> > +static unsigned long sleep_time;
>           ^
> Shouldn't this be volatile too?
> 
> We reset it from the test() thread while it's used in a loop in the
> sendsig() thread.

That's true.

> 
> > +/* signal thread active flag */
> > +static volatile int sendsig_active;
> > +/* number of pthread_create scenarios tested */
> > +static unsigned long count_ope;
> > +
> > +static unsigned long current_time_usec(void)
> > +{
> > +	struct timeval now;
> > +
> > +	SAFE_FUNC(gettimeofday(&now, NULL));
> > +	return now.tv_sec * 1000000 + now.tv_usec;
> > +}
> > +
> > +/* the following function keeps sending signal to the process */
> > +static void *sendsig(void *arg)
> > +{
> > +	static sigset_t usersigs;
> > +
> > +	(void)arg;
> > +	pid_t process = getpid();
> > +
> > +	/* block the signal SIGUSR1 for this THREAD */
> > +	SAFE_FUNC(sigemptyset(&usersigs));
> > +	SAFE_FUNC(sigaddset(&usersigs, SIGUSR1));
> > +	SAFE_PFUNC(pthread_sigmask(SIG_BLOCK, &usersigs, NULL));
> > +
> > +	while (sendsig_active) {
> > +		if (!sendsig_active)
> > +			break;
> 
> Why do we have the condition for sendsig_active twice here?

By mistake. I forgot to drop it when I dropped semaphore that synchronized
sending/handling of signal.

> 
> > +		/*
> > +		 * Keep increasing sleeptime to make sure we progress
> > +		 * allow SIGNALS_WITHOUT_DELAY signals without any pause,
> > +		 * then start increasing sleep_time to make sure all threads
> > +		 * can progress.
> > +		 */
> > +		sleep_time++;
> > +		if (sleep_time / SIGNALS_WITHOUT_DELAY > 0)
> > +			usleep(sleep_time / SIGNALS_WITHOUT_DELAY);
> > +
> > +		count_sig++;
> > +		SAFE_FUNC(kill(process, SIGUSR1));
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static void sighdl1(int sig)
> > +{
> > +	(void)sig;
> > +}
> > +
> > +static void *threaded(void *arg)
> > +{
> > +	int ret;
> > +
> > +	/* Signal we're done (especially in case of a detached thread) */
> > +	do {
> > +		ret = sem_post(&scenarii[sc].sem);
> > +	} while ((ret == -1) && (errno == EINTR));
> > +
> > +	if (ret == -1)
> > +		UNRESOLVED(errno, "Failed to wait for the semaphore");
> > +
> > +	return arg;
> > +}
> > +
> > +/* create threads and check that EINTR is never returned */
> > +static void test(void)
> > +{
> > +	int ret = 0;
> > +	pthread_t child;
> > +	pthread_t th_sig1;
> > +	struct sigaction sa;
> > +
> > +	sigemptyset(&sa.sa_mask);
> > +	sa.sa_flags = 0;
> > +	sa.sa_handler = sighdl1;
> > +	SAFE_FUNC(sigaction(SIGUSR1, &sa, NULL));
> > +
> > +	sendsig_active = 1;
> > +	SAFE_PFUNC(pthread_create(&th_sig1, NULL, sendsig, NULL));
> > +
> > +	for (sc = 0; sc < NSCENAR; sc++) {
> > +		/* reset sleep time for signal thread */
> > +		sleep_time = 0;
> > +
> > +		ret = pthread_create(&child, &scenarii[sc].ta, threaded, NULL);
> > +		if (ret == EINTR)
> > +			FAILED("pthread_create returned EINTR");
> > +
> > +		switch (scenarii[sc].result) {
> > +		case 0:	/* Operation was expected to succeed */
> > +			if (ret != 0)
> > +				UNRESOLVED(ret, "Failed to create this thread");
> > +			break;
> > +		case 1:	/* Operation was expected to fail */
> > +			if (ret == 0) {
> > +				UNRESOLVED(-1, "An error was expected but"
> > +					" the thread creation succeeded");
> > +			}
> > +			break;
> > +		case 2:	/* We did not know the expected result */
> > +		default:
> > +			break;
> > +		}
> > +
> > +		if (ret != 0)
> > +			continue;
> > +
> > +		/* The new thread is running */
> > +		/* Just wait for the thread to terminate */
> > +		do {
> > +			ret = sem_wait(&scenarii[sc].sem);
> > +		} while ((ret == -1) && (errno == EINTR));
> > +		if (ret == -1)
> > +			UNRESOLVED(errno, "Failed to wait for the semaphore");
> > +		if (scenarii[sc].detached == 0)
> > +			SAFE_PFUNC(pthread_join(child, NULL));
> > +	}
> > +	sendsig_active = 0;
> 
> We may possibly simplify the whole thread that sends the signals by
> letting it run in an infinite loop, it will be reaped by the exit() int
> the main_loop() anyway.
> 
> Or does disabling it here make the test faster (do we get more
> iterations)?

Probably not by much. I'll compare both ways. If it's minimal difference
I'll leave it as infinite loop.

> 
> > +	SAFE_PFUNC(pthread_join(th_sig1, NULL));
> > +}
> > +
> > +static void main_loop(void)
> > +{
> > +	int child_count = 0;
> > +	int ret;
> > +	int status;
> > +	int stat_pipe[2];
> > +	pid_t child;
> > +	unsigned long usec_start, usec;
> > +	unsigned long child_count_sig;
> > +
> > +	usec_start = current_time_usec();
> > +	do {
> > +		fflush(stdout);
> > +		SAFE_FUNC(pipe(stat_pipe));
> > +		child = fork();
> 
> We should handle the fork() failures here as well, even if it's quite
> unlikely to happen.

Agreed.

> 
> > +		if (child == 0) {
> > +			count_sig = 0;
> > +			close(stat_pipe[0]);
> > +			test();
> > +			SAFE_FUNC(write(stat_pipe[1], &count_sig,
> > +				sizeof(count_sig)));
> > +			close(stat_pipe[1]);
> > +			exit(0);
> > +		}
> > +		close(stat_pipe[1]);
> > +		SAFE_FUNC(read(stat_pipe[0], &child_count_sig,
> > +			sizeof(count_sig)));
> > +		close(stat_pipe[0]);
> > +		count_sig += child_count_sig;
> > +
> > +		ret = waitpid(child, &status, 0);
> > +		if (ret != child)
> > +			UNRESOLVED(errno, "Waitpid returned the wrong PID");
> > +		if (!WIFEXITED(status)) {
> > +			output("status: %d\n", status);
> > +			FAILED("Child exited abnormally");
> > +		}
> > +		if (WEXITSTATUS(status) != 0) {
> > +			output("exit status: %d\n", WEXITSTATUS(status));
> > +			FAILED("An error occurred in child");
> > +		}
> > +
> > +		child_count++;
> > +		count_ope += NSCENAR;
> > +		usec = current_time_usec();
> > +	} while ((usec - usec_start) < RUN_TIME_USEC);
> > +
> > +	output("Test spawned %d child processes.\n", child_count);
> > +	output("Test finished after %lu usec.\n", usec - usec_start);
> > +}
> > +
> > +int main(void)
> > +{
> > +	output_init();
> > +	scenar_init();
> > +	main_loop();
> > +	scenar_fini();
> > +
> > +	output("Test executed successfully.\n");
> > +	output("  %d thread creations.\n", count_ope);
> > +	output("  %d signals were sent meanwhile.\n", count_sig);
> > +	PASSED;
> > +}
> 
> Other than that this looks good to me.

Thanks for having a look.

Regards,
Jan

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>