Re: [PATCH] Breaking down the global IPC locks

Hugh Dickins (hugh@veritas.com)
Tue, 6 Aug 2002 16:53:59 +0100 (BST)


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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/