Re: [BUG] resolved deltas

From: Jeff King <peff@peff.net>
Date: Aug 28, 2014, 22:08 UTC
Message-ID: <20140828220821.GA31545@peff.net>
In-Reply-To: <53FB66D1.709@web.de>

On Mon, Aug 25, 2014 at 06:39:45PM +0200, René Scharfe wrote:

Show 5 quoted lines

> Thanks, that looks good.  But while preparing the patch I noticed that
> the added test sometimes fails.  Helgrind pointed outet a race
> condition.  It is not caused by the patch to turn the asserts into
> regular ifs, however -- here's a Helgrind report for the original code
> with the new test:

Interesting. I couldn't convince Helgrind to catch such a case, but it makes sense. We split the delta-resolving work by dividing up the base objects. We then find any deltas that need that base object (which is read-only). If there's only one instances of the base, then we'll be the only thread working on those delta. But if there are two such bases, they're going to look at the same deltas.

So we need some kind of mutual exclusion so that only one thread proceeds with resolving the delta. The "real_type" check sort-of functions in that way (except of course it is not actually thread safe). So one obvious option is just a coarse-grained global lock to modify or check real_type fields. That would probably perform badly (lots of useless lock contention on unrelated objects), but at least it would work.

If we accept pushing a lock into _each_ object_entry, then we would get a lot less lock contention (i.e., none at all in the common case of no duplicates). The cost is storing one lock per object, though. Not great, but probably OK.

Is there some way we can make real_type itself more atomic? I.e., use it as the mutex with the rule that we do not "claim" the object_entry as ours to work on until we atomically set delta_obj->real_type. I think doing a compare-and-swap looking for OBJ_REF_DELTA and replacing it with base->real_type would be enough there (and substitute OBJ_OFS_DELTA for the second conditional, of course).

However, I'm not sure we can portably rely on having a compare-and-swap primitive (or that we want to go through the hassle of conditionally using it).

-Peff