SHA* digest function speed test.

SHA* digest function speed test.

BUG=1210
TEST=This is a test,

[gauravsh, 2010-02-12 03:40:35]

Once I get an LGTM on this, I will add this to the Vboot crypto autotest.

[petkov, 2010-02-12 08:17:30]

http://codereview.chromium.org/596079/diff/1001/2001
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/596079/diff/1001/2001#newcode10
src/platform/vboot_reference/tests/Makefile:10: #SRCS = timer_utils.c
Remove

http://codereview.chromium.org/596079/diff/1001/2001#newcode11
src/platform/vboot_reference/tests/Makefile:11: #OBJS = $(SRCS:.c=.o)
Remove

http://codereview.chromium.org/596079/diff/1001/2001#newcode19
src/platform/vboot_reference/tests/Makefile:19: sha_benchmark:	sha_benchmark.c
timer_utils.o
Strange mix of *.c and *.o

http://codereview.chromium.org/596079/diff/1001/2001#newcode20
src/platform/vboot_reference/tests/Makefile:20: $(CC) $(CFLAGS) -DNDEBUG
$(INCLUDES) $< -o $@ $(LIBS) timer_utils.o
You can replace "$< timer_utils.o" with "$^"

http://codereview.chromium.org/596079/diff/1001/2001#newcode26
src/platform/vboot_reference/tests/Makefile:26: rm -f sha_tests sha_benchmark
$(OBJS)
OBJS is commented out -- so you're not cleaning up timer_utils.o.

http://codereview.chromium.org/596079/diff/1001/2002
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/1001/2002#newcode14
src/platform/vboot_reference/tests/sha_benchmark.c:14: #define TEST_BUFFER_SIZE
4000000  /* 4 Mbytes. */
The comment is probably unnecessary and a bit wrong -- it's 4 million bytes
really.

http://codereview.chromium.org/596079/diff/1001/2002#newcode45
src/platform/vboot_reference/tests/sha_benchmark.c:45: speed =
(((float)TEST_BUFFER_SIZE / 10e6)
I'm not sure you really need the (float) typecast here and below given the 10e
literals.

http://codereview.chromium.org/596079/diff/1001/2002#newcode46
src/platform/vboot_reference/tests/sha_benchmark.c:46: / ((float)msecs / 10e3));
/* Mbytes/sec */
Mbytes/sec here, Mbytes/msec below.

