[BUG] aiaiai-test-patchset removes obj dirs too early
Keller, Jacob E
jacob.e.keller at intel.com
Tue Aug 4 16:25:24 PDT 2015
> -----Original Message-----
> From: aiaiai [mailto:aiaiai-bounces at lists.infradead.org] On Behalf Of Brian
> 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
> 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
> 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)
> Subject: [PATCH] aiaiai-test-patchset: sync with bisection test before
> 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" >
> + # 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.
> if [ -n "$preserve" ]; then
> message "Preserving objdirs: $obj1 $obj2"
> @@ -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
> # Print the results
> aiaiai mailing list
> aiaiai at lists.infradead.org
More information about the aiaiai