Hi Mat, * Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2011-06-16 14:48:03 -0700]: > > On Tue, 14 Jun 2011, Gustavo F. Padovan wrote: > > >* Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2011-06-09 16:09:52 -0700]: > > > >> > >>On Thu, 9 Jun 2011, Gustavo F. Padovan wrote: > >> > >>>* Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2011-06-08 16:24:05 -0700]: > >>> > >>>> > >>>>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote: > >>>> > >>>>>* Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2011-06-03 16:21:08 -0700]: > >>>>> > >>>>>>This workqueue will be used for general L2CAP work, not just dealing > >>>>>>with "busy" frames. > >>>>> > >>>>>which general L2CAP work are we talking about? > >>>> > >>>>I'll update the commit message and resubmit, but to immediately > >>>>answer your question: > >>>> > >>>>* ERTM tx queue callbacks (patch 3/4 in this series) > >>>>* ERTM timers (ack, retrans, monitor) that can utilize proper locking. > >>> > >>>What are the problems with ERTM timers? From what I remember it was not using > >>>reference count on them, I fixed this. Patch is on this mailing for review. > >> > >>The ERTM timer handlers modify l2cap_chan data, but use > >>bh_lock_sock() and bh_unlock_sock() instead of lock_sock() and > >>release_sock(). The socket might be locked in userspace context, > >>but these timers can fire and start executing code concurrently. > >> > >>See http://article.gmane.org/gmane.linux.bluez.kernel/6808 for the > >>full description. > >> > >>With the refactoring of L2CAP to separate out the socket code, we > >>will still need a per-channel lock to protect l2cap_chan data. > > > >I understand. I'm just having a similar problem with the workqueue patch. > > > >> > >>>>* Updating the ERTM tx queue after an AMP channel move and L2CAP > >>>>reconfiguration > >>>>* Moving various AMP-related work out of interrupt context > >>> > >>>Marcel wrote a patch sometime ago the move the whole Bluetooth core work out > >>>of the interrupt context. That will help with many issues we have today, > >>>including these you are talking about. > >>>We need to take that patch, fix it, and push it no bluetooth-next. > >>>This task is on my TODO list, but my TODO list is getting bigger. ;) > >> > >>This would be a big help, do you have a pointer to this patch? > > > >http://permalink.gmane.org/gmane.linux.bluez.kernel/6966 > > > >It only moves the cmd rx task to workqueue, but it has problems, see the > >comments. I took this patches today and changed it to handle all rx events and > >data in an workqueue. But it still has a issue with timers, because those run > >in interrupt context. I just had no time to think on this, but will tomorrow. > > > > Could the timers be changed to use the "delayed_work" workqueue > APIs? Then all of those timeouts would be handled on workqueues and > locking would be much more manageable. Yes, that is the solution I found. If everything runs in process context is better. > > >>Is > >>the new workqueue accessible for all Bluetooth modules, or is it > >>just for the HCI core? The other issues in my list still need a > >>workqueue to use, they are not solved by moving rx processing out of > >>interrupt context. If rx processing gets rid of interrupt context, > >>we'll also want to take a close look at all other timer use. > > > >No, but there is a per controller wokqueue. It was added by f48fd9c8cd746. > >I think it is good idea reuse it. > > I didn't notice that there was already that workqueue for every > hdev, it's definitely better to use that instead of a global l2cap > queue. Please drop this patch. > > In that case, I have an idea for getting rid of _busy_wq. I'll > submit a patch. Great. That will help. Gustavo -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