http://codereview.chromium.org/596079/diff/1001/2002#newcode48
src/platform/vboot_reference/tests/sha_benchmark.c:48: fprintf(stderr, "%s Time
taken = %o ms, Speed = %f Mbytes/msec\n",
stderr?
%o is octal, no? why?
Mbytes/msec is a strange unit -- maybe Gbytes/sec

http://codereview.chromium.org/596079/diff/1001/2003
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/1001/2003#newcode17
src/platform/vboot_reference/tests/timer_utils.c:17: uint32_t start =
((uint32_t) ct->start_time.tv_sec * 1000000000 +
This easily overflows a 4-byte int.

[gauravsh, 2010-02-12 08:46:16]

http://codereview.chromium.org/596079/diff/1001/2001
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/596079/diff/1001/2001#newcode10
src/platform/vboot_reference/tests/Makefile:10: #SRCS = timer_utils.c
On 2010/02/12 08:17:30, petkov wrote:
> Remove
Done.

http://codereview.chromium.org/596079/diff/1001/2001#newcode11
src/platform/vboot_reference/tests/Makefile:11: #OBJS = $(SRCS:.c=.o)
On 2010/02/12 08:17:30, petkov wrote:
> Remove
Done.

http://codereview.chromium.org/596079/diff/1001/2001#newcode19
src/platform/vboot_reference/tests/Makefile:19: sha_benchmark:	sha_benchmark.c
timer_utils.o
On 2010/02/12 08:17:30, petkov wrote:
> Strange mix of *.c and *.o
Fixed, changed it to timer_utils.c and used $^ as per your comment below.

http://codereview.chromium.org/596079/diff/1001/2001#newcode20
src/platform/vboot_reference/tests/Makefile:20: $(CC) $(CFLAGS) -DNDEBUG
$(INCLUDES) $< -o $@ $(LIBS) timer_utils.o
On 2010/02/12 08:17:30, petkov wrote:
> You can replace "$< timer_utils.o" with "$^"
> 
done.

http://codereview.chromium.org/596079/diff/1001/2001#newcode26
src/platform/vboot_reference/tests/Makefile:26: rm -f sha_tests sha_benchmark
$(OBJS)
On 2010/02/12 08:17:30, petkov wrote:
> OBJS is commented out -- so you're not cleaning up timer_utils.o.
> 
Done, as per the change above.

http://codereview.chromium.org/596079/diff/1001/2002
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/1001/2002#newcode14
src/platform/vboot_reference/tests/sha_benchmark.c:14: #define TEST_BUFFER_SIZE
4000000  /* 4 Mbytes. */
On 2010/02/12 08:17:30, petkov wrote:
> The comment is probably unnecessary and a bit wrong -- it's 4 million bytes
> really.
> 

Removed the comment. (MB and Mb - confusing!)

http://codereview.chromium.org/596079/diff/1001/2002#newcode45
src/platform/vboot_reference/tests/sha_benchmark.c:45: speed =
(((float)TEST_BUFFER_SIZE / 10e6)
On 2010/02/12 08:17:30, petkov wrote:
> I'm not sure you really need the (float) typecast here and below given the 10e
> literals.
Done.

http://codereview.chromium.org/596079/diff/1001/2002#newcode46
src/platform/vboot_reference/tests/sha_benchmark.c:46: / ((float)msecs / 10e3));
/* Mbytes/sec */
On 2010/02/12 08:17:30, petkov wrote:
> Mbytes/sec here, Mbytes/msec below.
> 
Fixed. (Mybtes/sec)

http://codereview.chromium.org/596079/diff/1001/2002#newcode48
src/platform/vboot_reference/tests/sha_benchmark.c:48: fprintf(stderr, "%s Time
taken = %o ms, Speed = %f Mbytes/msec\n",
On 2010/02/12 08:17:30, petkov wrote:
> stderr?
> %o is octal, no? why?
> Mbytes/msec is a strange unit -- maybe Gbytes/sec
> 
Changed to Mbytes/sec. I meant %u. Funny, for some reason I thought %o was the
unsigned int format specifier.

http://codereview.chromium.org/596079/diff/1001/2003
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/1001/2003#newcode17
src/platform/vboot_reference/tests/timer_utils.c:17: uint32_t start =
((uint32_t) ct->start_time.tv_sec * 1000000000 +
On 2010/02/12 08:17:30, petkov wrote:
> This easily overflows a 4-byte int.

Changed to uint64_t. Is there a way to specify platform independent format
specifier to printf for uint64_t. Currently, I am not using %ld.

[petkov, 2010-02-12 15:51:15]

http://codereview.chromium.org/596079/diff/1001/2002
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/1001/2002#newcode14
src/platform/vboot_reference/tests/sha_benchmark.c:14: #define TEST_BUFFER_SIZE
4000000  /* 4 Mbytes. */
On 2010/02/12 08:46:16, gauravsh wrote:
> On 2010/02/12 08:17:30, petkov wrote:
> > The comment is probably unnecessary and a bit wrong -- it's 4 million bytes
> > really.
> > 
> 
> Removed the comment. (MB and Mb - confusing!)

You also reduced it to 400,000 -- I assume intentionally.

http://codereview.chromium.org/596079/diff/1001/2003
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/1001/2003#newcode17
src/platform/vboot_reference/tests/timer_utils.c:17: uint32_t start =
((uint32_t) ct->start_time.tv_sec * 1000000000 +
On 2010/02/12 08:46:16, gauravsh wrote:
> On 2010/02/12 08:17:30, petkov wrote:
> > This easily overflows a 4-byte int.
> 
> Changed to uint64_t. Is there a way to specify platform independent format
> specifier to printf for uint64_t. Currently, I am not using %ld. 

You probably meant to use %llu. But go back to uint32_t for the duration and
stick with %u.

http://codereview.chromium.org/596079/diff/10/11
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/596079/diff/10/11#newcode20
src/platform/vboot_reference/tests/Makefile:20: .c.o:
You don't need this rule now, do you?

http://codereview.chromium.org/596079/diff/10/12
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/10/12#newcode32
src/platform/vboot_reference/tests/sha_benchmark.c:32: uint64_t msecs;
4 billion mseconds is quite a few days. You're probably OK with a uint32_t here.

http://codereview.chromium.org/596079/diff/10/12#newcode45
src/platform/vboot_reference/tests/sha_benchmark.c:45: speed =
(((double)TEST_BUFFER_SIZE / 10e6)
I think 10e6 is double already -- so maybe no need for the typecast.

http://codereview.chromium.org/596079/diff/10/12#newcode48
src/platform/vboot_reference/tests/sha_benchmark.c:48: fprintf(stderr, "%s Time
taken = %ld ms, Speed = %f Mbytes/sec\n",
Per comment above, maybe just use %u and uint32_t.

http://codereview.chromium.org/596079/diff/10/13
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/10/13#newcode21
src/platform/vboot_reference/tests/timer_utils.c:21: return (end - start) /
1000000ULL; /* Nanoseconds -> Milliseconds. */
Convert to uint32_t here after scaling down?

http://codereview.chromium.org/596079/diff/10/14
File src/platform/vboot_reference/tests/timer_utils.h (right):

http://codereview.chromium.org/596079/diff/10/14#newcode24
src/platform/vboot_reference/tests/timer_utils.h:24: uint64_t
GetDurationMsecs(ClockTimerState* ct);
The duration in msecs is probably OK to be uint32_t.

[gauravsh, 2010-02-12 19:05:08]

http://codereview.chromium.org/596079/diff/1001/2002
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/1001/2002#newcode14
src/platform/vboot_reference/tests/sha_benchmark.c:14: #define TEST_BUFFER_SIZE
4000000  /* 4 Mbytes. */
On 2010/02/12 15:51:15, petkov wrote:
> On 2010/02/12 08:46:16, gauravsh wrote:
> > On 2010/02/12 08:17:30, petkov wrote:
> > > The comment is probably unnecessary and a bit wrong -- it's 4 million
bytes
> > > really.
> > > 
> > 
> > Removed the comment. (MB and Mb - confusing!)
> 
> You also reduced it to 400,000 -- I assume intentionally.
Uh, not intentionally. Thanks for catching that.

http://codereview.chromium.org/596079/diff/10/11
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/596079/diff/10/11#newcode20
src/platform/vboot_reference/tests/Makefile:20: .c.o:
On 2010/02/12 15:51:15, petkov wrote:
> You don't need this rule now, do you?
> 

Nope. Removed.

http://codereview.chromium.org/596079/diff/10/12
File src/platform/vboot_reference/tests/sha_benchmark.c (right):

http://codereview.chromium.org/596079/diff/10/12#newcode32
src/platform/vboot_reference/tests/sha_benchmark.c:32: uint64_t msecs;
On 2010/02/12 15:51:15, petkov wrote:
> 4 billion mseconds is quite a few days. You're probably OK with a uint32_t
here.

Done.

http://codereview.chromium.org/596079/diff/10/12#newcode45
src/platform/vboot_reference/tests/sha_benchmark.c:45: speed =
(((double)TEST_BUFFER_SIZE / 10e6)
On 2010/02/12 15:51:15, petkov wrote:
> I think 10e6 is double already -- so maybe no need for the typecast.
> 
Done.

http://codereview.chromium.org/596079/diff/10/12#newcode48
src/platform/vboot_reference/tests/sha_benchmark.c:48: fprintf(stderr, "%s Time
taken = %ld ms, Speed = %f Mbytes/sec\n",
On 2010/02/12 15:51:15, petkov wrote:
> Per comment above, maybe just use %u and uint32_t.

Done.

http://codereview.chromium.org/596079/diff/10/13
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/10/13#newcode21
src/platform/vboot_reference/tests/timer_utils.c:21: return (end - start) /
1000000ULL; /* Nanoseconds -> Milliseconds. */
On 2010/02/12 15:51:15, petkov wrote:
> Convert to uint32_t here after scaling down?
> 
Done.

http://codereview.chromium.org/596079/diff/10/14
File src/platform/vboot_reference/tests/timer_utils.h (right):

http://codereview.chromium.org/596079/diff/10/14#newcode24
src/platform/vboot_reference/tests/timer_utils.h:24: uint64_t
GetDurationMsecs(ClockTimerState* ct);
On 2010/02/12 15:51:15, petkov wrote:
> The duration in msecs is probably OK to be uint32_t.

Done.

[petkov, 2010-02-12 19:14:44]

LGTM with one comment.

http://codereview.chromium.org/596079/diff/3003/3006
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/3003/3006#newcode21
src/platform/vboot_reference/tests/timer_utils.c:21: uint64_t duration_msecs =
(end - start) / 1000000U;  /* Nanoseconds ->
This is still uint64_t but the function returns uint32_t. Don't you get a
compile-time warning for the implicit type conversion? I like -Wall -Werror, in
general.

[gauravsh, 2010-02-12 19:19:46]

http://codereview.chromium.org/596079/diff/3003/3006
File src/platform/vboot_reference/tests/timer_utils.c (right):

http://codereview.chromium.org/596079/diff/3003/3006#newcode21
src/platform/vboot_reference/tests/timer_utils.c:21: uint64_t duration_msecs =
(end - start) / 1000000U;  /* Nanoseconds ->
On 2010/02/12 19:14:45, petkov wrote:
> This is still uint64_t but the function returns uint32_t. Don't you get a
> compile-time warning for the implicit type conversion? I like -Wall -Werror,
in
> general.

There's no warning because it's C. C++ would need an explicit type cast. In any
case, I have added an explicit type-cast.