Re: [PATCH v1.1] Randomize memcpy benchmark addresses.

[OndÅej BÃlka <neleai at seznam dot cz>]

On Thu, Sep 05, 2013 at 08:41:02AM +0100, Will Newton wrote:
> On 4 September 2013 17:50, OndÅej BÃlka <neleai@seznam.cz> wrote:
> >  static void
> > -do_one_test (impl_t *impl, char *dst, const char *src,
> > +do_one_test (impl_t *impl, size_t align1, size_t align2,
> >              size_t len)
> >  {
> >    size_t i, iters = INNER_LOOP_ITERS;
> >    timing_t start, stop, cur;
> > -
> > -  if (CALL (impl, dst, src, len) != MEMCPY_RESULT (dst, len))
> > +  static unsigned int r_seed = 42;
> 
> Isn't randomizing with a static seed value replacing one predictable
> sequence with another? And it replaces a sequence that is simple to
> reason about with one that is not.
>
No, predictability depends on adversary. Unless branch predictor is
sophisticated enough that in can perfectly predict branches when they follow pattern
generated by rand_r function then randomization is enough.

Also new sequence is much harder than previous one from adversaries from
ACME corp. Now he could write following and show improvement in
benchmark.

char *evil_memcpy (char *x,char *y, size_t n)
{
  if (((uintptr_t)x)%4096 < 32 || ((uintptr_t)y)%4096 < 32)
    return optimized_memcpy (x, y, n); // Needs to improve current state.
  while (n--)
    x[n]=y[n];
  return x;
}

 
> This introduces build warnings:
> 
> bench-memcpy.c: In function âdo_one_testâ:
> bench-memcpy.c:69:15: warning: pointer targets in assignment differ in
> signedness [-Wpointer-sign]
> bench-memcpy.c:72:15: warning: pointer targets in assignment differ in
> signedness [-Wpointer-sign]
> 
yes, I will add extra cast.
> > -
> > -  printf ("Length %4zd, alignment %2zd/%2zd:", len, align1, align2);
> > +  printf ("Length %4zd, aligned %c/%c:", len, align1 ? 'y' : 'n', align2 ? 'y' : 'n');
> 
> This means we no longer print what the buffer alignment is which makes
> results analysis impossible.
> 
Could you elaborate.

> >    test_init ();
> > +  buf1 = malloc (2 * SIZE + 4096);
> > +  buf2 = malloc (2 * SIZE + 4096);
> > +  buf1 = buf1 + 4096 - ((uintptr_t) buf1) % 4096;
> > +  buf2 = buf2 + 4096 - ((uintptr_t) buf1) % 4096;
> > +
> 
> What is 4096? We should avoid magic constants.
> 
> buf1 and buf2 have already been allocated in bench-string.h. We should
> make any solution work with that header and work for all string
> benchmarks, not just one at a time.
> 
Well buf2 was allocated as 

 buf2 = mmap (0, 2 * page_size ...

which is too small for this benchmark, so it needs to get changed.