From owner-freebsd-current@FreeBSD.ORG Fri Jan 23 20:44:38 2009 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 515C31065673; Fri, 23 Jan 2009 20:44:38 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe13.tele2.se [212.247.155.129]) by mx1.freebsd.org (Postfix) with ESMTP id 25D3F8FC21; Fri, 23 Jan 2009 20:44:36 +0000 (UTC) (envelope-from hselasky@c2i.net) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=nTib0wbUE7AA:10 a=J2nnhgZB8JEA:10 a=6I5d2MoRAAAA:8 a=F4Xq1ygv2EPCaWVc8pUA:9 a=NPho0j7jtTSMCwuEI10A:7 a=7_GJtV10YOjpX6Wx9yM-mxKt1VYA:4 a=9aOQ2cSd83gA:10 a=50e4U0PicR4A:10 Received: from [85.19.218.115] (account mc467741@c2i.net HELO [10.37.1.92]) by mailfe13.swip.net (CommuniGate Pro SMTP 5.2.6) with ESMTPA id 795311138; Fri, 23 Jan 2009 21:44:35 +0100 From: Hans Petter Selasky To: freebsd-current@freebsd.org Date: Fri, 23 Jan 2009 21:46:55 +0100 User-Agent: KMail/1.9.7 References: <20090123154336.GJ60948@e.0x20.net> <200901232029.10538.hselasky@c2i.net> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200901232146.58360.hselasky@c2i.net> Cc: Lars Engels , Alfred Perlstein , current@freebsd.org, Maksim Yevmenkin Subject: Re: panic: mutex Giant not owned at /usr/src/sys/kern/tty_ttydisc.c:1127 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jan 2009 20:44:39 -0000 Hi Maksim, On Friday 23 January 2009, Maksim Yevmenkin wrote: > On Fri, Jan 23, 2009 at 11:29 AM, Hans Petter Selasky wrote: > > Hi Maksim, > > > > First of all I've made some patches which try to tidy up the USB blutooth > > driver. Please see: > > > > http://perforce.freebsd.org/chv.cgi?CH=156577 > > http://perforce.freebsd.org/chv.cgi?CH=156579 > > thanks! unfortunately, i have few problems with those patches. please > read below. > > >> that should not be needed (in theory) but i will add it. > > > > Yes! We are multithreaded! > > please tell me that you read the blob about locking strategy at the > top of ng_ubt2.c file. when ubt_task() is pending, it holds a > reference to the netgraph node. that means the pointer is still > pointing to a valid structure, but the node is marked as "dead" and > NG_NODE[_NOT]_VALID() macros can be use to check for that. so, even if > task is still pending while the rest of ng_ubt2 stuff is gone, it does > not matter because task holds netgraph node. so the next time task is > executed it will see that the node is "dead", release the node and > simply return doing nothing. This kind of complexity is unneccesary. We should be able to drain a node, or sleep until a node is released. 0) stop all USB activity 1) free node 2) wait until the node is actually freed 3) free USB resources which might access node fields Conclusion: No checks are required inside any of the USB callbacks. > > >> > 2) You drain all USB transfers: "usb2_transfer_drain()" called > >> > unlocked. > >> > >> yes, usb2_transfer_unsetup() does that, does it not? > > > > That is correct, but sometimes you need to stop the transfers at an > > earlier point. It depends on your design. > > when detach() is called its already out of driver's hands. all it can > do now is to call unsetup(), or am i missing something here? The correct thing is to do a usb2_transfer_drain() which will wait until the callback has been called back, if a transfer is pending. In USB2 a "usb2_start_hardware()" call is always paired with a USB transfer callback, no matter what you do! This way of design has been chosen so that you can do refcounts inside the callback! > > I see in your code that you try to do things Asynchronously. This > > complicates stuff quite a lot. In my patches I've tried to do some things > > synchronously. > > no, No, NO (sorry for shouting). we _CAN_NOT_ sleep in netgraph. > period. you _CAN_NOT_ grab usb interface mutexes unless you can > absolutely guarantee that they will not sleep for any significant > amount of time. usb2_transfer_start() and usb2_transfer_stop() are non-blocking and will never sleep. The exception is "usb2_transfer_drain()" which can sleep for a few milliseconds and is only called when the hook is disconnected. I do not see that as a problem versus having "X" more checks in the code. > netgraph is essentially single threaded and by > sleeping you will stall entire netgraph subsystem and not just current > thread. If we sleep 10 milliseconds on a UBT hook disconnect, is that a big problem? How often will UBT hook disconnect be called? > > so, objection #1 (the biggest one) is: please leave sc_mbufq_mtx and > ubt_task() glue in place. it is needed as far as i can tell. What I see is that USB already has a separate process handling the callback so that when you call "usb2_transfer_start()" this other process is woken up. Are you sure that there is a measurable gain using "ubt_task()" versus the way I'm doing it? > > ok, that makes sense. except the case when hardware is gone and > transfer callback is called with IOERROR (or something like that). i > guess the question is can callback be called with IOERROR for any > reason other than hardware departure? Yes, if there is interference on the cable. It is quite valid to setup an USB transaction after that the hardware has dissappeared, in xxx_detach() for example. Remeber USB is a polling bus. If the device is not there (I.E. not responds) the USB transfer will simply terminate, either immediately or after a timeout. > > objection #2 (somewhat major). please do NOT remove NG_NODE_REF() call > in detach() before calling ng_rmnode_self() (unless you remove > NG_NODE_UNREF() below as well). again the reason here is to tell > netgraph node that we are dying, but make sure we keep node_p pointer > pointing to a valid structure as long as possible. to summarize, We are already holding a reference to the node from ubt_attach(), right? Why do we need another node reference? > > NG_NODE_REF(node) <- grab our reference to ensure node pointer remains > valid ng_rmnode_self(node) <- tell node we are dying, mark node as "dead" > but NOT free node structure > /* do other stuff */ > NG_NODE_UNREF(node) <- drop our reference and possibly free node pointer > > > more of a question than objection. why you insist on having single > mutex for both interfaces? isoc transfers callbacks will be called > much more often that control, bulk and interrupt callbacks, so > wouldn't it make sense to use different lock for isoc transfers? Because a single mutex is much easier to maintain and I see no gain fine graining more for a bluetooth device :-) > changes in attach() and usb2_config structures are fine (i guess). but > please keep asynchronous design in place. it is required. Is it possible we can make a compromise here? From my experience synchronous code is smaller and easier to understand than asynchronous code! The only disadvantage about synchronous code is that more threads might be required. And hence Netgraph is single threaded other signalling might be delayed, but is that a real problem if we are talking about delays around 1x10ms ever time you disconnect the hook of an UBT node? --HPS