Re: dynamic shared memory and locks

(2014/01/11 3:11), Robert Haas wrote:
> On Mon, Jan 6, 2014 at 5:50 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> This is only part of the solution, of course: a complete solution will
>> involve making the hash table key something other than the lock ID.
>> What I'm thinking we can do is making the lock ID consist of two
>> unsigned 32-bit integers. One of these will be stored in the lwlock
>> itself, which if my calculations are correct won't increase the size
>> of LWLockPadded on any common platforms (a 64-bit integer would).
>> Let's call this the "tranch id". The other will be derived from the
>> LWLock address. Let's call this the "instance ID". We'll keep a
>> table of tranch IDs, which will be assigned consecutively starting
>> with 0. We'll keep an array of metadata for tranches, indexed by
>> tranch ID, and each entry will have three associated pieces of
>> information: an array base, a stride length, and a printable name.
>> When we need to identify an lwlock in the log or to dtrace, we'll
>> fetch the tranch ID from the lwlock itself and use that to index into
>> the tranch metadata array. We'll then take the address of the lwlock,
>> subtract the array base address for the tranch, and divide by the
>> stride length; the result is the instance ID. When reporting the
>> user, we can report either the tranch ID directly or the associated
>> name for that tranch; in either case, we'll also report the instance
>> ID.
>>
>> So initially we'll probably just have tranch 0: the main LWLock array.
>> If we move buffer content and I/O locks to the buffer headers, we'll
>> define tranch 1 and tranch 2 with the same base address: the start of
>> the buffer descriptor array, and the same stride length, the size of a
>> buffer descriptor. One will have the associated name "buffer content
>> lock" and the other "buffer I/O lock". If we want, we can define
>> split the main LWLock array into several tranches so that we can more
>> easily identify lock manager locks, predicate lock manager locks, and
>> buffer mapping locks.
>
> OK, I've implemented this: here's what I believe to be a complete
> patch, based on the previous lwlock-pointers.patch but now handling
> LOCK_DEBUG and TRACE_LWLOCKS and dtrace and a bunch of other loose
> ends. I think this should be adequate for allowing lwlocks to be
> stored elsewhere in the main shared memory segment as well as in
> dynamic shared memory segments.
>
> Thoughts?
>
I briefly checked the patch. Most of lines are mechanical replacement
from LWLockId to LWLock *, and compiler didn't claim anything with
-Wall -Werror option.

My concern is around LWLockTranche mechanism. Isn't it too complicated
towards the purpose?
For example, it may make sense to add "char lockname[NAMEDATALEN];" at
the tail of LWLock structure if LOCK_DEBUG is enabled. Probably, it
also adds an argument of LWLockAssign() to gives the human readable
name. Is the additional 64bytes (= NAMEDATALEN) per lock too large
for recent hardware?

Below is minor comments.

It seems to me this newer form increased direct reference to
the MainLWLockArray. Is it really graceful?
My recommendation is to define a macro that wraps the reference to
MainLWLockArray.

For example:
#define PartitionLock(i) \
(&MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + (i)].lock)

instead of the following manner.

@@ -3400,7 +3406,12 @@ GetLockStatusData(void)
* Must grab LWLocks in partition-number order to avoid LWLock deadlock.
*/
for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
- LWLockAcquire(FirstLockMgrLock + i, LW_SHARED);
+ {
+ LWLock *partitionLock;
+
+ partitionLock = &MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + i].lock;
+ LWLockAcquire(partitionLock, LW_SHARED);
+ }

This chunk is enclosed by #ifdef EXEC_BACKEND, the legacy LWLockArray
was not removed.

@@ -5633,6 +5630,7 @@ save_backend_variables(BackendParameters *param, Port *port,
param->SpinlockSemaArray = SpinlockSemaArray;
#endif
param->LWLockArray = LWLockArray;
+ param->MainLWLockArray = MainLWLockArray;
param->ProcStructLock = ProcStructLock;
param->ProcGlobal = ProcGlobal;
param->AuxiliaryProcs = AuxiliaryProcs;

Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>