From owner-freebsd-arch Mon Oct 7 15:45: 7 2002 Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E07DB37B428 for ; Mon, 7 Oct 2002 15:44:53 -0700 (PDT) Received: from mail.speakeasy.net (mail14.speakeasy.net [216.254.0.214]) by mx1.FreeBSD.org (Postfix) with ESMTP id 943A143E3B for ; Mon, 7 Oct 2002 15:44:52 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: (qmail 14220 invoked from network); 7 Oct 2002 22:44:53 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) by mail14.speakeasy.net (qmail-ldap-1.03) with DES-CBC3-SHA encrypted SMTP for ; 7 Oct 2002 22:44:53 -0000 Received: from laptop.baldwin.cx (laptop.baldwin.cx [192.168.0.4]) by server.baldwin.cx (8.12.6/8.12.6) with ESMTP id g97Mipn5003705; Mon, 7 Oct 2002 18:44:51 -0400 (EDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.5.2 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <200210072123.g97LNGvU033246@gw.catspoiler.org> Date: Mon, 07 Oct 2002 18:44:55 -0400 (EDT) From: John Baldwin To: Don Lewis Subject: Re: [jmallett@FreeBSD.org: [PATCH] Reliable signal queues, etc., Cc: jmallett@FreeBSD.ORG, arch@FreeBSD.ORG Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 07-Oct-2002 Don Lewis wrote: > On 7 Oct, John Baldwin wrote: >> >> On 05-Oct-2002 Don Lewis wrote: >>> On 5 Oct, Juli Mallett wrote: >>> >>>> diff -Nrdu -x *CVS* -x *dev* sys/kern/kern_exit.c kernel/kern/kern_exit.c >>>> --- sys/kern/kern_exit.c Tue Oct 1 12:15:51 2002 >>>> +++ kernel/kern/kern_exit.c Sat Oct 5 01:20:57 2002 >>> >>>> @@ -209,12 +210,12 @@ >>>> PROC_LOCK(p); >>>> if (p == p->p_leader) { >>>> q = p->p_peers; >>>> + PROC_UNLOCK(p); >>>> while (q != NULL) { >>>> - PROC_LOCK(q); >>>> psignal(q, SIGKILL); >>>> - PROC_UNLOCK(q); >>>> q = q->p_peers; >>>> } >>>> + PROC_LOCK(p); >>>> while (p->p_peers) >>>> msleep(p, &p->p_mtx, PWAIT, "exit1", 0); >>>> } >>> >>> This scary looking fragment of code in exit1() is relying on the lock on >>> p->p_leader being continuously held to prevent the p_peers list from >>> changing while the list traversal is in progress. The code in >>> kern_fork.c and elsewhere in kern_exit.c holds a lock on p_leader while >>> the list modifications are done. >>> >>> The existing code looks like it could deadlock if q is locked because it >>> is in fork() or exit(). Process p will block when it tries to lock q, >>> and q will block when it tries to lock its p_leader, which happens to be >>> p. >> >> Ugh. Probably the code should be changed to do something like this: >> >> --- kern_exit.c 2 Oct 2002 23:12:01 -0000 1.181 >> +++ kern_exit.c 7 Oct 2002 18:48:18 -0000 >> @@ -203,17 +203,18 @@ >> */ >> >> p->p_flag |= P_WEXIT; >> - PROC_UNLOCK(p); >> >> /* Are we a task leader? */ >> - PROC_LOCK(p); >> if (p == p->p_leader) { >> q = p->p_peers; >> while (q != NULL) { >> + nq = q->p_peers; >> + PROC_UNLOCK(p); >> PROC_LOCK(q); >> psignal(q, SIGKILL); >> PROC_UNLOCK(q); >> - q = q->p_peers; >> + PROC_LOCK(p); >> + q = nq; >> } >> while (p->p_peers) >> msleep(p, &p->p_mtx, PWAIT, "exit1", 0); > > It's not obvious to me that your alternative is safe. It avoids the > deadlock problem, but what keeps the list from changing while it is > being traversed, especially while we're waiting for PROC_LOCK(q)? It > separate lock for the peer list (instead of using PROC_LOCK(p_leader)) > looks like the obvious fix. Grabbing the peer list lock after unlocking > P would avoid the deadlock and allow us to do whatever locking is needed > for psignal(). Hmm, you are right. Yuck. *sigh* I think we need to check P_WEXIT in fork1() for this to really DTRT as well. >> Also, we might should check P_WEXIT and abort in fork1() if it is >> set. (We don't appear to do that presently.) >> > > Probably, but the list is also modified in the exit code. All those > processes that we are sending SIGKILL to are removing themselves from > the list. Processes dieing from SIGKILL that we send them aren't a problem since we have already read their p_peers member before we kill them. That's the point of 'nq'. The problem is that 'nq' could exit and could be an invalid pointer. If a process later in the list after 'nq' died that is not a problem either. Well, how about this: http://www.FreeBSD.org/~jhb/patches/ppeers.patch -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message