Messages in this thread | | | Subject | Re: [PATCH v3] Perf Bench: Locking Microbenchmark | From | Tuan Bui <> | Date | Wed, 17 Dec 2014 14:28:50 -0800 |
| |
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
| |