diff -urN -X ../dontdiff ../base/linux-2.5.30/ipc/util.c 2.5.30-ipc/ipc/util.c
--- ../base/linux-2.5.30/ipc/util.c Thu Aug 1 14:16:21 2002
+++ 2.5.30-ipc/ipc/util.c Fri Aug 2 16:06:19 2002
@@ -74,9 +74,11 @@
printk(KERN_ERR "ipc_init_ids() failed, ipc service disabled.\n");
ids->size = 0;
}
- ids->ary = SPIN_LOCK_UNLOCKED;
- for(i=0;i<ids->size;i++)
+ ids->ary_lock =RW_LOCK_UNLOCKED;
+ for(i=0;i<ids->size;i++) {
ids->entries[i].p = NULL;
+ ids->entries[i].lock = SPIN_LOCK_UNLOCKED;
+ }
}
/**
@@ -119,14 +121,15 @@
memcpy(new, ids->entries, sizeof(struct ipc_id)*ids->size);
for(i=ids->size;i<newsize;i++) {
new[i].p = NULL;
+ new[i].lock = SPIN_LOCK_UNLOCKED;
}
- spin_lock(&ids->ary);
+ write_lock(&ids->ary_lock);
old = ids->entries;
ids->entries = new;
i = ids->size;
ids->size = newsize;
- spin_unlock(&ids->ary);
+ write_unlock(&ids->ary_lock);
ipc_free(old, sizeof(struct ipc_id)*i);
return ids->size;
}
@@ -165,7 +168,8 @@
if(ids->seq > ids->seq_max)
ids->seq = 0;
- spin_lock(&ids->ary);
+ read_lock(&ids->ary_lock);
+ spin_lock(&ids->entries[id].lock);
ids->entries[id].p = new;
return id;
}
diff -urN -X ../dontdiff ../base/linux-2.5.30/ipc/util.h 2.5.30-ipc/ipc/util.h
--- ../base/linux-2.5.30/ipc/util.h Thu Aug 1 14:16:28 2002
+++ 2.5.30-ipc/ipc/util.h Fri Aug 2 16:06:19 2002
@@ -20,11 +20,13 @@
unsigned short seq_max;
struct semaphore sem;
spinlock_t ary;
+ rwlock_t ary_lock;
struct ipc_id* entries;
};
struct ipc_id {
struct kern_ipc_perm* p;
+ spinlock_t lock;
};
@@ -72,16 +74,25 @@
if(lid >= ids->size)
return NULL;
- spin_lock(&ids->ary);
+ /*spin_lock(&ids->ary);*/
+ read_lock(&ids->ary_lock);
+ spin_lock(&ids->entries[lid].lock);
out = ids->entries[lid].p;
- if(out==NULL)
- spin_unlock(&ids->ary);
+ if(out==NULL) {
+ spin_unlock(&ids->entries[lid].lock);
+ read_unlock(&ids->ary_lock);
+ }
return out;
}
extern inline void ipc_unlock(struct ipc_ids* ids, int id)
{
- spin_unlock(&ids->ary);
+ int lid = id % SEQ_MULTIPLIER;
+ if(lid >= ids->size)
+ return;
+
+ spin_unlock(&ids->entries[lid].lock);
+ read_unlock(&ids->ary_lock);
}
extern inline int ipc_buildid(struct ipc_ids* ids, int id, int seq)
On Mon, 5 Aug 2002, mingming cao wrote:
> In current implementation, the operations on any IPC semaphores are
> synchronized by one single IPC semaphore lock. Changing the IPC locks
> from one lock per IPC resource type into one lock per IPC ID makes sense
> to me. By doing so could reduce the possible lock contention in some
> applications where the IPC resources are heavily used.
Yes, the unused "id" argument to ipc_lock() cries out to be put to use.
However...
> Test results from the LMbench Pipe and IPC latency test shows this patch
> improves the performance of those functions from 1% to 9%.
I cast doubt in other mail: I don't see how LMbench gets here at all.
If it's worth changing around this SysV IPC locking,
then I think there's rather more that needs to be done:
1. To reduce dirty cacheline bouncing, shouldn't the per-id spinlock
be in the kern_ipc_perm structure pointed to by entries[lid], not
mixed in with the pointers of the entries array? I expect a few
areas would need to be reworked if that change were made, easy to
imagine wanting the lock/unlock before/after the structure is there.
2. I worry more about the msg_ids.sem, sem_ids.sem, shm_ids.sem which
guard these areas too. Yes, there are some paths where the ipc_lock
is taken without the down(&ipc_ids.sem) (perhaps those are even the
significant paths, I haven't determined); but I suspect there's more
to be gained by avoiding those (kernel)semaphores than by splitting
the spinlocks.
3. You've added yet another level of locking, the read/write-locking
on ary_lock. That may be the right way to go, but I think there's
now huge redundancy between that and the ipc_ids.sem - should be
possible to get rid of one or the other.
4. You've retained the ids->ary field when you should have removed it;
presumably retained so ipc_lockall,ipc_unlockall compile, but note
that now ipc_lockall only locks against another ipc_lockall, which
is certainly not its intent. If it's essential (it's only used for
SHM_INFO), then I think you need to convert it to a lock on ary_lock.
Hugh
Hugh Dickins wrote:
>
> 1. To reduce dirty cacheline bouncing, shouldn't the per-id spinlock
> be in the kern_ipc_perm structure pointed to by entries[lid], not
> mixed in with the pointers of the entries array? I expect a few
> areas would need to be reworked if that change were made, easy to
> imagine wanting the lock/unlock before/after the structure is there.
>
You are right at the cacheline bouncing issue. I will make that change.
> 2. I worry more about the msg_ids.sem, sem_ids.sem, shm_ids.sem which
> guard these areas too. Yes, there are some paths where the ipc_lock
> is taken without the down(&ipc_ids.sem) (perhaps those are even the
> significant paths, I haven't determined); but I suspect there's more
> to be gained by avoiding those (kernel)semaphores than by splitting
> the spinlocks.
>
I don't worry the ipc_ids.sem very much. They are used to protect the
IPC info, which is not updated quiet often (by operation such as
semctl()). Significant IPC operations, like semop(), msgsnd() and
msgrcv(), need the access to the IPC ID, where only the ipc_lock() is
needed.
> 3. You've added yet another level of locking, the read/write-locking
> on ary_lock. That may be the right way to go, but I think there's
> now huge redundancy between that and the ipc_ids.sem - should be
> possible to get rid of one or the other.
>
They look similar at the first glance. But they serve for different
purposes. The ipc_ids.sem is used to protect the IPC infos, while the
ary_lock is used to protect the IPC ID array. This way we could do
semctl() and semop() in parallel.
> 4. You've retained the ids->ary field when you should have removed it;
> presumably retained so ipc_lockall,ipc_unlockall compile, but note
> that now ipc_lockall only locks against another ipc_lockall, which
> is certainly not its intent. If it's essential (it's only used for
> SHM_INFO), then I think you need to convert it to a lock on ary_lock.
>
Thanks for point this out to me. I need to get ipc_lockall/ipc_unlockall
fixed.
Mingming