linux-kernel - Re: [PATCH 2/2] dont commify big numbers by default, let -B do it

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110525124401.GB29300@elte.hu>
Date:	Wed, 25 May 2011 14:44:01 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Jim Cromie <jim.cromie@...il.com>
Cc:	linux-kernel@...r.kernel.org, a.p.zijlstra@...llo.nl,
	paulus@...ba.org, acme@...stprotocols.net
Subject: Re: [PATCH 2/2] dont commify big numbers by default, let -B do it

* Jim Cromie <jim.cromie@...il.com> wrote:

> > Also, i think for automation we'd also like to have a 'simple 
> > output' mode, would you like to add that?
> 
> OK, heres 1st cut at it, adding option --simple, for review / 
> feedback
> 
> Its based upon the csv-output code mostly, with some vestiges of 
> verbose..

Ok, we can do something like this - but please see the comments on 
the patch further below:

> > Something a bit like what you can see in 'perf stat -v true': 
> > Without the human output later on, and with elapsed time added as 
> > well.
> 
> for my part, Id like the moral equivalent of time(s)? output too, 
> though I suspect thats a separate patch..

yeah. No objection to that either: time is well known and the stddev 
output by perf stat --repeat is well liked. Wanna combine the two?

> [jimc@...ucho perf]$ time ./perf stat -x'  ' -- sh -c 'sleep 3'
> task-clock-msecs6.830999
> context-switches  2
> CPU-migrations  1
> page-faults  477
> cycles  5404163  (scaled from 72.07%)
> instructions  3302699
> branches  804750
> branch-misses  51870
> cache-references  1584532  (scaled from 45.93%)
> cache-misses  36616  (scaled from 31.36%)
> 
> real	0m3.019s
> user	0m0.005s
> sys	0m0.012s
> 
> are these timings already taken by perf-stat ?
> is it a simple matter of addition and printing ?
> If not, whats involved ?

Do you mean the real/user/sys bits? Elapsed time is already measured, 
but you'd have to extract the rusage bits to get to the stime/utime 
values.

We could also add those as explicit events.

> Also, task-clock-msec doesnt quite match up with times' user number 
> Whats going on here ?

task-clock-msec is generally much more accurate than the 0.005 
printed by 'time'.

About the patch:

> -	fprintf(logfp, "   ( +- %7.3f%% )",
> -			100 * stddev_stats(&ps->res_stats[0]) / avg);
> +	if (!csv_output)
> +		fprintf(logfp, "   ( +- %7.3f%% )",
> +				100 * stddev_stats(&ps->res_stats[0]) / avg);
> +	else
> +	  fprintf(logfp, "%s%.3f%%",
> +		  csv_sep, 100 * stddev_stats(&ps->res_stats[0]) / avg);

We *really* want something cleaner here: a print_ops vector of 
function pointers which would contain a handful of helper functions 
called by the higher level code?

That way adding a new format method would not uglify the highlevel 
code but could be done by adding a new print_ops 'driver' which would 
be selected if the right option is provided.

Also, instead of adding -s/--simple we really want a single 
print-format option that knows about all the format 'drivers':

  --print simple
  --print csv
  --print default
  --print verbose

or so.

Also, please run scripts/checkpatch.pl over your patches, the above 
is i think not matching the coding style we use in the kernel (unless 
webmail mangled your patch).

Thanks,

	Ingo
--
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/