[PATCH] fio: consolidate rand_seed to uint64_t

[PATCH] fio: consolidate rand_seed to uint64_t

[Grant Grundler]

csscope showed rand_seed was defined 4 times - each a different type.
While they are used differently, the places where they overlap
should reference the same type. This patch makes three of them the same
as suggested by Jens Axboe.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 ioengine.h       | 2 +-
 thread_options.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ioengine.h b/ioengine.h
index 949af91..0756bc7 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -56,7 +56,7 @@ struct io_u {
 	/*
 	 * Initial seed for generating the buffer contents
 	 */
-	unsigned long rand_seed;
+	uint64_t rand_seed;
 
 	/*
 	 * IO engine state, may be different from above when we get
diff --git a/thread_options.h b/thread_options.h
index 2f807cd..59ddd7e 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -324,7 +324,7 @@ struct thread_options_pack {
 	uint32_t do_disk_util;
 	uint32_t override_sync;
 	uint32_t rand_repeatable;
-	uint32_t rand_seed;
+	uint64_t rand_seed;
 	uint32_t use_os_rand;
 	uint32_t log_avg_msec;
 	uint32_t norandommap;
-- 
1.8.5.3

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Jens Axboe]

On Tue, Jan 28 2014, Grant Grundler wrote:
> csscope showed rand_seed was defined 4 times - each a different type.
> While they are used differently, the places where they overlap
> should reference the same type. This patch makes three of them the same
> as suggested by Jens Axboe.

Time for public shaming, as you did not even compile this patch! I'll
let you figure out the issue :-)

-- 
Jens Axboe

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Grant Grundler]

On Tue, Jan 28, 2014 at 1:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On Tue, Jan 28 2014, Grant Grundler wrote:
>> csscope showed rand_seed was defined 4 times - each a different type.
>> While they are used differently, the places where they overlap
>> should reference the same type. This patch makes three of them the same
>> as suggested by Jens Axboe.
>
> Time for public shaming, as you did not even compile this patch!

No, I didn't. :)  I told you I didn't test it.

> I'll let you figure out the issue :-)

Oh man...harsh master :P

-       o->rand_seed = le32_to_cpu(top->rand_seed);
+       o->rand_seed = le64_to_cpu(top->rand_seed);

I saw this when reviewing the code but didn't realize "top" was the
packed version that got changed.

Resending V2

Compiled and lightly tested on x86, not ARM where I'm doing most of my
testing these days. :)

cheers,
grant

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Jens Axboe]

On Tue, Jan 28 2014, Grant Grundler wrote:
> On Tue, Jan 28, 2014 at 1:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > On Tue, Jan 28 2014, Grant Grundler wrote:
> >> csscope showed rand_seed was defined 4 times - each a different type.
> >> While they are used differently, the places where they overlap
> >> should reference the same type. This patch makes three of them the same
> >> as suggested by Jens Axboe.
> >
> > Time for public shaming, as you did not even compile this patch!
> 
> No, I didn't. :)  I told you I didn't test it.

When you said no testing, I'm assuming no runtime testing. I figure a
compile would at least be in the cards :-)

> > I'll let you figure out the issue :-)
> 
> Oh man...harsh master :P
> 
> -       o->rand_seed = le32_to_cpu(top->rand_seed);
> +       o->rand_seed = le64_to_cpu(top->rand_seed);
> 
> I saw this when reviewing the code but didn't realize "top" was the
> packed version that got changed.

Yeah, and vice versa for top -> o.

> Resending V2
> 
> Compiled and lightly tested on x86, not ARM where I'm doing most of my
> testing these days. :)

It should work fine, as long as the cconv.c issue is sorted out.

-- 
Jens Axboe

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Grant Grundler]

On Tue, Jan 28, 2014 at 2:03 PM, Jens Axboe <axboe@kernel.dk> wrote:
...
>> -       o->rand_seed = le32_to_cpu(top->rand_seed);
>> +       o->rand_seed = le64_to_cpu(top->rand_seed);
>>
>> I saw this when reviewing the code but didn't realize "top" was the
>> packed version that got changed.
>
> Yeah, and vice versa for top -> o.

Oh! :(

(And I assume you mean line 288 as shown below)

If I add this:

diff --git a/cconv.c b/cconv.c
index c4941ba..57996fb 100644
--- a/cconv.c
+++ b/cconv.c
@@ -285,7 +285,7 @@ void convert_thread_options_to_net(struct thread_options_pac
        top->do_disk_util = cpu_to_le32(o->do_disk_util);
        top->override_sync = cpu_to_le32(o->override_sync);
        top->rand_repeatable = cpu_to_le32(o->rand_repeatable);
-       top->rand_seed = cpu_to_le32(o->rand_seed);
+       top->rand_seed = cpu_to_le64(o->rand_seed);
        top->use_os_rand = cpu_to_le32(o->use_os_rand);
        top->log_avg_msec = cpu_to_le32(o->log_avg_msec);
        top->norandommap = cpu_to_le32(o->norandommap);

I get this warning:

grundler <2090>make V=1
gcc -o cconv.o -std=gnu99 -Wwrite-strings -Wall
-Wdeclaration-after-statement -O3 -g -ffast-math  -D_GNU_SOURCE
-include config-host.h -DBITS_PER_LONG=64
-DFIO_VERSION='"fio-2.1.4-26-g3a2a"' -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -DFIO_INC_DEBUG -c cconv.c

    CC cconv.o
cconv.c: In function ‘convert_thread_options_to_net’:
cconv.c:288:19: warning: initialization from incompatible pointer type
[enabled by default]
    LINK fio

grundler <2091>file cconv.o
cconv.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

286         top->override_sync = cpu_to_le32(o->override_sync);
287         top->rand_repeatable = cpu_to_le32(o->rand_repeatable);
288         top->rand_seed = cpu_to_le64(o->rand_seed);
289         top->use_os_rand = cpu_to_le32(o->use_os_rand);
290         top->log_avg_msec = cpu_to_le32(o->log_avg_msec);

Now I'm just confused.

grant

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Jens Axboe]

On Tue, Jan 28 2014, Grant Grundler wrote:
> On Tue, Jan 28, 2014 at 2:03 PM, Jens Axboe <axboe@kernel.dk> wrote:
> ...
> >> -       o->rand_seed = le32_to_cpu(top->rand_seed);
> >> +       o->rand_seed = le64_to_cpu(top->rand_seed);
> >>
> >> I saw this when reviewing the code but didn't realize "top" was the
> >> packed version that got changed.
> >
> > Yeah, and vice versa for top -> o.
> 
> Oh! :(
> 
> (And I assume you mean line 288 as shown below)
> 
> If I add this:
> 
> diff --git a/cconv.c b/cconv.c
> index c4941ba..57996fb 100644
> --- a/cconv.c
> +++ b/cconv.c
> @@ -285,7 +285,7 @@ void convert_thread_options_to_net(struct thread_options_pac
>         top->do_disk_util = cpu_to_le32(o->do_disk_util);
>         top->override_sync = cpu_to_le32(o->override_sync);
>         top->rand_repeatable = cpu_to_le32(o->rand_repeatable);
> -       top->rand_seed = cpu_to_le32(o->rand_seed);
> +       top->rand_seed = cpu_to_le64(o->rand_seed);
>         top->use_os_rand = cpu_to_le32(o->use_os_rand);
>         top->log_avg_msec = cpu_to_le32(o->log_avg_msec);
>         top->norandommap = cpu_to_le32(o->norandommap);
> 
> I get this warning:
> 
> grundler <2090>make V=1
> gcc -o cconv.o -std=gnu99 -Wwrite-strings -Wall
> -Wdeclaration-after-statement -O3 -g -ffast-math  -D_GNU_SOURCE
> -include config-host.h -DBITS_PER_LONG=64
> -DFIO_VERSION='"fio-2.1.4-26-g3a2a"' -D_LARGEFILE_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -DFIO_INC_DEBUG -c cconv.c
> 
>     CC cconv.o
> cconv.c: In function ‘convert_thread_options_to_net’:
> cconv.c:288:19: warning: initialization from incompatible pointer type
> [enabled by default]
>     LINK fio
> 
> grundler <2091>file cconv.o
> cconv.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped
> 
> 286         top->override_sync = cpu_to_le32(o->override_sync);
> 287         top->rand_repeatable = cpu_to_le32(o->rand_repeatable);
> 288         top->rand_seed = cpu_to_le64(o->rand_seed);
> 289         top->use_os_rand = cpu_to_le32(o->use_os_rand);
> 290         top->log_avg_msec = cpu_to_le32(o->log_avg_msec);
> 
> Now I'm just confused.

It's because the parser is using non uintxx_t types, so you can't really
please it here. There's __cpu_to_le64() to avoid this crap. But ideally,
the parser just needs changing so that anything that is now unsigned int
is uint32_t and unsigned long long needs to be uint64_t.

If you look at your committed patch, I made those changes for you.

-- 
Jens Axboe

Re: [PATCH] fio: consolidate rand_seed to uint64_t

[Grant Grundler]

On Tue, Jan 28, 2014 at 2:17 PM, Jens Axboe <axboe@kernel.dk> wrote:
...
> It's because the parser is using non uintxx_t types, so you can't really
> please it here. There's __cpu_to_le64() to avoid this crap. But ideally,
> the parser just needs changing so that anything that is now unsigned int
> is uint32_t and unsigned long long needs to be uint64_t.

Got it.

> If you look at your committed patch, I made those changes for you.

Thanks! LGTM.

cheers
grant