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

2012-09-21 Thread Thiago Macieira
On sexta-feira, 21 de setembro de 2012 17.45.27, Tony Van Eerd wrote: > I have no solutions (yet?). I just want to know how to constrain my > thinking. I basically won't even try going down the DCAS road. Nor the > LL/SC road, since C++11 has basically shunned that idea, sadly. > > Solutions that

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

2012-09-21 Thread Tony Van Eerd
oject.org > Subject: Re: [Development] QBasicMutex::lockInternal() race condition? > > On sexta-feira, 21 de setembro de 2012 16.46.44, Tony Van Eerd wrote: > > I'll take that as a 'No'. ie use of DCAS would be limited to the > point of > > it not being

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

2012-09-21 Thread Thiago Macieira
On sexta-feira, 21 de setembro de 2012 16.46.44, Tony Van Eerd wrote: > I'll take that as a 'No'. ie use of DCAS would be limited to the point of > it not being worth maintaining 2 separate implementations - one with DCAS, > one without. I think DCAS is only worth using if it could be used > ever

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

2012-09-21 Thread Tony Van Eerd
: development-bounces+tvaneerd=rim@qt-project.org > [mailto:development-bounces+tvaneerd=rim@qt-project.org] On Behalf > Of Thiago Macieira > Sent: Friday, September 21, 2012 10:52 AM > To: development@qt-project.org > Subject: Re: [Development] QBasicMutex::lockInternal() race c

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

2012-09-21 Thread Thiago Macieira
On sexta-feira, 21 de setembro de 2012 13.42.13, Tony Van Eerd wrote: > By the way, I assume the intent is to limit the implementation to only using > int/pointer-sized atomics, not double width atomics? We can use double-width atomics if necessary. But the only architecture for which I implemente

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

2012-09-21 Thread Tony Van Eerd
> -Original Message- > > Not really, the id can't change. > The id is only set once when the QMutexPrivate is used for the first > time. > (And that change has already been aquired for a long time) > > Acquire is uneccessary. > > It was added in commit 5bfeab87 when the uses of the ope

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

2012-09-20 Thread Olivier Goffart
On Thursday 20 September 2012 19:07:26 Thiago Macieira wrote: > On quinta-feira, 20 de setembro de 2012 16.50.18, Tony Van Eerd wrote: > > But we already did an acquire earlier when we got the d_ptr the first > > time. All our instructions on id and waiters happens after that. > > > > If we are w

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

2012-09-20 Thread Tony Van Eerd
ect: Re: [Development] QBasicMutex::lockInternal() race condition? > > 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 t

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

2012-09-20 Thread 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

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

2012-09-20 Thread Thiago Macieira
On quinta-feira, 20 de setembro de 2012 16.50.18, Tony Van Eerd wrote: > But we already did an acquire earlier when we got the d_ptr the first > time. All our instructions on id and waiters happens after that. > > If we are worried about ABA on the d_ptr, hmmm, I'd need to think about > that. Rig

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

2012-09-20 Thread Tony Van Eerd
> -Original Message- > internals? (P.S. I noticed one of those had a static QBasicMutex > inside a function, which gcc will then put a 'mutex' around...) > correction: no constructor, no compiler-installed mutex. I would hope/assume.

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

2012-09-20 Thread Tony Van Eerd
> -Original Message- > > I have a change to document more the internal, but it is not merged > yet. > > https://codereview.qt-project.org/33739 > > Staged now, thanks :-) It would be nice if the new comments on the if (d->ref()) line pointed out that 'd' is never invalid because we recyc

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

2012-09-20 Thread Tony Van Eerd
=rim@qt-project.org > [mailto:development-bounces+tvaneerd=rim@qt-project.org] On Behalf > Of Thiago Macieira > Sent: Thursday, September 20, 2012 12:14 PM > To: development@qt-project.org > Subject: Re: [Development] QBasicMutex::lockInternal() race condition? > > On

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

2012-09-20 Thread Thiago Macieira
On quinta-feira, 20 de setembro de 2012 15.50.12, Tony Van Eerd wrote: > > We check for that case shortly after: > > > > > > 361 if (d != d_ptr.loadAcquire()) { > > 362//Either the mutex is already unlocked, or relocked with > > another mutex > > 363d->deref(); > > 364co

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

2012-09-20 Thread Thiago Macieira
On quinta-feira, 20 de setembro de 2012 17.38.40, Olivier Goffart wrote: > I have a change to document more the internal, but it is not merged yet. > https://codereview.qt-project.org/33739 Staged now, thanks :-) Tony: I'd really appreciate giving the implementation a thorough once-over. I've don

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

2012-09-20 Thread Tony Van Eerd
> -Original Message- > We check for that case shortly after: > > 361 if (d != d_ptr.loadAcquire()) { > 362//Either the mutex is already unlocked, or relocked with > another mutex > 363d->deref(); > 364continue; > 365 } > Or course. Very next lines! "sh

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

2012-09-20 Thread Olivier Goffart
On Thursday 20 September 2012 15:30:32 Tony Van Eerd wrote: > I've been looking at the implementation of QMutex (et al), via woboq, ie > http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex.cpp.html. (nice > work on the code viewer, by the way!) Thanks :-) > > I'm wondering if there isn't

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

2012-09-20 Thread Tony Van Eerd
I've been looking at the implementation of QMutex (et al), via woboq, ie http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex.cpp.html. (nice work on the code viewer, by the way!) I'm wondering if there isn't a subtle race condition in lockInternal() at line 358: 358 if