Date: Mon, 1 Nov 2010 11:51:22 -0700 From: Weongyo Jeong <weongyo.jeong@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: [CFR 2-3/n] removes uther dependency of axe(4) Message-ID: <20101101185122.GE3918@weongyo> In-Reply-To: <201011010930.26038.hselasky@c2i.net> References: <20101031224304.GB3918@weongyo> <201011010930.26038.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
> > +static void
> > +axe_watchdog(void *arg)
> > +{
> > + struct axe_softc *sc = arg;
> > + struct ifnet *ifp = sc->sc_ifp;
> > +
> > + if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0)
> > + return;
> > +
>
> 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,
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.
> 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:
- Please fill.
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.
> 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.
regards,
Weongyo Jeong
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101101185122.GE3918>