Re: [patch] __lock_sock race condition in 2.3.18*

David S. Miller (davem@redhat.com)
Mon, 20 Sep 1999 11:23:46 -0700


Date: Mon, 20 Sep 1999 20:16:22 +0200 (CEST)
From: Andrea Arcangeli <andrea@suse.de>

Woops, I missed that. If so I propose also this incremental patch to
improve the scalability of the code:

--- 2.3.18ac6/include/net/sock.h Mon Sep 20 18:06:40 1999
+++ /tmp/sock.h Mon Sep 20 20:10:49 1999
@@ -672,8 +672,8 @@
(__sk)->lock.users = 0; \
if ((__sk)->backlog.tail != NULL) \
__release_sock(__sk); \
- wake_up(&((__sk)->lock.wq)); \
spin_unlock_bh(&((__sk)->lock.slock)); \
+ wake_up(&((__sk)->lock.wq)); \
} while(0)

/* BH context may only use the following locking interface. */

No, you have decreased the scalability of this code especially in the
case where some other CPU is spinning on the socket lock.

What happens now with your code is that the local cpu dirties the
cache line containing the socket lock, now portions of the socket
structure will travel over to the other cpu and leave the current cpu
for the cache coherency transaction.

The other cpu will dirty the cache line by obtaining the spinlock.

Next the wait queue operations will be performed, moving a cache
line'd portion of the sock structure back to the local processor.

Worse yet, if the local cpu dirties this cache line during the wake up
operation it will be removed from the cache of the other processor due
to cache coherency transaction rules.

In short, this suggested change will almost certainly make performance
worse under high load.

Don't play with this code like this Andrea, Alexey and myself been
thinking about these issues for months now. If you find the deadlock
bug, thats great and please provide more details or even better a fix.

Later,
David S. Miller
davem@redhat.com

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