Re: [PATCHv2 2/2] rpm: run binary package generation via thread pools

[Mark Hatle <mark.hatle@windriver.com>]

On 6/8/17 12:23 PM, Alexander Kanavin wrote:
> On 06/08/2017 07:55 PM, Mark Hatle wrote:
>>> +@@ -565,8 +559,8 @@ static rpmRC packageBinary(rpmSpec spec, Package pkg, const char *cookie, int ch
>>> + 	headerCopyTags(spec->packages->header, pkg->header, copyTags);
>>> + 	
>>> + 	headerPutString(pkg->header, RPMTAG_RPMVERSION, VERSION);
>>> +-	headerPutString(pkg->header, RPMTAG_BUILDHOST, buildHost());
>>> +-	headerPutUint32(pkg->header, RPMTAG_BUILDTIME, getBuildTime(), 1);
>>> ++	headerPutString(pkg->header, RPMTAG_BUILDHOST, buildHost);
>>> ++	headerPutUint32(pkg->header, RPMTAG_BUILDTIME, &buildTime, 1);
>>
>> I don't see what the advantage of this change over the current approach is --
>> unless the first call is in a thread so the variable can clash.  If that is the
>> case, it may make more sense to ensure that these two are run very early in the
>> build process before it threads.
> 
> They're run from threads, and they do clash, and I had spent a whole day 
> tracing data corruption coredumps back to these two.
> 
> This is explained in the commit message:
> 
> "Their use is causing difficult to diagnoze data races when building 
> multiple packages in parallel..."
> 
> And I had changed the code so that they're run early in the build 
> process, and just once. See below...

Yes, but you were no longer calling them and moved to passing in a variable.
The point of my comment is two fold.

One, by changing the behavior of the function you now HAVE to record the value
or if you run it again you will get a different result.  (Likely not what is
desired in the general case...)

..and two you also had to change the function calls to pass in this single value.

Assuming that moving the call earlier (like you did) works, and the static
values are retrievable by the function.  You could have fixed this with the
snipped below (without storing the values).  This would simplify the patch.

(If there is something in the way the threads work with openmp that makes the
static variable not transfer from the main execution thread to the children
threads -- that is a different issue and should be stated in the commit.)

> 
>>> +@@ -629,6 +623,8 @@ static struct binaryPackageTaskData* runBinaryPackageTasks(rpmSpec spec, const c
>>> +     struct binaryPackageTaskData *tasks = NULL;
>>> +     struct binaryPackageTaskData *task = NULL;
>>> +     struct binaryPackageTaskData *prev = NULL;
>>> ++    rpm_time_t buildTime = getBuildTime();
>>> ++    char *host = buildHost();
>>> +
>>> +     for (Package pkg = spec->packages; pkg != NULL; pkg = pkg->next) {
>>> +         task = rcalloc(1, sizeof(*task));
> 
> 
> ^^^^ right there.
> 
> Please read the code more carefully :-)
> 
> 
> Alex
>