linux-kernel - Re: [PATCH v3] Perf Bench: Locking Microbenchmark

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 17 Dec 2014 14:28:50 -0800
From:	Tuan Bui <tuan.d.bui@...com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	linux-kernel@...r.kernel.org, dbueso@...e.de,
	a.p.zijlstra@...llo.nl, paulus@...ba.org, artagnon@...il.com,
	jolsa@...hat.com, dvhart@...ux.intel.com,
	Aswin Chandramouleeswaran <aswin@...com>,
	Jason Low <jason.low2@...com>, akpm@...ux-foundation.org,
	mingo@...nel.org
Subject: Re: [PATCH  v3] Perf Bench: Locking Microbenchmark

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/