Re: [PATCH rt-tests v5 01/13] cyclictest: Move thread data to struct thread_param

[John Kacur <jkacur@redhat.com>]

On Wed, 10 Feb 2021, Daniel Wagner wrote:

> Group thread realated data such as thread ID to struct thread_param.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/cyclictest/cyclictest.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index c4b2369bee6b..7c45732c1553 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -113,6 +113,9 @@ static char *policyname(int policy);
>  
>  /* Struct to transfer parameters to the thread */
>  struct thread_param {
> +	pthread_t thread;
> +	int threadstarted;
> +	int tid;
>  	int prio;
>  	int policy;
>  	int mode;
> @@ -141,9 +144,6 @@ struct thread_stat {
>  	long *smis;
>  	long *hist_array;
>  	long *outliers;
> -	pthread_t thread;
> -	int threadstarted;
> -	int tid;
>  	long reduce;
>  	long redmax;
>  	long cycleofmax;
> @@ -530,7 +530,7 @@ static void *timerthread(void *param)
>  	interval.tv_sec = par->interval / USEC_PER_SEC;
>  	interval.tv_nsec = (par->interval % USEC_PER_SEC) * 1000;
>  
> -	stat->tid = gettid();
> +	par->tid = gettid();
>  
>  	sigemptyset(&sigset);
>  	sigaddset(&sigset, par->signal);
> @@ -539,7 +539,7 @@ static void *timerthread(void *param)
>  	if (par->mode == MODE_CYCLIC) {
>  		sigev.sigev_notify = SIGEV_THREAD_ID | SIGEV_SIGNAL;
>  		sigev.sigev_signo = par->signal;
> -		sigev.sigev_notify_thread_id = stat->tid;
> +		sigev.sigev_notify_thread_id = par->tid;
>  		timer_create(par->clock, &sigev, &timer);
>  		tspec.it_interval = interval;
>  	}
> @@ -613,7 +613,7 @@ static void *timerthread(void *param)
>  		setitimer(ITIMER_REAL, &itimer, NULL);
>  	}
>  
> -	stat->threadstarted++;
> +	par->threadstarted++;
>  
>  	while (!shutdown) {
>  
> @@ -719,7 +719,7 @@ static void *timerthread(void *param)
>  			shutdown++;
>  			pthread_mutex_lock(&break_thread_id_lock);
>  			if (break_thread_id == 0) {
> -				break_thread_id = stat->tid;
> +				break_thread_id = par->tid;
>  				tracemark("hit latency threshold (%llu > %d)",
>  					  (unsigned long long) diff, tracelimit);
>  				break_thread_value = diff;
> @@ -795,7 +795,7 @@ static void *timerthread(void *param)
>  	/* switch to normal */
>  	schedp.sched_priority = 0;
>  	sched_setscheduler(0, SCHED_OTHER, &schedp);
> -	stat->threadstarted = -1;
> +	par->threadstarted = -1;
>  
>  	return NULL;
>  }
> @@ -1293,7 +1293,7 @@ static void print_tids(struct thread_param *par[], int nthreads)
>  
>  	printf("# Thread Ids:");
>  	for (i = 0; i < nthreads; i++)
> -		printf(" %05d", par[i]->stats->tid);
> +		printf(" %05d", par[i]->tid);
>  	printf("\n");
>  }
>  
> @@ -1407,7 +1407,7 @@ static void print_stat(FILE *fp, struct thread_param *par, int index, int verbos
>  				fmt = "T:%2d (%5d) P:%2d I:%ld C:%7lu "
>  				        "Min:%7ld Act:%5ld Avg:%5ld Max:%8ld";
>  
> -			fprintf(fp, fmt, index, stat->tid, par->prio,
> +			fprintf(fp, fmt, index, par->tid, par->prio,
>  				par->interval, stat->cycles, stat->min,
>  				stat->act, stat->cycles ?
>  				(long)(stat->avg/stat->cycles) : 0, stat->max);
> @@ -1463,7 +1463,7 @@ static void rstat_print_stat(struct thread_param *par, int index, int verbose, i
>  				fmt = "T:%2d (%5d) P:%2d I:%ld C:%7lu "
>  				        "Min:%7ld Act:%5ld Avg:%5ld Max:%8ld";
>  
> -			dprintf(fd, fmt, index, stat->tid, par->prio,
> +			dprintf(fd, fmt, index, par->tid, par->prio,
>  				par->interval, stat->cycles, stat->min,
>  				stat->act, stat->cycles ?
>  				(long)(stat->avg/stat->cycles) : 0, stat->max);
> @@ -1966,9 +1966,9 @@ int main(int argc, char **argv)
>  		stat->min = 1000000;
>  		stat->max = 0;
>  		stat->avg = 0.0;
> -		stat->threadstarted = 1;
>  		stat->smi_count = 0;
> -		status = pthread_create(&stat->thread, &attr, timerthread, par);
> +		par->threadstarted = 1;
> +		status = pthread_create(&par->thread, &attr, timerthread, par);
>  		if (status)
>  			fatal("failed to create thread %d: %s\n", i, strerror(status));
>  
> @@ -2038,10 +2038,10 @@ int main(int argc, char **argv)
>  	if (quiet)
>  		quiet = 2;
>  	for (i = 0; i < num_threads; i++) {
> -		if (statistics[i]->threadstarted > 0)
> -			pthread_kill(statistics[i]->thread, SIGTERM);
> -		if (statistics[i]->threadstarted) {
> -			pthread_join(statistics[i]->thread, NULL);
> +		if (parameters[i]->threadstarted > 0)
> +			pthread_kill(parameters[i]->thread, SIGTERM);
> +		if (parameters[i]->threadstarted) {
> +			pthread_join(parameters[i]->thread, NULL);
>  			if (quiet && !histogram)
>  				print_stat(stdout, parameters[i], i, 0, 0);
>  		}
> -- 
> 2.30.0
> 
> 

Why? I don't see any advantage to this, and according to the comments at 
the top of the struct, thread_param is to transfer params to a thread
and thread_stat is for statistics. This is unnecessary churn, unless
you can convince me otherwise. I was worried that your JSON changes
would rely on this being changed, but as far as I can see, they do not!