Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/#review18472 --- OK, I've committed a spinlock implementation that should work on OpenBSD without relying on GCC intrinsics alone (it does rely on Qt's atomic classes though). Instead of making the additional functions used wait for the CMake changes I've simply checked for _POSIX_PRIORITY_SCHEDULING as documented in the Linux manpage (and verified not to be defined in OpenBSD's unistd.h or cdefs.h, since sched_yield isn't provided in sched.h). Since the standards regarding sched_yield in particular seem to have changed several times recently (most recently moving to the Base specification for SUS) the only safe thing would be to have a CMake check for that specific function even if the POSIX feature checks appear to support it being available. But that will need to be worked on in the 4.10 branch. - Michael Pyne On Aug. 26, 2012, 7:09 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/ --- (Updated Aug. 26, 2012, 7:09 p.m.) Review request for kdelibs and Michael Pyne. Description --- Add simple spin locking mechanism. Written by Michael Pyne as from https://bugs.kde.org/attachment.cgi?id=73282 , with some tweaking by me. This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Diffs - kdecore/util/ConfigureChecks.cmake fe9f47e kdecore/util/config-util.h.cmake 83ccdf7 kdecore/util/kshareddatacache_p.h ec5a7a0 Diff: http://git.reviewboard.kde.org/r/106224/diff/ Testing --- On OpenBSD-CURRENT, i386 Thanks, Vadim Zhukov
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On segunda-feira, 27 de agosto de 2012 20.29.52, Michael Pyne wrote: On Monday, August 27, 2012 20:18:34 Michael Pyne wrote: On Tuesday, August 28, 2012 00:41:16 Thiago Macieira wrote: QBasicAtomicInt are permitted in unions. Besides, why do you want it in a union in the first place? You should not access the data that it holds *except* via the QBasicAtomicInt functions. That would be the idea, yes (to use the public QBAI functions). The problem with having it in a union was that it's a non-POD type according to C++ 03 rules (or at least, that seemed to be the issue when I had tried that initially). Actually I take that back. I was using QAtomicInt, which had that problem. QBasicAtomicInt works just fine in the union... yay! That's the whole point of QBasicAtomicInt: it's POD. Anyway, you haven't explained why you need it in a union with something else. Are you accessing the data outside of QBasicAtomicInt? If so, that's wrong. if you're not, you probably don't need the union. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On terça-feira, 28 de agosto de 2012 12.28.24, Vadim Zhukov wrote: See the definition of SharedLock structure in kshareddatacache_p.h. Actually, other union members will not be accessed simultaneously with spinlock, but compiler doesn't know about that. I don't see the need for a union. The other types aren't related to a spinlock. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On quarta-feira, 29 de agosto de 2012 00.36.07, Vadim Zhukov wrote: 2012/8/28 Thiago Macieira thi...@kde.org: On terça-feira, 28 de agosto de 2012 12.28.24, Vadim Zhukov wrote: See the definition of SharedLock structure in kshareddatacache_p.h. Actually, other union members will not be accessed simultaneously with spinlock, but compiler doesn't know about that. I don't see the need for a union. The other types aren't related to a spinlock. The main thing there is char unused[64] below. The union is needed to keep the size of the whole structure constant. Or... is it impossible that there will be run two KDE-based apps with size of Qt atomic type simultaneously; e.g. during OS update? The union is useless. If the size of the structure changed, the cross-process locking would stop working completely. And in any case, if you're using a spinlock, you're not using a pthread_mutex. You don't need it, so remove it. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On Wednesday, August 29, 2012 00:36:07 Vadim Zhukov wrote: 2012/8/28 Thiago Macieira thi...@kde.org: On terça-feira, 28 de agosto de 2012 12.28.24, Vadim Zhukov wrote: See the definition of SharedLock structure in kshareddatacache_p.h. Actually, other union members will not be accessed simultaneously with spinlock, but compiler doesn't know about that. I don't see the need for a union. The other types aren't related to a spinlock. The main thing there is char unused[64] below. The union is needed to keep the size of the whole structure constant. Or... is it impossible that there will be run two KDE-based apps with size of Qt atomic type simultaneously; e.g. during OS update? It's correct that the unused[64] is the key to the union. Since the SharedLock type (including the union) will be placed in shared memory it is desired that the size does that structure does not depend on the type of lock that is being used. It is true that once a cache is created with a given locktype that the locktype will never change for that cache again, so I suppose this feature is not an inherent requirement. I'll look into that too I suppose. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Review Request: Add spinlocks lock type, based on GCC intrisincs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/ --- Review request for kdelibs and Michael Pyne. Description --- Add simple spin locking mechanism. Written by Michael Pyne as from https://bugs.kde.org/attachment.cgi?id=73282 , with some tweaking by me. This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Diffs - kdecore/util/ConfigureChecks.cmake fe9f47e kdecore/util/config-util.h.cmake 83ccdf7 kdecore/util/kshareddatacache_p.h ec5a7a0 Diff: http://git.reviewboard.kde.org/r/106224/diff/ Testing --- On OpenBSD-CURRENT, i386 Thanks, Vadim Zhukov
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On domingo, 26 de agosto de 2012 19.09.15, Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/ --- Review request for kdelibs and Michael Pyne. Description --- Add simple spin locking mechanism. Written by Michael Pyne as from https://bugs.kde.org/attachment.cgi?id=73282 , with some tweaking by me. This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Please use the Qt atomic types. Until GCC 4.7, they generate better code. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On Tuesday, August 28, 2012 00:41:16 Thiago Macieira wrote: On segunda-feira, 27 de agosto de 2012 18.20.15, Michael Pyne wrote: Please use the Qt atomic types. Until GCC 4.7, they generate better code. I agree, the reason it wasn't that way initially is mentioned in the discussion on the bug (but basically because you can't simply put QBasicAtomicInt in the union used to store the different lock types that are possible). Why not? QBasicAtomicInt are permitted in unions. Besides, why do you want it in a union in the first place? You should not access the data that it holds *except* via the QBasicAtomicInt functions. That would be the idea, yes (to use the public QBAI functions). The problem with having it in a union was that it's a non-POD type according to C++ 03 rules (or at least, that seemed to be the issue when I had tried that initially). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
