Date: Mon, 1 Nov 2010 21:09:03 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: Weongyo Jeong <weongyo@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: [CFR 2-3/n] removes uther dependency of axe(4) Message-ID: <201011012109.04021.hselasky@c2i.net> In-Reply-To: <20101101185122.GE3918@weongyo> References: <20101031224304.GB3918@weongyo> <201011010930.26038.hselasky@c2i.net> <20101101185122.GE3918@weongyo>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday 01 November 2010 19:51:22 Weongyo Jeong wrote: > On Mon, Nov 01, 2010 at 09:30:25AM +0100, Hans Petter Selasky wrote: > > On Sunday 31 October 2010 23:43:04 Weongyo Jeong wrote: > > > > Hi, > > > > Please explain what is wrong with the existing code regarding code > > synchronisation. Your patch is very big and is likely to introduce > > problems. > > Hello Hans, Hi Weongyo, > > I think your approach to synchronize the multiple tasks isn't bad though > the implementation of approach isn't familiar with me comparing the > other task queue implementations. It seems to me that it's a customized > task queue implementation only for USB subsystem. Yes. I'm going to change that now, so that we get all the benefits of the taskqueue system. Please see other e-mail posted today. > > I oppose the introduction of SX-locks. Please explain why you think > > SX-locks are better than the USB process taskqueue. > > I'd like to say that I also don't like SX locks that the reason is > mentioned at CONTEXT section of sx(9) man page. > > I think using SX lock and the USB process taskqueue are both bad that if > I could avoid using it, I'll willing to avoid it to use it. But the > problem is that it's inevitable. I think we could choice one of two > approaches that one is using SX lock other is using USB process > taskqueue. The result of both would same, serialization. > > Regarding to the approach there'll be pros and cons: > > Pros of using SX lock: > - Don't need to create extra thread. > - Simple to read. Other developers could catch writer's intention > easily. > > Cons of using SX lock: > - Unexpecting sleep with holding mutex according to CONTEXT section > of sx(9) man page. It should be used carefully. > - Adds another lock to the driver. > > Pros of using USB process taksqueue: The USB process taskqueue is a temporary solution until the taskqueue system is updated to support the "USB requirements". Are kernel taskqueues any better than USB process'es? > Cons of using USB process taskqueue: > - No atomic operation that it need to be drained explicitly with > sleeping that adds extra quirk code. > - Tasks would be merged into one if the task is enqueued multiple > times during pending that it means the writer takes care extra > exceptions. It could lead to unexpected device behavior depending > on device characteristics. > - At least two threads are involved. One is caller thread other > would be worker thread (USB process). It makes weak to race > conditions. It always make code complex. > > > Are you absolutely sure that all the IOCTL's that are called are allowed > > to block in the way you have programmed? > > Of course not because I'm a human. Human always do a mistake. :-) But > there are few cases which need to be atomic in USB devices. For > example, device up and down. It means sx(9) lock would be used very > limited places only. Ok. My intention was not to completely tear apart your patches :-) What I can say is that I've tried to keep all corner cases tight, and maybe it's not directly obvious to everyone how everything is working inside the USB stack like you write. Sometimes I even forget all of the small things I've fixed, and then it takes some time until I remember and can point out to you exactly why. If a USB task/process gets stuck (starts having USB timeouts), then it's easier to recover than if we are using a SX lock. Because if we are using an SX lock, the caller can get stuck for a substantial amount of time, whilst if we do it via task queue, we have the possibility to recover from the USB explore thread which runs the detach of the USB device. > > The checks in xxx_watchdog() are not good enough. axe_tick() will execute > > synchronous USB functions, which sleep for many hundreds of microseconds. > > You should add this check before the sleepout_reset() too, and is this > > code called with any lock locked? I.E. Are you doing the clearing of > > IFF_DRV_RUNNING atomic to testing this flag? Else the result can be > > random? > > Maybe xxx_watchdog() would be called with holding the driver lock so > at least ifp->if_drv_flags would be protected. That's also a solution. --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011012109.04021.hselasky>