valgrind spews

valgrind spews

[Jens Axboe]

Hi Bart,

After your commit:

commit 0ffccc21fcd67d1e1d2a360e90f3fe8efc0d6b52
Author: Bart Van Assche <bart.vanassche@wdc.com>
Date:   Thu Mar 8 13:41:36 2018 -0800

    Improve Valgrind instrumentation of memory allocations

running valgrind on a fio spews a lot of warnings:

==14331== Invalid write of size 8
==14331==    at 0x4C3451F: memset (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14331==    by 0x4595F8: memset (string3.h:90)
==14331==    by 0x4595F8: smalloc_pool (smalloc.c:434)
==14331==    by 0x4595F8: __smalloc (smalloc.c:452)
==14331==    by 0x4727CD: flow_init (flow.c:112)
==14331==    by 0x421886: setup_thread_area (init.c:391)
==14331==    by 0x42552A: get_new_job (init.c:468)
==14331==    by 0x42552A: __parse_jobs_ini (init.c:1928)
==14331==    by 0x42585F: parse_jobs_ini (init.c:2082)
[...]

Basically off any path that ends up in smalloc, on the memset()
that we do:

        ptr = __smalloc_pool(pool, alloc_size);
        if (ptr) {
                struct block_hdr *hdr = ptr;

                hdr->size = alloc_size;
                fill_redzone(hdr);

                ptr += sizeof(*hdr);
                memset(ptr, 0, size);
		^^^
        }

for every alloc. From configure:

Valgrind headers              yes


-- 
Jens Axboe

Re: valgrind spews

[Sitsofe Wheeler]

On 22 March 2018 at 16:14, Jens Axboe <axboe@kernel.dk> wrote:
> Hi Bart,
>
> After your commit:
>
> commit 0ffccc21fcd67d1e1d2a360e90f3fe8efc0d6b52
> Author: Bart Van Assche <bart.vanassche@wdc.com>
> Date:   Thu Mar 8 13:41:36 2018 -0800
>
>     Improve Valgrind instrumentation of memory allocations
>
> running valgrind on a fio spews a lot of warnings:
>
> ==14331== Invalid write of size 8
> ==14331==    at 0x4C3451F: memset (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==14331==    by 0x4595F8: memset (string3.h:90)
> ==14331==    by 0x4595F8: smalloc_pool (smalloc.c:434)
> ==14331==    by 0x4595F8: __smalloc (smalloc.c:452)
> ==14331==    by 0x4727CD: flow_init (flow.c:112)
> ==14331==    by 0x421886: setup_thread_area (init.c:391)
> ==14331==    by 0x42552A: get_new_job (init.c:468)
> ==14331==    by 0x42552A: __parse_jobs_ini (init.c:1928)
> ==14331==    by 0x42585F: parse_jobs_ini (init.c:2082)
> [...]
>
> Basically off any path that ends up in smalloc, on the memset()
> that we do:
>
>         ptr = __smalloc_pool(pool, alloc_size);
>         if (ptr) {
>                 struct block_hdr *hdr = ptr;
>
>                 hdr->size = alloc_size;
>                 fill_redzone(hdr);
>
>                 ptr += sizeof(*hdr);
>                 memset(ptr, 0, size);
>                 ^^^
>         }
>
> for every alloc. From configure:
>
> Valgrind headers              yes

I can't reproduce that one here with the following:

valgrind ./fio --ioengine=libaio --thread --rw=read
--filename=/tmp/fio.tmp --size=1M  --flow=2 --stonewall --name=test
--name=test2

My configure says this:

Valgrind headers              yes

Additionally after a
make clean
do you find that
git clean -xn
still finds built files to remove?

-- 
Sitsofe | http://sucs.org/~sits/

Re: valgrind spews

[Jens Axboe]

On 3/22/18 10:53 AM, Sitsofe Wheeler wrote:
> On 22 March 2018 at 16:14, Jens Axboe <axboe@kernel.dk> wrote:
>> Hi Bart,
>>
>> After your commit:
>>
>> commit 0ffccc21fcd67d1e1d2a360e90f3fe8efc0d6b52
>> Author: Bart Van Assche <bart.vanassche@wdc.com>
>> Date:   Thu Mar 8 13:41:36 2018 -0800
>>
>>     Improve Valgrind instrumentation of memory allocations
>>
>> running valgrind on a fio spews a lot of warnings:
>>
>> ==14331== Invalid write of size 8
>> ==14331==    at 0x4C3451F: memset (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==14331==    by 0x4595F8: memset (string3.h:90)
>> ==14331==    by 0x4595F8: smalloc_pool (smalloc.c:434)
>> ==14331==    by 0x4595F8: __smalloc (smalloc.c:452)
>> ==14331==    by 0x4727CD: flow_init (flow.c:112)
>> ==14331==    by 0x421886: setup_thread_area (init.c:391)
>> ==14331==    by 0x42552A: get_new_job (init.c:468)
>> ==14331==    by 0x42552A: __parse_jobs_ini (init.c:1928)
>> ==14331==    by 0x42585F: parse_jobs_ini (init.c:2082)
>> [...]
>>
>> Basically off any path that ends up in smalloc, on the memset()
>> that we do:
>>
>>         ptr = __smalloc_pool(pool, alloc_size);
>>         if (ptr) {
>>                 struct block_hdr *hdr = ptr;
>>
>>                 hdr->size = alloc_size;
>>                 fill_redzone(hdr);
>>
>>                 ptr += sizeof(*hdr);
>>                 memset(ptr, 0, size);
>>                 ^^^
>>         }
>>
>> for every alloc. From configure:
>>
>> Valgrind headers              yes
> 
> I can't reproduce that one here with the following:
> 
> valgrind ./fio --ioengine=libaio --thread --rw=read
> --filename=/tmp/fio.tmp --size=1M  --flow=2 --stonewall --name=test
> --name=test2
> 
> My configure says this:
> 
> Valgrind headers              yes
> 
> Additionally after a
> make clean
> do you find that
> git clean -xn
> still finds built files to remove?

Run it as a server:

$ valgrind ./fio --server
$ ./fio --client=localhost null.fio

$ cat null.fio
[null]
bs=4k
ioengine=null
size=1g
rw=read

The above was supposed to say "fio server", but apparently I missed
putting that in...

-- 
Jens Axboe

Re: valgrind spews

[Bart Van Assche]

On Thu, 2018-03-22 at 11:01 -0600, Jens Axboe wrote:
> Run it as a server:
> 
> $ valgrind ./fio --server
> $ ./fio --client=localhost null.fio
> 
> $ cat null.fio
> [null]
> bs=4k
> ioengine=null
> size=1g
> rw=read
> 
> The above was supposed to say "fio server", but apparently I missed
> putting that in...

If I run that test, Valgrind only reports the following on my laptop:

==17458== Conditional jump or move depends on uninitialised value(s)
==17458==    at 0x463FFD: __sk_out_drop (server.c:142)
==17458==    by 0x463FFD: handle_connection (server.c:1257)
==17458==    by 0x463FFD: accept_loop (server.c:1389)
==17458==    by 0x463FFD: fio_server (server.c:2468)
==17458==    by 0x464894: fio_start_server (server.c:2552)
==17458==    by 0x426F8E: parse_cmd_line (init.c:2880)
==17458==    by 0x427602: parse_options (init.c:2931)
==17458==    by 0x41134A: main (fio.c:42)
==17458== 
==17458== Conditional jump or move depends on uninitialised value(s)
==17458==    at 0x46400F: __sk_out_drop (server.c:146)
==17458==    by 0x46400F: handle_connection (server.c:1257)
==17458==    by 0x46400F: accept_loop (server.c:1389)
==17458==    by 0x46400F: fio_server (server.c:2468)
==17458==    by 0x464894: fio_start_server (server.c:2552)
==17458==    by 0x426F8E: parse_cmd_line (init.c:2880)
==17458==    by 0x427602: parse_options (init.c:2931)
==17458==    by 0x41134A: main (fio.c:42)

Where is the sk_out structure allocated? Can Valgrind know whether or not
that structure has been initialized?

Thanks,

Bart.

Re: valgrind spews

[Jens Axboe]

On 3/22/18 11:12 AM, Bart Van Assche wrote:
> On Thu, 2018-03-22 at 11:01 -0600, Jens Axboe wrote:
>> Run it as a server:
>>
>> $ valgrind ./fio --server
>> $ ./fio --client=localhost null.fio
>>
>> $ cat null.fio
>> [null]
>> bs=4k
>> ioengine=null
>> size=1g
>> rw=read
>>
>> The above was supposed to say "fio server", but apparently I missed
>> putting that in...
> 
> If I run that test, Valgrind only reports the following on my laptop:
> 
> ==17458== Conditional jump or move depends on uninitialised value(s)
> ==17458==    at 0x463FFD: __sk_out_drop (server.c:142)
> ==17458==    by 0x463FFD: handle_connection (server.c:1257)
> ==17458==    by 0x463FFD: accept_loop (server.c:1389)
> ==17458==    by 0x463FFD: fio_server (server.c:2468)
> ==17458==    by 0x464894: fio_start_server (server.c:2552)
> ==17458==    by 0x426F8E: parse_cmd_line (init.c:2880)
> ==17458==    by 0x427602: parse_options (init.c:2931)
> ==17458==    by 0x41134A: main (fio.c:42)
> ==17458== 
> ==17458== Conditional jump or move depends on uninitialised value(s)
> ==17458==    at 0x46400F: __sk_out_drop (server.c:146)
> ==17458==    by 0x46400F: handle_connection (server.c:1257)
> ==17458==    by 0x46400F: accept_loop (server.c:1389)
> ==17458==    by 0x46400F: fio_server (server.c:2468)
> ==17458==    by 0x464894: fio_start_server (server.c:2552)
> ==17458==    by 0x426F8E: parse_cmd_line (init.c:2880)
> ==17458==    by 0x427602: parse_options (init.c:2931)
> ==17458==    by 0x41134A: main (fio.c:42)
> 
> Where is the sk_out structure allocated? Can Valgrind know whether or not
> that structure has been initialized?

In server.c:accept_loop() and that structure is memset at alloc
time as it's coming out of smalloc().

-- 
Jens Axboe

Re: valgrind spews

[Bart Van Assche]

On Thu, 2018-03-22 at 11:23 -0600, Jens Axboe wrote:
> On 3/22/18 11:12 AM, Bart Van Assche wrote:
> > Where is the sk_out structure allocated? Can Valgrind know whether or not
> > that structure has been initialized?
> 
> In server.c:accept_loop() and that structure is memset at alloc
> time as it's coming out of smalloc().

Would you be OK with changing that smalloc() call into scalloc() such that
Valgrind knows that that structure is initialized? On my laptop the following
is sufficient to suppress the complaints mentioned in my previous e-mail:

diff --git a/server.c b/server.c
index 15dc2c4b38ba..d3f69774165f 100644
--- a/server.c
+++ b/server.c
@@ -1359,7 +1359,7 @@ static int accept_loop(int listen_sk)
 
 		dprint(FD_NET, "server: connect from %s\n", from);
 
-		sk_out = smalloc(sizeof(*sk_out));
+		sk_out = scalloc(1, sizeof(*sk_out));
 		if (!sk_out) {
 			close(sk);
 			return -1;

Thanks,

Bart.

Re: valgrind spews

[Jens Axboe]

On 3/22/18 11:27 AM, Bart Van Assche wrote:
> On Thu, 2018-03-22 at 11:23 -0600, Jens Axboe wrote:
>> On 3/22/18 11:12 AM, Bart Van Assche wrote:
>>> Where is the sk_out structure allocated? Can Valgrind know whether or not
>>> that structure has been initialized?
>>
>> In server.c:accept_loop() and that structure is memset at alloc
>> time as it's coming out of smalloc().
> 
> Would you be OK with changing that smalloc() call into scalloc() such that
> Valgrind knows that that structure is initialized? On my laptop the following
> is sufficient to suppress the complaints mentioned in my previous e-mail:

Sure, done.


-- 
Jens Axboe

Re: valgrind spews

[Sitsofe Wheeler]

On 22 March 2018 at 17:29, Jens Axboe <axboe@kernel.dk> wrote:
> On 3/22/18 11:27 AM, Bart Van Assche wrote:
>> On Thu, 2018-03-22 at 11:23 -0600, Jens Axboe wrote:
>>> On 3/22/18 11:12 AM, Bart Van Assche wrote:
>>>> Where is the sk_out structure allocated? Can Valgrind know whether or not
>>>> that structure has been initialized?
>>>
>>> In server.c:accept_loop() and that structure is memset at alloc
>>> time as it's coming out of smalloc().
>>
>> Would you be OK with changing that smalloc() call into scalloc() such that
>> Valgrind knows that that structure is initialized? On my laptop the following
>> is sufficient to suppress the complaints mentioned in my previous e-mail:
>
> Sure, done.

Even with that patch I'm still seeing problems with that job when I do this:

for i in {1..5}; do ./fio --client=localhost null.fio; done

Here's what the valgrind is saying for the server:

==2532== Warning: set address range perms: large range [0x14f90000,
0x2dce1000) (defined)
==2532== Invalid write of size 8
==2532==    at 0x4C34514: memset (vg_replace_strmem.c:1239)
==2532==    by 0x45E98A: smalloc_pool (smalloc.c:434)
==2532==    by 0x45E3D1: __smalloc (smalloc.c:452)
==2532==    by 0x45E346: smalloc (smalloc.c:478)
==2532==    by 0x485FA3: flow_init (flow.c:112)
==2532==    by 0x42CEA4: setup_thread_area (init.c:391)
==2532==    by 0x426B43: get_new_job (init.c:468)
==2532==    by 0x4273B8: __parse_jobs_ini (init.c:1928)
==2532==    by 0x426D77: parse_jobs_ini (init.c:2082)
==2532==    by 0x46FB6E: handle_job_cmd (server.c:792)
==2532==    by 0x46F756: handle_command (server.c:1000)
==2532==    by 0x46F15C: handle_connection (server.c:1242)
==2532==  Address 0x636f650 is 0 bytes inside a block of size 64 free'd
==2532==    at 0x45E151: sfree (smalloc.c:349)
==2532==    by 0x46DFE1: finish_entry (server.c:1100)
==2532==    by 0x46DE3E: handle_sk_entry (server.c:1159)
==2532==    by 0x46F640: handle_xmits (server.c:1181)
==2532==    by 0x46F02B: handle_connection (server.c:1209)
==2532==    by 0x46E92A: accept_loop (server.c:1389)
==2532==    by 0x46D17F: fio_server (server.c:2468)
==2532==    by 0x46CF9B: fio_start_server (server.c:2552)
==2532==    by 0x429081: parse_cmd_line (init.c:2876)
==2532==    by 0x42A114: parse_options (init.c:2927)
==2532==    by 0x4AABCD: main (fio.c:42)
==2532==  Block was alloc'd at
==2532==    at 0x45E43D: __smalloc (smalloc.c:456)
==2532==    by 0x45E346: smalloc (smalloc.c:478)
==2532==    by 0x46BE58: fio_net_prep_cmd (server.c:528)
==2532==    by 0x469F6A: fio_net_queue_cmd (server.c:571)
==2532==    by 0x46FFB0: handle_probe_cmd (server.c:877)
==2532==    by 0x46F778: handle_command (server.c:1006)
==2532==    by 0x46F15C: handle_connection (server.c:1242)
==2532==    by 0x46E92A: accept_loop (server.c:1389)
==2532==    by 0x46D17F: fio_server (server.c:2468)
==2532==    by 0x46CF9B: fio_start_server (server.c:2552)
==2532==    by 0x429081: parse_cmd_line (init.c:2876)
==2532==    by 0x42A114: parse_options (init.c:2927)

I also occasionally also see output like this:

==2584==
==2584== HEAP SUMMARY:
==2584==     in use at exit: 2,125,067 bytes in 36 blocks
==2584==   total heap usage: 361 allocs, 346 frees, 2,299,852 bytes allocated
==2584==
==2584== Block 0x636f4d0..0x636f61f overlaps with block 0x636f4d0..0x636f61f
==2584== Blocks allocation contexts:
==2584==    at 0x45E43D: __smalloc (smalloc.c:456)
==2584==    by 0x45E507: scalloc (smalloc.c:483)
==2584==    by 0x46E82F: accept_loop (server.c:1362)
==2584==    by 0x46D17F: fio_server (server.c:2468)
==2584==    by 0x46CF9B: fio_start_server (server.c:2552)
==2584==    by 0x429081: parse_cmd_line (init.c:2876)
==2584==    by 0x42A114: parse_options (init.c:2927)
==2584==    by 0x4AABCD: main (fio.c:42)
==2584==
==2584==    at 0x45E43D: __smalloc (smalloc.c:456)
==2584==    by 0x45E507: scalloc (smalloc.c:483)
==2584==    by 0x46E82F: accept_loop (server.c:1362)
==2584==    by 0x46D17F: fio_server (server.c:2468)
==2584==    by 0x46CF9B: fio_start_server (server.c:2552)
==2584==    by 0x429081: parse_cmd_line (init.c:2876)
==2584==    by 0x42A114: parse_options (init.c:2927)
==2584==    by 0x4AABCD: main (fio.c:42)
==2584== This is usually caused by using VALGRIND_MALLOCLIKE_BLOCK in
an inappropriate way.

Memcheck: mc_leakcheck.c:2141 (vgMemCheck_detect_memory_leaks): the
'impossible' happened.

Valgrind comes from valgrind-devel-3.13.0-12.fc26.x86_64 .

-- 
Sitsofe | http://sucs.org/~sits/

Re: valgrind spews

[Jens Axboe]

On 3/23/18 8:39 AM, Sitsofe Wheeler wrote:
> On 22 March 2018 at 17:29, Jens Axboe <axboe@kernel.dk> wrote:
>> On 3/22/18 11:27 AM, Bart Van Assche wrote:
>>> On Thu, 2018-03-22 at 11:23 -0600, Jens Axboe wrote:
>>>> On 3/22/18 11:12 AM, Bart Van Assche wrote:
>>>>> Where is the sk_out structure allocated? Can Valgrind know whether or not
>>>>> that structure has been initialized?
>>>>
>>>> In server.c:accept_loop() and that structure is memset at alloc
>>>> time as it's coming out of smalloc().
>>>
>>> Would you be OK with changing that smalloc() call into scalloc() such that
>>> Valgrind knows that that structure is initialized? On my laptop the following
>>> is sufficient to suppress the complaints mentioned in my previous e-mail:
>>
>> Sure, done.
> 
> Even with that patch I'm still seeing problems with that job when I do this:
> 
> for i in {1..5}; do ./fio --client=localhost null.fio; done
> 
> Here's what the valgrind is saying for the server:
> 
> ==2532== Warning: set address range perms: large range [0x14f90000,
> 0x2dce1000) (defined)
> ==2532== Invalid write of size 8
> ==2532==    at 0x4C34514: memset (vg_replace_strmem.c:1239)
> ==2532==    by 0x45E98A: smalloc_pool (smalloc.c:434)
> ==2532==    by 0x45E3D1: __smalloc (smalloc.c:452)
> ==2532==    by 0x45E346: smalloc (smalloc.c:478)
> ==2532==    by 0x485FA3: flow_init (flow.c:112)
> ==2532==    by 0x42CEA4: setup_thread_area (init.c:391)
> ==2532==    by 0x426B43: get_new_job (init.c:468)
> ==2532==    by 0x4273B8: __parse_jobs_ini (init.c:1928)
> ==2532==    by 0x426D77: parse_jobs_ini (init.c:2082)
> ==2532==    by 0x46FB6E: handle_job_cmd (server.c:792)
> ==2532==    by 0x46F756: handle_command (server.c:1000)
> ==2532==    by 0x46F15C: handle_connection (server.c:1242)
> ==2532==  Address 0x636f650 is 0 bytes inside a block of size 64 free'd
> ==2532==    at 0x45E151: sfree (smalloc.c:349)
> ==2532==    by 0x46DFE1: finish_entry (server.c:1100)
> ==2532==    by 0x46DE3E: handle_sk_entry (server.c:1159)
> ==2532==    by 0x46F640: handle_xmits (server.c:1181)
> ==2532==    by 0x46F02B: handle_connection (server.c:1209)
> ==2532==    by 0x46E92A: accept_loop (server.c:1389)
> ==2532==    by 0x46D17F: fio_server (server.c:2468)
> ==2532==    by 0x46CF9B: fio_start_server (server.c:2552)
> ==2532==    by 0x429081: parse_cmd_line (init.c:2876)
> ==2532==    by 0x42A114: parse_options (init.c:2927)
> ==2532==    by 0x4AABCD: main (fio.c:42)
> ==2532==  Block was alloc'd at
> ==2532==    at 0x45E43D: __smalloc (smalloc.c:456)
> ==2532==    by 0x45E346: smalloc (smalloc.c:478)
> ==2532==    by 0x46BE58: fio_net_prep_cmd (server.c:528)
> ==2532==    by 0x469F6A: fio_net_queue_cmd (server.c:571)
> ==2532==    by 0x46FFB0: handle_probe_cmd (server.c:877)
> ==2532==    by 0x46F778: handle_command (server.c:1006)
> ==2532==    by 0x46F15C: handle_connection (server.c:1242)
> ==2532==    by 0x46E92A: accept_loop (server.c:1389)
> ==2532==    by 0x46D17F: fio_server (server.c:2468)
> ==2532==    by 0x46CF9B: fio_start_server (server.c:2552)
> ==2532==    by 0x429081: parse_cmd_line (init.c:2876)
> ==2532==    by 0x42A114: parse_options (init.c:2927)

Right, this is identical to what I reported. I get tons of these with
the patch, not without.

-- 
Jens Axboe

Re: valgrind spews

[Bart Van Assche]

On Fri, 2018-03-23 at 08:49 -0600, Jens Axboe wrote:
> Right, this is identical to what I reported. I get tons of these with
> the patch, not without.

Since I can reproduce this I will have a look at what is going on.

Bart.

Re: valgrind spews

[Sitsofe Wheeler]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On 23 March 2018 at 15:15, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Fri, 2018-03-23 at 08:49 -0600, Jens Axboe wrote:
>> Right, this is identical to what I reported. I get tons of these with
>> the patch, not without.
>
> Since I can reproduce this I will have a look at what is going on.

I wonder if the manner that fio allocates redzones is incompatible
with valgrind's due to fio's redzone's potentially being smaller than
the size of a pointer and the pre-redzone being partially inside the
header structure. I'm attaching a patch I made but it still seems to
generate valgrind warnings that aren't there when falling back to
malloc/free without pools...

-- 
Sitsofe | http://sucs.org/~sits/

[-- Attachment #2: smalloc.patch --]
[-- Type: application/octet-stream, Size: 2494 bytes --]

diff --git a/smalloc.c b/smalloc.c
index 7b1690ad..36c66b96 100644
--- a/smalloc.c
+++ b/smalloc.c
@@ -7,6 +7,7 @@
 #include <string.h>
 #ifdef CONFIG_VALGRIND_DEV
 #include <valgrind/valgrind.h>
+#include <valgrind/memcheck.h>
 #else
 #define RUNNING_ON_VALGRIND 0
 #define VALGRIND_MALLOCLIKE_BLOCK(addr, size, rzB, is_zeroed) do { } while (0)
@@ -261,24 +262,28 @@ static void *postred_ptr(struct block_hdr *hdr)
 
 static void fill_redzone(struct block_hdr *hdr)
 {
-	unsigned int *postred = postred_ptr(hdr);
+	unsigned int *postred;
 
 	/* Let Valgrind fill the red zones. */
 	if (RUNNING_ON_VALGRIND)
 		return;
 
+	postred = postred_ptr(hdr);
+
 	hdr->prered = SMALLOC_PRE_RED;
 	*postred = SMALLOC_POST_RED;
 }
 
 static void sfree_check_redzone(struct block_hdr *hdr)
 {
-	unsigned int *postred = postred_ptr(hdr);
+	unsigned int *postred;
 
 	/* Let Valgrind check the red zones. */
 	if (RUNNING_ON_VALGRIND)
 		return;
 
+	postred = postred_ptr(hdr);
+
 	if (hdr->prered != SMALLOC_PRE_RED) {
 		log_err("smalloc pre redzone destroyed!\n"
 			" ptr=%p, prered=%x, expected %x\n",
@@ -316,6 +321,7 @@ static void sfree_pool(struct pool *pool, void *ptr)
 
 	assert(ptr_valid(pool, ptr));
 
+	VALGRIND_MAKE_MEM_DEFINED(hdr, sizeof(struct block_hdr));
 	sfree_check_redzone(hdr);
 
 	offset = ptr - pool->map;
@@ -327,6 +333,7 @@ static void sfree_pool(struct pool *pool, void *ptr)
 	if (i < pool->next_non_full)
 		pool->next_non_full = i;
 	pool->free_blocks += size_to_blocks(hdr->size);
+	VALGRIND_MAKE_MEM_NOACCESS(hdr, sizeof(struct block_hdr));
 	fio_sem_up(pool->lock);
 }
 
@@ -346,7 +353,7 @@ void sfree(void *ptr)
 	}
 
 	if (pool) {
-		VALGRIND_FREELIKE_BLOCK(ptr, REDZONE_SIZE);
+		VALGRIND_FREELIKE_BLOCK(ptr - REDZONE_SIZE, REDZONE_SIZE);
 		sfree_pool(pool, ptr);
 		return;
 	}
@@ -427,10 +434,16 @@ static void *smalloc_pool(struct pool *pool, size_t size)
 	if (ptr) {
 		struct block_hdr *hdr = ptr;
 
+		VALGRIND_MAKE_MEM_DEFINED(hdr, sizeof(struct block_hdr));
 		hdr->size = alloc_size;
 		fill_redzone(hdr);
+		VALGRIND_MAKE_MEM_NOACCESS(hdr, sizeof(struct block_hdr));
 
 		ptr += sizeof(*hdr);
+
+		VALGRIND_MALLOCLIKE_BLOCK(ptr - REDZONE_SIZE, size + REDZONE_SIZE,
+					  REDZONE_SIZE,
+					  0);
 		memset(ptr, 0, size);
 	}
 
@@ -453,9 +466,6 @@ static void *__smalloc(size_t size, bool is_zeroed)
 
 			if (ptr) {
 				last_pool = i;
-				VALGRIND_MALLOCLIKE_BLOCK(ptr, size,
-							  REDZONE_SIZE,
-							  is_zeroed);
 				return ptr;
 			}
 		}

Re: valgrind spews

[Bart Van Assche]

On Fri, 2018-03-23 at 21:43 +0000, Sitsofe Wheeler wrote:
> On 23 March 2018 at 15:15, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > On Fri, 2018-03-23 at 08:49 -0600, Jens Axboe wrote:
> > > Right, this is identical to what I reported. I get tons of these with
> > > the patch, not without.
> > 
> > Since I can reproduce this I will have a look at what is going on.
> 
> I wonder if the manner that fio allocates redzones is incompatible
> with valgrind's due to fio's redzone's potentially being smaller than
> the size of a pointer and the pre-redzone being partially inside the
> header structure. I'm attaching a patch I made but it still seems to
> generate valgrind warnings that aren't there when falling back to
> malloc/free without pools...

Hello Sitsofe,

Have you enabled github's feature to send an e-mail every time a pull request
is created or a comment is added? This issue should have been resolved by a
pull request I submitted this morning. That pull request has been accepted by
Jens.

Bart.

Re: valgrind spews

[Sitsofe Wheeler]

On 23 March 2018 at 21:49, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Fri, 2018-03-23 at 21:43 +0000, Sitsofe Wheeler wrote:
>> On 23 March 2018 at 15:15, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>> > On Fri, 2018-03-23 at 08:49 -0600, Jens Axboe wrote:
>> > > Right, this is identical to what I reported. I get tons of these with
>> > > the patch, not without.
>> >
>> > Since I can reproduce this I will have a look at what is going on.
>>
>> I wonder if the manner that fio allocates redzones is incompatible
>> with valgrind's due to fio's redzone's potentially being smaller than
>> the size of a pointer and the pre-redzone being partially inside the
>> header structure. I'm attaching a patch I made but it still seems to
>> generate valgrind warnings that aren't there when falling back to
>> malloc/free without pools...
>
> Hello Sitsofe,
>
> Have you enabled github's feature to send an e-mail every time a pull request
> is created or a comment is added? This issue should have been resolved by a
> pull request I submitted this morning. That pull request has been accepted by
> Jens.

Ah thanks Bart - I see it now over on
https://github.com/axboe/fio/pull/567 (my GitHub email goes to a
different email account that I check less frequently). Kind of a shame
valgrind can't track malloc/free across a fork but also
understandable. Anyone up for enabling thread mode for fio's --server
? ;-)

-- 
Sitsofe | http://sucs.org/~sits/

Re: valgrind spews

[Jens Axboe]

> On Mar 23, 2018, at 4:18 PM, Sitsofe Wheeler <sitsofe@gmail.com> wrote:
> 
>> On 23 March 2018 at 21:49, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>>> On Fri, 2018-03-23 at 21:43 +0000, Sitsofe Wheeler wrote:
>>>> On 23 March 2018 at 15:15, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>>>>> On Fri, 2018-03-23 at 08:49 -0600, Jens Axboe wrote:
>>>>> Right, this is identical to what I reported. I get tons of these with
>>>>> the patch, not without.
>>>> 
>>>> Since I can reproduce this I will have a look at what is going on.
>>> 
>>> I wonder if the manner that fio allocates redzones is incompatible
>>> with valgrind's due to fio's redzone's potentially being smaller than
>>> the size of a pointer and the pre-redzone being partially inside the
>>> header structure. I'm attaching a patch I made but it still seems to
>>> generate valgrind warnings that aren't there when falling back to
>>> malloc/free without pools...
>> 
>> Hello Sitsofe,
>> 
>> Have you enabled github's feature to send an e-mail every time a pull request
>> is created or a comment is added? This issue should have been resolved by a
>> pull request I submitted this morning. That pull request has been accepted by
>> Jens.
> 
> Ah thanks Bart - I see it now over on
> https://github.com/axboe/fio/pull/567 (my GitHub email goes to a
> different email account that I check less frequently). Kind of a shame
> valgrind can't track malloc/free across a fork but also
> understandable. Anyone up for enabling thread mode for fio's --server
> ? ;-)

That would actually be awesome for other reasons, like enabling duo server support on platforms like Windows that don’t support fork().

Not easy, but highly encouraged :-)

