Eliminate/convert manual pthread synchronization with STL counterparts #3302
Conversation
|
|
||
| if (ctx == NULL) | ||
| return; | ||
| rpmlogCtx ctx = rpmlogCtxAcquire(); |
There was a problem hiding this comment.
We're not using a read lock here any more (only a write lock if saverec is true), yet we read the ctx fields below. But I guess concurrent access is now prevented by the serialize lock, right?
There was a problem hiding this comment.
Good question. I think I managed to convince myself that this is safe somehow but it's not:
the serialize mutex will protect that section of code, but in the meanwhile the context is unlocked and so the callback could be changed halfway through us reading it, oops.
With the STL locking stuff, one can't choose the lock type as dynamically as with the low-level pthread stuff, and I think this is me just trying to wipe that under the carpet. Good thing you spotted it. I guess we'll just have to take a write lock here always.
There was a problem hiding this comment.
Restored the former way, only we now always take a write lock there. You win some, lose some...
But that inspired me to do what our kernel friends pointed out some time ago: eliminate one round of locking on each and every entry to rpmlog() - rpmlogSetMask() requires a lock because it does two operations, but rpmlog() entry only wants to read, and just reading an integer is atomic. (this is a separate new commit in there now)
There was a problem hiding this comment.
Good question. I think I managed to convince myself that this is safe somehow but it's not: the serialize mutex will protect that section of code, but in the meanwhile the context is unlocked and so the callback could be changed halfway through us reading it, oops.
Oh, indeed, I see it now (in the old version of the commit), too 😅. I didn't quite realize that initially but somehow the lock being scoped to the saverec block only was enough to raise a red flag 😄
With the STL locking stuff, one can't choose the lock type as dynamically as with the low-level pthread stuff, and I think this is me just trying to wipe that under the carpet. Good thing you spotted it. I guess we'll just have to take a write lock here always.
Yep, the patch is now much clearer, thanks!
There was a problem hiding this comment.
But that inspired me to do what our kernel friends pointed out some time ago: eliminate one round of locking on each and every entry to rpmlog() - rpmlogSetMask() requires a lock because it does two operations, but rpmlog() entry only wants to read, and just reading an integer is atomic. (this is a separate new commit in there now)
Nice find, too. Yeah, this also improves readability a tiny bit, using rpmlogSetMask() to, well, get a mask, was a bit confusing regardless 😄
C++ guarantees initialization of static objects takes place before threads can run, so we don't need all the locksInitialized fubar. The native objects also release themselves which fixes a theoretical leak we had: we never destroyed the lock or lockattr so they remained reachable until program exit. We could continue to acquire and release the lock in the corresponding functions but that's not RAII and would be inconsistent with what we do elsewhere in rpm, just convert to the local lock variable style.
Shared and exclusive locks are of different types in STL so we can't easily return one or the other from poolLock() as per the write argument. Just convert all poolLock() calls sites to name their lock type locally.
Replace manual pthread_* calls with the STL counterparts. Since there was nothing to wrap these calls to begin with, this makes for a particularly nice cleanup.
See previous commits for rationale. Of particular note here is that we now always take a write lock where we previously dynamically selected between read/write, because the STL primitives don't seem to support that in any easy way.
See previous commits for rationale.
See previous commits for rationale.
On entry to rpmlog() we compare the log mask to see if it's something we need to act on at all. Since we've been using rpmlogSetMask() for this, each and every RPMLOG_DEBUG etc call that is not normally logged stops on its way to take at least one read mutex to do nothing at all. Which is nuts. rpmlogSetMask() technically does need the mutex lock because it both reads and writes, and something could come in between. But for the rpmlog() entry we only need to read, and reading an int is atomic. Add an internal helper that lets us get the silly mask without locking.
The initial implementation in 4e158f5 used pthread once-only initialization for verifylevel but that got dropped soon after, only the include remains.
C++ guarantees static initializers run in a thread-safe manner, move the confdir initialization to a constructor of a tiny object and voila we don't need the pthread once dance.
Move the tag table initialization to static object constructor so it runs before there are threads to worry about. This will slightly increase our startup time for the arbitrary 'rpm --eval "%foo"' type use but unlikely to matter in practise. To do this, minimally objectify our tag entry table. The resulting code division doesn't make too much sense but that can be cleaned up later.
Details in commits, tldr version: eliminate all manual pthread mutex and init-once calls, either by taking advantage of the fact that unlike C, C++ guarantees static initialization to be thread-safe, or by using STL mutex/lock facilities.