Date: Sat, 24 Jan 2009 09:52:20 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-current@freebsd.org Cc: Alfred Perlstein <alfred@freebsd.org>, current@freebsd.org, Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Subject: Re: panic: mutex Giant not owned at /usr/src/sys/kern/tty_ttydisc.c:1127 Message-ID: <200901240952.21670.hselasky@c2i.net> In-Reply-To: <bb4a86c70901231514x7696e457k3a6dc0e59d4daed7@mail.gmail.com> References: <20090123154336.GJ60948@e.0x20.net> <200901232337.05627.hselasky@c2i.net> <bb4a86c70901231514x7696e457k3a6dc0e59d4daed7@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Maksim,
On Saturday 24 January 2009, Maksim Yevmenkin wrote:
> [...]
> i'm sorry, did i mention there is no sleeping in netgraph? :-)
>
> again, sorry, but this is not going to work. you still doing
> UBT_LOCK()/UBT_UNLOCK() in netgraph method. that is you are trying to
> grab the same lock that is used for locking interfaces.
Yes, you have to do that. Else you end up with a state nightmare with the
taskqueue where you don't know in the end if you should start or stop the USB
interface!
> more importantly, like i said before, usb2_transfer_stop() does
> USB_BUS_LOCK()/USB_BUS_UNLOCK(). is there a guarantee that another usb
> device will not do something synchronously over the same usb bus?
Every time something is done synchronously the USB_BUS_LOCK() is dropped. This
mutex is only used when filling out DMA structures and when touching USB
hardware registers. I cannot see that this will take any time at all. We are
talking about microseconds of congestion time! USB_BUS_LOCK() is a mutex and
not a SX lock! A mutex cannot sleep in the same way an SX lock can.
> i'm actually working on including some of your changes (except getting
> rid of ubt_task), so, please, lets just stop for a second and talk
> before we start commit war here :) please bear with me for just a
> second, ok?
Ok.
>
> >
> > I have a question. What is the following doing at the middle of the
> > ubt_detach():
> >
> > if (node != NULL)
> > NG_NODE_UNREF(node);
> >
> > If you say that:
> >
> > ng_rmnode_self(node);
> >
> > Cleans up the last reference?
>
> the complete code is
>
> node_p node = sc->sc_node;
>
> if (node != NULL) {
> sc->sc_node = NULL; <-- clear sc_node
>
> NG_NODE_SET_PRIVATE(node, NULL);
> NG_NODE_REALLY_DIE(node);
>
> NG_NODE_REF(node); <--- grab +1 reference
> ng_rmnode_self(node); <--- mark node as "dead", but not ensure its
> not free()d
> }
>
> /* bla, bla */
>
> if (node != NULL)
> NG_NODE_UNREF(node); <--- drop 1 reference and possibly free() node
Yes, but you are already dropping an extra reference in ubt_shutdown(). What
about that?
> so, say we enter detach() with only one reference (from attach()).
> when we see the sc_node pointer, we assume that there could be
> multiple (> 1) references on it. so just for added protection we grab
> one more (now reference count is 2). then we call ng_rmnode_self()
> which will mark node as "dead" and drop one reference (now reference
> count is 1). then we do "bla bla" stuff which could access node
> pointer. the important thing is that node pointer still points to a
> valid "dead" node, so NG_NODE_NOT_VALID() can be used to check if node
> is "dead" or "alive". finally when "bla bla" stuff is done, we know we
> are not going to access node pointer any mode and we drop our
> reference (now reference count is 0 and node is destroyed).
> do you mean transfer stop on hook disconnection? but those are
> protected by interface locks, no? and like i said, i'm changing
> ubt_task to call drain() instead of stop() to make stop completely
> synchronous (which is actually a nice thing).
I think you misunderstand. I'm using mutexes. They will not block for very
long! There are no DELAY()'s with mutexes held! When another USB device is
attached / detached it is:
1) Done from another thread
2) The USB locks in the critical path are dropped when waiting for wakeup or
doing so called synchronous stuff.
>
> it would be if we could sleep in netgraph, and we can not.
Do mutexes sleep? No? So my code should be fine?
> in theory,
> we only need one extra reference which would tell us if there anything
> in usb2 that still could access node pointer. in practice, instead of
> checking every transfer and making sure its no pending before dropping
> that extra reference i just add multiple references for each usb2
> transfer and ubt_task (i.e. things that could access node pointer).
I'm just thinking that this is starting to get complicated, and please
understand I've spent much time on detach issues, and there is alot of
builtin funcionality inside USB which will handle start/stop/re-start issues
automagically. I see it like duplicate code when you check for USB transfer
re-start in netgraph ???
>
> right, and what if some other usb device attached to the same usb
> controller is doing something synchronously? do you see that you could
> potentially block netgraph for arbitrary time and that is really out
> of ng_ubt2 driver control?
Again, USB_BUS_LOCK() is a mutex, not an SX lock. All code using this lock is
called from High Priority threads! See explanation further up. And will not
block very long at all.
>
> i beg to differ :)
Ok, I think we are going somewhere!
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901240952.21670.hselasky>