Re: valgrind spews

[Bart Van Assche]

On Thu, 2018-03-22 at 10:14 -0600, Jens Axboe wrote:
> Hi Bart,
> 
> After your commit:
> 
> commit 0ffccc21fcd67d1e1d2a360e90f3fe8efc0d6b52
> Author: Bart Van Assche <bart.vanassche@wdc.com>
> Date:   Thu Mar 8 13:41:36 2018 -0800
> 
>     Improve Valgrind instrumentation of memory allocations
> 
> running valgrind on a fio spews a lot of warnings:
> 
> ==14331== Invalid write of size 8
> ==14331==    at 0x4C3451F: memset (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==14331==    by 0x4595F8: memset (string3.h:90)
> ==14331==    by 0x4595F8: smalloc_pool (smalloc.c:434)
> ==14331==    by 0x4595F8: __smalloc (smalloc.c:452)
> ==14331==    by 0x4727CD: flow_init (flow.c:112)
> ==14331==    by 0x421886: setup_thread_area (init.c:391)
> ==14331==    by 0x42552A: get_new_job (init.c:468)
> ==14331==    by 0x42552A: __parse_jobs_ini (init.c:1928)
> ==14331==    by 0x42585F: parse_jobs_ini (init.c:2082)
> [...]
> 
> Basically off any path that ends up in smalloc, on the memset()
> that we do:
> 
>         ptr = __smalloc_pool(pool, alloc_size);
>         if (ptr) {
>                 struct block_hdr *hdr = ptr;
> 
>                 hdr->size = alloc_size;
>                 fill_redzone(hdr);
> 
>                 ptr += sizeof(*hdr);
>                 memset(ptr, 0, size);
> 		^^^
>         }
> 
> for every alloc. From configure:
> 
> Valgrind headers              yes

That's weird. The only explanation I can think of is that Valgrind failed
to intercept the mmap() call in add_pool(). Can you check with the
Valgrind option --trace-syscalls=yes whether Valgrind is able on your
system to intercept mmap()?

Thanks,

Bart.