Re: [PATCH v3] Perf Bench: Locking Microbenchmark

[Tuan Bui <tuan.d.bui@hp.com>]

On Thu, 2014-12-11 at 18:12 -0300, Arnaldo Carvalho de Melo wrote:
> > +struct timeval start, end, total;
> > +static unsigned int start_nr = 100;
> > +static unsigned int end_nr = 1100;
> > +static unsigned int increment_by = 100;
> > +static int bench_dur = NOTSET;
> > +static int num_jobs = NOTSET;
> > +static bool run_jobs;
> > +static int n_pro;
> > +
> > +/* Shared variables between fork processes*/
> > +unsigned int *finished, *setup;
> > +unsigned long long *shared_workers;
> > +char *tmp_dir;
> 
> Are you sure these variables aren't static?
> 

Yes you are right.  I will change these to static.


> > +/* Running bench vfs workload */
> > +static void *run_bench_vfs(struct worker *workers)
> > +{
> > +	int fd;
> > +	unsigned long long nr_ops = 0;
> > +	int jobs = num_jobs;
> > +
> > +	sprintf(workers->str, "%s/%d-XXXXXX", tmp_dir, getpid());
> 
> 
> Please use snprintf, checking for overflows on the target string
> 

Thank you for pointing this out.  I will change all sprintf to use
snprintf and check for overflows on the target string.


> > +
> > +	/* finished shared var is use to signal start and end of benchmark */
> > +	finished_tmp = (void *)mmap(0, sizeof(unsigned int), PROT_READ|PROT_WRITE,
> > +			MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> 
> Why do you use these void * casts before mmap alreayd returns void *?
> 

Yes the cast to void here is redundant. I will remove the cast to (void
*) here.

> > +	if (finished_tmp == (void *) -1)
> 
> Please use MAP_FAILED instead of its equivalent (void *) -1.
> 

I will check for MAP_FAILED here instead.  Thank you for the suggestion.


-Tuan