Re: The updated socket patch and axing sotryfree() (Re: Locking down a socket, milestone 1)
On Wed, 8 May 2002 09:16:56 -0700, Alfred Perlstein [EMAIL PROTECTED] said: bright * Seigo Tanimura [EMAIL PROTECTED] [020508 04:59] wrote: I would like to commit this patch in one or two weeks to start working on a possible race between a user process and a netisr kthread, prevented by only the Giant lock at the moment. When a user process calls sofree() for a listening socket, it attempts to free the sockets in the connection queues by soabort(). If the connection of an aborting socket gets dropped by a remote host (eg by TCP RST), a netisr kthread also attempts to free the socket. Since the reference count of a socket in a connection queue is zero, this would resust in doubly freeing a socket. To solve that problem, I would like to axe sotryfree(). The PCB of a socket and a connection queue should hold a reference to the socket. This should make the reference count of an alive socket always be = 1, and ensure that there is only one referer to a socket to be freed. Comments? bright I'm not sure exactly how this solves the problem, there may need to bright be a global socket mutex, perhaps putting this sort of operation under bright that may do what you want. Yes, at least, we should introduce a global lock to protect the relation between sockets and PCBs. bright Off the top of my head... bright I think one way of doing it is storing the hashlist that the socket bright belongs to (inpcb_hash) inside the sockets. That way after a lookup bright you will have the lock on the parent structure, a user process will bright have to follow the same locking paradym, basically look at the head bright socket, lock the hashlist, then manipulate the incomplete queue. bright Basically, protect this sort of operation via the hashlist because bright you pretty much need to anyway. :) In order to solve the issue of deallocation race by a hashlist lock, we *always* have to obtain a socket or a PCB by looking up a hashlist. This is quite problematic because: 1. the lock order between a socket and a PCB gets tangled, 2. a hashlist introduces an overhead of calculating a hash index value, and 3. a hashlint lock cannot be per-socket or per-PCB, resulting in a contention under a huge number of socket operations or incoming packets. In order to make our lock strategy readable and comprehensible, we should keep a lock order as simple as the following: 1. a lock to protect the relation between/among objects, (eg the proctree lock or the allproc lock) and 2. a lock dedicated to a single object. (eg a proc lock) A reference count allows us a flexible way to keep a lock order clean. Once we grabbed a reference to an object, we can unlock it completely to restart with locking any lock protecting a relation. For instance, in the interrupt handler of a protocol stack, you lock a hashlist to look up the PCB appropriate to an incoming packet. You then lock the PCB to do some work. If you have to modify the socket corresponding to the PCB, hold a reference to the PCB and unlock it. Now you can lock the relation between sockets and PCBs to grab the socket safely. This strategy should be applicable to a socket operation initiated by a user process as well. We will not have to worry about the lock order between a socket and a PCB. Another advantage of a reference count is its cost. Provided that we hold an appropriate lock, we can simply follow a pointer to obtain an object. This is much cheaper than we calculate a hash index. We can also reduce the contention over a lock because the lock of a reference count is per-socket or per-PCB. bright Other than that, have you looked at what BSD/os does and what Linux bright does? Do they get it wrong or have any particular drawbacks? BSD/OS seems to ensure the existence of a PCB by locking the hashlist of PCBs. I am worrying about the fact that they lock the hashlist for quite a long duration. (about a half of udp_input() hold a read lock, and almost all of tcp_input() hold a *write* lock.) -- Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
On Wed, 24 Apr 2002 20:08:56 +0900, Seigo Tanimura [EMAIL PROTECTED] said: Seigo I am now working on locking down a socket. (I have heard that Jeffrey Seigo Hsu is also doing that, but I have never seen his patch. Has anyone Seigo seen that?) My first milestone patch is now available at: Seigo http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz I ripped off and committed the part of a sigio lock. The updated patch is found at: http://people.FreeBSD.org/~tanimura/patches/socket_milestone1a.diff.gz -- Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
On Sun, 28 Apr 2002, Seigo Tanimura wrote: On Wed, 24 Apr 2002 20:08:56 +0900, Seigo Tanimura [EMAIL PROTECTED] said: Seigo I am now working on locking down a socket. (I have heard that Jeffrey Seigo Hsu is also doing that, but I have never seen his patch. Has anyone Seigo seen that?) My first milestone patch is now available at: Seigo http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz I ripped off and committed the part of a sigio lock. The updated patch is found at: The committed part re-adds lots of namespace pollution to sys/filedesc.h and sys/socketvar.h. Please fix this. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
On Wed, 24 Apr 2002 10:18:01 -0400 (EDT), John Baldwin [EMAIL PROTECTED] said: jhb On 24-Apr-2002 Seigo Tanimura wrote: I am now working on locking down a socket. (I have heard that Jeffrey Hsu is also doing that, but I have never seen his patch. Has anyone seen that?) My first milestone patch is now available at: http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz The works I have done so far are: - Determine the lock required to protect each of the members in struct socket. - Add mutexes to each of the sockbufs in a socket as BSD/OS does. - Lock down so_count, so_options, so_linger and so_state. - Add a global mutex socq_lock to protect the connection queues of a listening socket. Lock socq_lock to lock two sockets at once, followed by enqueuing or dequeuing a socket, or moving a socket across queues. socq_lock is not an sx lock because we usually have to lock two sockets to modify them. jhb Do you actually lock two sockets at once or do you lock one at a time while jhb holding socq_lock. If you do lock two at once, what is the defined locking jhb order? At the moment, I lock two sockets at once. This is required, eg in soisconnected() to move an accepting socket across the connection queues of a listening socket. The lock order is: 1. socq_lock 2. an accepting socket 3. a listening socket (in so_head of the accepting socket) -- Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
* Seigo Tanimura [EMAIL PROTECTED] [020425 01:19] wrote: http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz This looks really good so far! Needs some more comments explaining socq_lock. Watch long line wraps. Why is there a sigio lock in this delta? -- -Alfred Perlstein [[EMAIL PROTECTED]] 'Instead of asking why a piece of software is using 1970s technology, start asking why software is ignoring 30 years of accumulated wisdom.' Tax deductible donations for FreeBSD: http://www.freebsdfoundation.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
A sigio lock (was: Re: Locking down a socket, milestone 1)
On Thu, 25 Apr 2002 18:01:51 +0900, Seigo Tanimura [EMAIL PROTECTED] said: Seigo On Thu, 25 Apr 2002 01:36:44 -0700, Seigo Alfred Perlstein [EMAIL PROTECTED] said: bright * Seigo Tanimura [EMAIL PROTECTED] [020425 01:19] wrote: http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz bright Why is there a sigio lock in this delta? Seigo I should have stripped that out, but my patch would result in lock Seigo order reversal between a socket lock and a process lock. As we Seigo protect p_fd by a process lock, we have to lock it prior to locking a Seigo file descriptor or a socket. Seigo I suppose I have to commit the stripped patch of a sigio lock first. The patch of a sigio lock is not stripped out at: http://people.FreeBSD.org/~tanimura/patches/sigio.diff.gz -- Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
* Jeffrey Hsu [EMAIL PROTECTED] [020425 10:50] wrote: In article [EMAIL PROTECTED] you write: I am now working on locking down a socket. (I have heard that Jeffrey Hsu is also doing that, but I have never seen his patch. Has anyone seen that?) I have. :-) I do coarse-grain locking at the inpcb and sockbuf level as is done in BSD/OS. This is a lot simpler than locking down individual fields in struct socket. Are you sure we need such fine-grain locking? Huh? BSD/OS's source drop has two locks in each socket and a couple of global locks for things like the inpcb and such, it's pretty fine grained except the unix domain sockets where a global lock is held to protect against lock order reversals when having to lock both sockets. -- -Alfred Perlstein [[EMAIL PROTECTED]] 'Instead of asking why a piece of software is using 1970s technology, start asking why software is ignoring 30 years of accumulated wisdom.' Tax deductible donations for FreeBSD: http://www.freebsdfoundation.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Locking down a socket, milestone 1
If you compare the two approaches, the BSD/OS approach is simpler because it is coarse. You're confusing finer grain locking with better. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
RE: Locking down a socket, milestone 1
On 24-Apr-2002 Seigo Tanimura wrote: I am now working on locking down a socket. (I have heard that Jeffrey Hsu is also doing that, but I have never seen his patch. Has anyone seen that?) My first milestone patch is now available at: http://people.FreeBSD.org/~tanimura/patches/socket_milestone1.diff.gz The works I have done so far are: - Determine the lock required to protect each of the members in struct socket. - Add mutexes to each of the sockbufs in a socket as BSD/OS does. - Lock down so_count, so_options, so_linger and so_state. - Add a global mutex socq_lock to protect the connection queues of a listening socket. Lock socq_lock to lock two sockets at once, followed by enqueuing or dequeuing a socket, or moving a socket across queues. socq_lock is not an sx lock because we usually have to lock two sockets to modify them. Do you actually lock two sockets at once or do you lock one at a time while holding socq_lock. If you do lock two at once, what is the defined locking order? -- John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/ Power Users Use the Power to Serve! - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
