[BUG] aiaiai-test-patchset removes obj dirs too early

[Keller, Jacob E]

Hi Brian,

> -----Original Message-----
> From: aiaiai [mailto:aiaiai-bounces at lists.infradead.org] On Behalf Of Brian
> Norris
> Sent: Tuesday, August 04, 2015 4:11 PM
> To: aiaiai at lists.infradead.org
> Cc: Brian Norris; Robert Jarzmik
> Subject: [BUG] aiaiai-test-patchset removes obj dirs too early
> 
> Hi,
> 
> This one has been a productivity staller, now that I'm finding machines
> that will expose the race condition... it looks like this commit is very
> broken:
> 
> commit 2115f7ff059260732208d9330d8d8db17734dfab
> Author: Robert Jarzmik <robert.jarzmik at free.fr>
> Date:   Thu Nov 6 20:46:50 2014 +0100
> 
>     aiaiai-test-patchset: optimize disk space
> 
> It tries to remove the obj directories as soon as the before + after
> builds are done, but it overlooks the fact that the bisectability test
> is being forked in the background.
> 

Ouch, nice find.

> How would y'all recommend fixing this? I can see two options:
> 
> 1. revert the "optimization"
> 2. sync with (i.e., 'wait') the bisectability test within
>    test_configuration(), before removing the build artifacts
> 
> According to one test, option 1 works fine.
> 
> However, the parallelism that exposed this bug looks like a problem too,
> so I suppose it's better to fix this properly, a la option 2. Patch
> below. I'm not sure if it has any performance implications, or if it's
> really just a good patch to fix up a flaw in the design. Comment away!
> 

I agree with (2)

> Regards,
> Brian
> 
> Subject: [PATCH] aiaiai-test-patchset: sync with bisection test before
> moving
>  on
> 
> For whatever reason (parallelism, I suppose?) we fork the bisectability
> test to the background and continue -- we only sync with this test after
> all configs are completed. This is bad for a number of reasons.
> 
> 1. commit 2115f7ff0592 ("aiaiai-test-patchset: optimize disk space")
>    introduced a race condition, where we might try to blow away the
>    build objects before the bisection test is complete. This is really bad.
> 
> 2. Theoretically, we could have more than one bisection test forked to
>    the background, if there are more than 1 running config. But we
>    clobber $pid_bisect every time we run test_configuration(), so we
>    effectively only sync with the last one. This is *usually* OK,
>    because the prior config can likely complete by the time subsequent
>    config(s) complete, but that's still technically a race.
> 
> So, let's move the 'wait $pid_bisect' into the test_configuration()
> function, to make sure the test is complete before we move to the next
> config (and remove the build objects). While this removes a little bit
> of parallelism, I suspect that parallelism was unintentional.
> 

I agree here, I don't think this particular parallelism was really intended.

> Signed-off-by: Brian Norris <computersforpeace at gmail.com>
> ---
>  aiaiai-test-patchset | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/aiaiai-test-patchset b/aiaiai-test-patchset
> index 85fd0575dd4b..92ca3dfe8a40 100755
> --- a/aiaiai-test-patchset
> +++ b/aiaiai-test-patchset
> @@ -248,6 +248,10 @@ test_configuration()
>  	aiaiai-diff-log $verbose $preserve "$tmpdir/diff-for-diff-log"
> "$log1.stderr.log" \
>  		        -w "$tmpdir" "$log2.stderr.log" >
> "$tmpdir/$config.stderr.diff"
> 
> +	# MUST sync with bisect test before removing build artifacts
> +	[ -z "$pid_bisect" ] || wait "$pid_bisect" || die "aiaiai-test-
> bisectability failed"
> +	verbose "Done with bisectability test"
> +

This seems fine to me. I don't think we lose that much parallelism here. I don't think this will drastically reduce performance, and it definitely avoids the issue.

I'll go ahead and queue this patch up.

Regards,
Jake

>  	if [ -n "$preserve" ]; then
>  		message "Preserving objdirs: $obj1 $obj2"
>  	else
> @@ -537,7 +541,6 @@ done
> 
>  [ -z "$checkpatch" ]   || wait "$pid_checkpatch" || die "checkpatch failed"
>  [ -z "$keywords" ]     || wait "$pid_keywords" || die "aiaiai-match-
> keywords failed"
> -[ -z "$pid_bisect" ]   || wait "$pid_bisect"   || die "aiaiai-test-bisectability
> failed"
> 
>  # Print the results
> 
> --
> 2.5.0.rc2.392.g76e840b
> 
> 
> _______________________________________________
> aiaiai mailing list
> aiaiai at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/aiaiai