[Development] QBasicMutex::lockInternal() race condition?

[Thiago Macieira]

On quinta-feira, 20 de setembro de 2012 16.53.23, Tony Van Eerd wrote:
> > Tony: I'd really appreciate giving the implementation a thorough once-
> > over.
> 
> I will try to find the time, although I'm already further down the rabbit
> hole than I should be.  I was really investigating what can be used in a
> static context, and what can't.  Would you recommend that client code use
> QBasicMutex when static mutexes are needed (which is a common use for
> mutexes).  Like the recent changes done for Qt internals?  

QBasicAtomic and QBasicMutex are PODs, but QBasicTimer isn't. Additionally, 
QElapsedTimer is POD.

But QBasicAtomic and QBasicMutex are private classes. You should not use them 
outside Qt code -- at least, not yet. For Qt 5.1, I want to make QBasicAtomic 
public and documented. QBasicMutex will require some more thinking.

Also note that QAtomicInt is fine as a static in C++11 mode, since its 
constructor is constexpr. But we can't depend on that in Qt code.

> (P.S. I noticed
> one of those had a static QBasicMutex inside a function, which gcc will
> then put a 'mutex' around...)

As you said in the next email, QBasicMutex is POD, so there's no compiler 
guard.

> P.S. although I agree that recursive mutex is often wrong, it is commonly
> used.  Possibly the most common, particularly for OOP and ex-java
> developers.  (And to me, Qt is very java-esque, no offense.)

Yeah, it usually means you could not resolve the recursion properly.

> > I've done it once and, after rewriting it, I came up with almost the
> > same
> > solutions. My implementation only differed that I added a spinlock, by
> > using
> > the lowest bit on the QMutexPrivate pointer.
> 
> did you use "testAndtestAndSet" ?
> 
> while (locked || !testAndSet(&locked, 0, 1))
>    yield-or-whatever;
>
> ie mostly spin on a relaxed read.  Only do the memory-barrier CAS when it
> "looks" unlocked, and thus has a chance of succeeding.

No, see qmutex.cpp in https://codereview.qt-project.org/33366
 
   while (!d_ptr.testAndSetAcquire(0, dummyLocked())) {
        QMutexData *copy = d_ptr.load();
        loopPause();
        copy = spinUnlocked(copy);

        if (d_ptr.testAndSetAcquire(copy, spinLocked(copy))) {

It always tries to go from unlocked to locked, even if it read the locked 
value (copy == spinLocked(copy)).

This is mostly because I wrote the original code with the 3-argument 
testAndSet:

    while (true) {
        loopPause();
        copy = spinUnlocked(copy);

        if (d_ptr.testAndSetAcquire(copy, spinLocked(copy), copy)) {

Which avoids the extra load from memory. It only uses the value that it has 
already got.

> In general, this problem seems the same as the pthread_once(), so various
> solutions are out there.  Lawrence Crowl had a good solution using
> thread-local-storage.  It is used in boost::call_once() for the non-Windows
> implementation.  The Windows impl is similar to QMutex in that it tries to
> avoid CreateEvent (like trying to avoid allocating the d_ptr) unless there
> is contention.  But the boost version has a small case where it never
> cleans up the event (which is OK for once(), since it tends to be a static
> that is never cleaned up anyhow).

This can only happen for timed locks when one thread gives up waiting at the 
moment the other unlocks. That's why Olivier's implementation has the 
possiblyUnlocked flag. That's why I applied a patch that removes timed locks 
from QBasicMutex.

In my implementation, that can't happen because the reference count is only 
modified under spinlock.

> I had a solution to that small case, but
> it took about 5 pages of code (lots of comments of course).  And used a
> list built across objects on the stacks of the waiting threads.  Which is
> scary if someone decides to terminate a thread.  But it showed that you
> could do everything with a single int, if you really wanted to.  I think I
> had a solution with 2 ints without the list...

I can only imagine what the solution might be. Or not.

Anyway, there's another possibility:
QBasicMutex can only be used as a static, never on the stack or as a member. 
Therefore, it's acceptable if it leaks the waiter structure, like the case you 
mentioned above. The leak would only manifest if someone is loading and 
unloading constantly a plugin. So a possibility is simply to keep the same 
waiter structure forever and leak it at the end of the execution.

QMutex has a destructor, so it can properly free it when it is destroyed.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
     Intel Sweden AB - Registration Number: 556189-6027
     Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20120920/36442358/attachment.sig>