From owner-cvs-all Tue Nov 14 22:53:28 2000 Delivered-To: cvs-all@freebsd.org Received: from mass.osd.bsdi.com (adsl-63-206-90-77.dsl.snfc21.pacbell.net [63.206.90.77]) by hub.freebsd.org (Postfix) with ESMTP id D4E8D37B4CF; Tue, 14 Nov 2000 22:53:21 -0800 (PST) Received: from mass.osd.bsdi.com (localhost [127.0.0.1]) by mass.osd.bsdi.com (8.11.0/8.11.1) with ESMTP id eAF6wPF01843; Tue, 14 Nov 2000 22:58:25 -0800 (PST) (envelope-from msmith@mass.osd.bsdi.com) Message-Id: <200011150658.eAF6wPF01843@mass.osd.bsdi.com> X-Mailer: exmh version 2.1.1 10/15/1999 To: Jake Burkholder Cc: John Baldwin , cvs-all@FreeBSD.org, cvs-committers@FreeBSD.org, jake@io.yi.org Subject: Re: cvs commit: src/sys/sys eventhandler.h In-reply-to: Your message of "Tue, 14 Nov 2000 22:37:08 PST." <20001115063708.614A0BA7A@io.yi.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Tue, 14 Nov 2000 22:58:25 -0800 From: Mike Smith Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG > > > This is wrong and should be backed out; the tsleep() in the handler is > > > the bug. You can't release the lock on the list while you're traversing > > > it, since the traversal is holding private state which has to remain > > > consistent with the list. > > > > > > If you want to release the lock on the list, you would have to detect > > > when the list is changed and rescan it to find the 'new' current > > > location. Only that doesn't work either, since the list traversal allows > > > you to remove the current handler from the list. > > > > > > Please revert this change and fix eventhandler clients so that they don't > > > sleep. > > > > Hrm, I guess I could go hard-code all the shutdown events to not use tsleep but > > to use delay()'s. This means hacking up or duplicating things like > > suspend_kproc(). :(. You shouldn't be holding a lock while you are calling > > functions that aren't related to the resource you are holding. Grrr. Would > > it be feasible to say that an eventhandler may not modify a list it is on? > > Perhaps extending the eventhandler interface to allow for once-only events > > would be sufficient. > > The timeout code deals with a similar situation. It saves > the next element in the list in a global variable, nextsoftcheck, > spls down, calls the timeout handler, spls back up and uses nextsoftcheck > as the next element. If untimeout (callout_stop) finds that nextsoftcheck > points to the timeout that's about to be deleted, it advances nextsoftcheck > to point to the next element. > > Wouldn't something similar work? No. Consider an example. The list contains two event handlers, A and B. The eventhandler is invoked twice, 1) and 2) 1) Eventhandler invoked 1) Mutex acquired 1) List traversed, A found, pointer to B saved 1) Mutex released 1) A called 1) A sleeps 2) Eventhandler invoked 2) Mutex acquired 2) List traversed, A found, pointer to B saved 2) Mutex released 2) A called 2) Mutex acquired 2) List traversed, end found 2) Mutex released 2) B called 2) B unlinks self from list 2) Eventhandler ends 1) A wakes up 1) Mutex acquired 1) Dereferences invalid saved pointer to B > Holding the mutex over the whole traversal also means you rely on mutex > recursion in order to be able to deregister from an event handler. True. I've been thinking about Garrett's point about flagging deleted list elements too. I'm actually trying to work out what the issue is with witness; I can only assume that it doesn't like the fact that an eventhandler mutex may be acquired either before or after giant, and this exposes a weakness in witness - that it can't deal with the situation where lock ordering *doesn't* matter. -- ... every activity meets with opposition, everyone who acts has his rivals and unfortunately opponents also. But not because people want to be opponents, rather because the tasks and relationships force people to take different points of view. [Dr. Fritz Todt] V I C T O R Y N O T V E N G E A N C E To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message