On 2014-06-23 19:59:10 +0530, Amit Kapila wrote: > On Tue, Jun 17, 2014 at 8:56 PM, Andres Freund <and...@2ndquadrant.com> > wrote: > > On 2014-06-17 20:47:51 +0530, Amit Kapila wrote: > > > On Tue, Jun 17, 2014 at 6:35 PM, Andres Freund <and...@2ndquadrant.com> > > > wrote: > > > > > > You have followed it pretty well as far as I can understand from your > > > replies, as there is no reproducible test (which I think is bit tricky > to > > > prepare), so it becomes difficult to explain by theory. > > > > I'm working an updated patch that moves the releaseOK into the > > spinlocks. Maybe that's the problem already - it's certainly not correct > > as is. > > Sure, I will do the test/performance test with updated patch; you > might want to include some more changes based on comments > in mail below:
I'm nearly finished in cleaning up the atomics part of the patch which also includes a bit of cleanup of the lwlocks code. > Few more comments: > > 1. > LWLockAcquireCommon() > { > .. > iterations++; > } > > In current logic, I could not see any use of these *iterations* variable. It's useful for debugging. Should be gone in the final code. > 2. > LWLockAcquireCommon() > { > .. > if (!LWLockDequeueSelf(l)) > { > /* > * Somebody else dequeued us and has or will.. > .. > */ > for (;;) > { > PGSemaphoreLock(&proc->sem, false); > if (!proc->lwWaiting) > break; > extraWaits++; > } > lock->releaseOK = true; > } > > Do we want to set result = false; after waking in above code? > The idea behind setting false is to indicate whether we get the lock > immediately or not which previously was decided based on if it needs > to queue itself? Hm. I don't think it's clear which version is better. > 3. > LWLockAcquireCommon() > { > .. > /* > * Ok, at this point we couldn't grab the lock on the first try. We > * cannot simply queue ourselves to the end of the list and wait to be > * woken up because by now the lock could long have been released. > * Instead add us to the queue and try to grab the lock again. If we > * suceed we need to revert the queuing and be happy, otherwise we > * recheck the lock. If we still couldn't grab it, we know that the > * other lock will see our queue entries when releasing since they > * existed before we checked for the lock. > */ > /* add to the queue */ > LWLockQueueSelf(l, mode); > > /* we're now guaranteed to be woken up if necessary */ > mustwait = LWLockAttemptLock(l, mode, false, &potentially_spurious); > .. > } > > a. By reading above code and comments, it is not quite clear why > second attempt is important unless somebody thinks on it or refer > your comments in *Notes* section at top of file. I think it's better to > indicate in some way so that code reader can refer to Notes section or > whereever you are planing to keep those comments. Ok. > b. There is typo in above comment suceed/succeed. Thanks, fixed. > > 4. > LWLockAcquireCommon() > { > .. > if (!LWLockDequeueSelf(l)) > { > for (;;) > { > PGSemaphoreLock(&proc->sem, false); > if (!proc->lwWaiting) > break; > extraWaits++; > } > lock->releaseOK = true; > .. > } > > Setting releaseOK in above context might not be required because if the > control comes in this part of code, it will not retry to acquire another > time. Hm. You're probably right. > 5. > LWLockWaitForVar() > { > .. > else > mustwait = false; > > if (!mustwait) > break; > .. > } > > I think we can directly break in else part in above code. Well, there's another case of mustwait=false above which is triggered while the spinlock is held. Don't think it'd get simpler. > 6. > LWLockWaitForVar() > { > .. > /* > * Quick test first to see if it the slot is free right now. > * > * XXX: the caller uses a spinlock before this,... > */ > if (pg_atomic_read_u32(&lock->lockcount) == 0) > return true; > } > > Does the part of comment that refers to spinlock is still relevant > after using atomic ops? Yes. pg_atomic_read_u32() isn't a memory barrier (and explicitly documented not to be). > 7. > LWLockWaitForVar() > { > .. > /* > * Add myself to wait queue. Note that this is racy, somebody else > * could wakeup before we're finished queuing. > * NB: We're using nearly the same twice-in-a-row lock acquisition > * protocol as LWLockAcquire(). Check its comments for details. > */ > LWLockQueueSelf(l, LW_WAIT_UNTIL_FREE); > > /* we're now guaranteed to be woken up if necessary */ > mustwait = LWLockAttemptLock(l, LW_EXCLUSIVE, false, > &potentially_spurious); > } > > Why is it important to Attempt lock after queuing in this case, can't > we just re-check exclusive lock as done before queuing? Well, that's how Heikki designed LWLockWaitForVar(). > 8. > LWLockWaitForVar() > { > .. > PRINT_LWDEBUG("LWLockAcquire undo queue", lock, mode); > break; > } > else > { > PRINT_LWDEBUG("LWLockAcquire waiting 4", lock, mode); > } > .. > } > > a. I think instead of LWLockAcquire, here we should use > LWLockWaitForVar right. > b. Isn't it better to use LOG_LWDEBUG instead of PRINT_LWDEBUG(), > as PRINT_LWDEBUG() is generally used in file at entry of functions to > log info about locks? Fine with me. > 9. > LWLockUpdateVar() > { > /* Acquire mutex. Time spent holding mutex should be short! */ > SpinLockAcquire(&lock->mutex); > .. > } > > Current code has an Assert for exclusive lock which is missing in > patch, is there any reason for removing Assert? That assert didn't use to be there on master... I'll add it again. > 10. > LWLockRelease(LWLock *l) > { > .. > if (l == held_lwlocks[i].lock) > { > mode = held_lwlocks[i].mode; > .. > } > > It seems that mode is added to held_lwlocks to use it in LWLockRelease(). > If yes, then can we deduce the same from lockcount? No. It can be temporarily too high (the whole backout stuff). It's also much cheaper to test a process local variable. > 11. > LWLockRelease() > { > .. > PRINT_LWDEBUG("LWLockRelease", lock, mode); > } > > Shouldn't this be in begining of LWLockRelease function rather than > after processing held_lwlocks array? Ok. > 12. > #ifdef LWLOCK_DEBUG > lock->owner = MyProc; > #endif > > Shouldn't it be reset in LWLockRelease? That's actually intentional. It's quite useful to know the last owner when debugging lwlock code. > * 2) If somebody spuriously got blocked from acquiring the lock, they will > * get queued in Phase 2 and we can wake them up if neccessary or they > will > * have gotten the lock in Phase 2. > > In the above line, I think the second mention of *Phase 2* should be *Phase > 3*. Right, good catch. > > I think both are actually critical for performance... Otherwise even a > > only lightly contended lock would require scheduler activity when a > > processes tries to lock something twice. Given the frequency we acquire > > some locks with that'd be disastrous... > > Do you have any suggestion how both behaviours can be retained? Not sure what you mean. They currently *are* retained? Or do you mean whether they could be retained while making lwlocks fai? > Note - Still there is more to review in this patch, however I feel it is > good idea to start some test/performance test of this patch, if you > agree, then I will start the same with the updated patch (result > of conclusion of current review comments). I'll post a new version of this + the atomics patch tomorrow. Since the whole atomics stuff has changed noticeably it probably makes sense to wait till then. Thanks for the look! Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers