When code was moved from l2cap_core.c to l2cap_sock.c in commit
6de0702b5b93da0ef097aa092b4597fbc024ebba, one line was dropped
from the old __l2cap_sock_close() implementation. This sk_state
change should still be in l2cap_chan_close().
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 15e6a53..dc5c654 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -435,6 +435,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
result = L2CAP_CR_SEC_BLOCK;
else
result = L2CAP_CR_BAD_PSM;
+ sk->sk_state = BT_DISCONN;
rsp.scid = cpu_to_le16(chan->dcid);
rsp.dcid = cpu_to_le16(chan->scid);
--
1.7.5.2
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
* Gustavo F. Padovan <[email protected]> [2011-06-20 16:23:46 -0300]:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2011-06-16 14:48:03 -0700]:
>
> >
> > On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
> >
> > >* Mat Martineau <[email protected]> [2011-06-09 16:09:52 -0700]:
> > >
> > >>
> > >>On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> > >>
> > >>>* Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
> > >>>
> > >>>>
> > >>>>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> > >>>>
> > >>>>>* Mat Martineau <[email protected]> [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.
Btw, my latest patches on this are here
http://git.kernel.org/?p=linux/kernel/git/padovan/bluetooth-testing.git;a=summary
But they are ready for merge yet.
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2011-06-16 14:48:03 -0700]:
>
> On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
>
> >* Mat Martineau <[email protected]> [2011-06-09 16:09:52 -0700]:
> >
> >>
> >>On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> >>
> >>>* Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
> >>>
> >>>>
> >>>>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> >>>>
> >>>>>* Mat Martineau <[email protected]> [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
Gustavo -
On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2011-06-09 16:36:29 -0700]:
>
...
>> So, I think we have two options:
>>
>> * Use the skb_destructor mechanism to pull data for ERTM (which is
>> what my patch does), and leave queuing for other modes alone
>> * Rearchitect HCI & L2CAP so that data is pulled from the L2CAP
>> layer as num_comp_pkts events are received
>
> I prefer the rearchitect HCI and L2CAP approach, but I still don't have too
> much idea on how to do this. I'm open to suggestions here.
My suggestion would be:
* Run HCI tx on a workqueue
* Remove the hci_conn data_q
* Use a tx queue in each l2cap_chan for all L2CAP modes
* L2CAP sends put data in the l2cap_chan tx queue and hci_send_acl
informs the hci layer that there's a channel to pull from
* When the HCI device can accept ACL frames (packets are available or
num_completed_packets is received), instead of having the hci
scheduler pull data from hci_conn queues, it calls back to L2CAP to
pull from l2cap_chan tx queues. ACL data goes directly from the L2CAP
tx queue to the HCI device.
Pulling data for ERTM would require locking (while the control bits
are set), but basic mode and streaming mode probably would not need
locking in the L2CAP callback.
There are several benefits: fewer layers of queuing, improved fairness
between L2CAP channels on the same ACL, and just one workqueue instead
of a workqueue and a tasklet.
> Another idea is to delay in setting the control bits until the moment we
> actually send the frame. If we do that, we can have priority for REJ/SREJ.
> Before send, hci calls a l2cap function to fill the control bit. These are
> just ideas.
This very similar to what my patch does :). When frames can be sent,
a job runs on the workqueue to set the control bits and push the
frames to the hci_conn data_q. However, there is not a callback for
every frame in my patch. Several frames are sent during each callback
to reduce overhead.
> I just think that use an workqueue and a tasklet to send packet is just too
> much.
It seems to work very well with AMP on mobile devices. Keep in mind
that there are more dropped packets on AMP than on BR/EDR, so quick
handling of REJ/SREJ becomes important for maintaining throughput.
ERTM flow control (local busy) works a lot better too.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
> * Mat Martineau <[email protected]> [2011-06-09 16:09:52 -0700]:
>
>>
>> On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
>>
>>> * Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
>>>
>>>>
>>>> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
>>>>
>>>>> * Mat Martineau <[email protected]> [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.
>> 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.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
* Mat Martineau <[email protected]> [2011-06-09 16:09:52 -0700]:
>
> On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
>
> >* Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
> >
> >>
> >>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> >>
> >>>* Mat Martineau <[email protected]> [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.
> 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.
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2011-06-09 16:36:29 -0700]:
>
> Gustavo,
>
> On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:
>
> >Hi Mat,
> >
> >* Mat Martineau <[email protected]> [2011-06-03 16:21:09 -0700]:
> >
> >>In order to provide timely responses to REJ, SREJ, and poll input from
> >>the remote device, it helps to reduce the number of ERTM data frames
> >>in the HCI TX queue at one time. If a full TX window of data is in the
> >>HCI TX queue, any responses to REJ, SREJ, or polls must wait in line
> >>behind all previously queued data. This latency leads to disconnects,
> >>and will be more severe with extended window sizes.
> >
> >I prefer if we go with a hci_send_acl_prio() implementation. It will have much
> >less overhead using a workqueue. As it will be filled only by S-frames with a
> >few bytes each I don't think we will have problems. So lets go with this
> >approach and see what we can get.
>
> I considered that approach too, but it breaks some major assumptions
> and I don't think it complies with the ERTM spec. I-frames contain
> reqseq fields and a final bit, so if S-frames and I-frames are
> delivered out-of-sequence, you can easily end up with a confusing
> series of reqseq values at the receiver.
>
> Suppose the HCI tx queue is full of I-frames, and the oldest I-frame
> has reqseq set to 1. Since that I-frame has been queued, other
> incoming I-frames have been processed, so the last recieved I-frame
> had txseq 20. The remote device sends a poll, and we reply with an
> RR (reqseq 21) using the priority queue. HCI sends that RR first,
> then an I-frame from the normal queue with reqseq 1. Now the remote
> side thinks it missed all of the frames from 21 to 1 (having wrapped
> around). The remote side then has to send REJ or SREJ frames, even
> though nothing is actually missing.
Good point. That will be a problem indeed.
>
> So, I think we have two options:
>
> * Use the skb_destructor mechanism to pull data for ERTM (which is
> what my patch does), and leave queuing for other modes alone
> * Rearchitect HCI & L2CAP so that data is pulled from the L2CAP
> layer as num_comp_pkts events are received
I prefer the rearchitect HCI and L2CAP approach, but I still don't have too
much idea on how to do this. I'm open to suggestions here.
Another idea is to delay in setting the control bits until the moment we
actually send the frame. If we do that, we can have priority for REJ/SREJ.
Before send, hci calls a l2cap function to fill the control bit. These are
just ideas.
I just think that use an workqueue and a tasklet to send packet is just too
much.
Gustavo
Hi Gustavo -
On Thu, 9 Jun 2011, Mat Martineau wrote:
>
> Gustavo,
>
> On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:
>
>> Hi Mat,
>>
>> * Mat Martineau <[email protected]> [2011-06-03 16:21:09 -0700]:
>>
>>> In order to provide timely responses to REJ, SREJ, and poll input from
>>> the remote device, it helps to reduce the number of ERTM data frames
>>> in the HCI TX queue at one time. If a full TX window of data is in the
>>> HCI TX queue, any responses to REJ, SREJ, or polls must wait in line
>>> behind all previously queued data. This latency leads to disconnects,
>>> and will be more severe with extended window sizes.
>>
>> I prefer if we go with a hci_send_acl_prio() implementation. It
>> will have much less overhead using a workqueue. As it will be
>> filled only by S-frames with a few bytes each I don't think we will
>> have problems. So lets go with this approach and see what we can
>> get.
>
> I considered that approach too, but it breaks some major assumptions and I
> don't think it complies with the ERTM spec. I-frames contain reqseq fields
> and a final bit, so if S-frames and I-frames are delivered out-of-sequence,
> you can easily end up with a confusing series of reqseq values at the
> receiver.
>
> Suppose the HCI tx queue is full of I-frames, and the oldest I-frame has
> reqseq set to 1. Since that I-frame has been queued, other incoming I-frames
> have been processed, so the last recieved I-frame had txseq 20. The remote
> device sends a poll, and we reply with an RR (reqseq 21) using the priority
> queue. HCI sends that RR first, then an I-frame from the normal queue with
> reqseq 1. Now the remote side thinks it missed all of the frames from 21 to
> 1 (having wrapped around). The remote side then has to send REJ or SREJ
> frames, even though nothing is actually missing.
>
>
> So, I think we have two options:
>
> * Use the skb_destructor mechanism to pull data for ERTM (which is what my
> patch does), and leave queuing for other modes alone
> * Rearchitect HCI & L2CAP so that data is pulled from the L2CAP layer as
> num_comp_pkts events are received
>
>
> I realize there is increased overhead to make the callbacks to get data out
> of the ERTM tx queue, but the skb destructor is very lightweight (since it
> uses an atomic_t counter). The overhead is tunable using
> L2CAP_MAX_ERTM_QUEUED and L2CAP_MIN_ERTM_QUEUED to control how often the
> callback to l2cap_ertm_send() is actually made. With the current queuing
> behavior, things get unmanageable on AMP with extra latency from larger tx
> windows and much shorter timeouts.
>
Just pinging you regarding the ERTM tx queuing questions. Please let
me know what I can do!
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Gustavo,
On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2011-06-03 16:21:09 -0700]:
>
>> In order to provide timely responses to REJ, SREJ, and poll input from
>> the remote device, it helps to reduce the number of ERTM data frames
>> in the HCI TX queue at one time. If a full TX window of data is in the
>> HCI TX queue, any responses to REJ, SREJ, or polls must wait in line
>> behind all previously queued data. This latency leads to disconnects,
>> and will be more severe with extended window sizes.
>
> I prefer if we go with a hci_send_acl_prio() implementation. It will have much
> less overhead using a workqueue. As it will be filled only by S-frames with a
> few bytes each I don't think we will have problems. So lets go with this
> approach and see what we can get.
I considered that approach too, but it breaks some major assumptions
and I don't think it complies with the ERTM spec. I-frames contain
reqseq fields and a final bit, so if S-frames and I-frames are
delivered out-of-sequence, you can easily end up with a confusing
series of reqseq values at the receiver.
Suppose the HCI tx queue is full of I-frames, and the oldest I-frame
has reqseq set to 1. Since that I-frame has been queued, other
incoming I-frames have been processed, so the last recieved I-frame
had txseq 20. The remote device sends a poll, and we reply with an RR
(reqseq 21) using the priority queue. HCI sends that RR first, then
an I-frame from the normal queue with reqseq 1. Now the remote side
thinks it missed all of the frames from 21 to 1 (having wrapped
around). The remote side then has to send REJ or SREJ frames, even
though nothing is actually missing.
So, I think we have two options:
* Use the skb_destructor mechanism to pull data for ERTM (which is
what my patch does), and leave queuing for other modes alone
* Rearchitect HCI & L2CAP so that data is pulled from the L2CAP layer
as num_comp_pkts events are received
I realize there is increased overhead to make the callbacks to get
data out of the ERTM tx queue, but the skb destructor is very
lightweight (since it uses an atomic_t counter). The overhead is
tunable using L2CAP_MAX_ERTM_QUEUED and L2CAP_MIN_ERTM_QUEUED to
control how often the callback to l2cap_ertm_send() is actually made.
With the current queuing behavior, things get unmanageable on AMP with
extra latency from larger tx windows and much shorter timeouts.
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> * Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
>
>>
>> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
>>
>>> * Mat Martineau <[email protected]> [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.
>> * 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? 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.
Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
* Mat Martineau <[email protected]> [2011-06-08 16:24:05 -0700]:
>
> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
>
> >* Mat Martineau <[email protected]> [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.
> * 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. ;)
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2011-06-03 16:21:09 -0700]:
> In order to provide timely responses to REJ, SREJ, and poll input from
> the remote device, it helps to reduce the number of ERTM data frames
> in the HCI TX queue at one time. If a full TX window of data is in the
> HCI TX queue, any responses to REJ, SREJ, or polls must wait in line
> behind all previously queued data. This latency leads to disconnects,
> and will be more severe with extended window sizes.
I prefer if we go with a hci_send_acl_prio() implementation. It will have much
less overhead using a workqueue. As it will be filled only by S-frames with a
few bytes each I don't think we will have problems. So lets go with this
approach and see what we can get.
Gustavo
On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> * Mat Martineau <[email protected]> [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.
* Updating the ERTM tx queue after an AMP channel move and L2CAP
reconfiguration
* Moving various AMP-related work out of interrupt context
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
* Mat Martineau <[email protected]> [2011-06-03 16:21:10 -0700]:
> Local busy is encoded in a bitfield, but was not masked out correctly.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Applied, thanks.
Gustavo
* Mat Martineau <[email protected]> [2011-06-03 16:21:07 -0700]:
> When code was moved from l2cap_core.c to l2cap_sock.c in commit
> 6de0702b5b93da0ef097aa092b4597fbc024ebba, one line was dropped
> from the old __l2cap_sock_close() implementation. This sk_state
> change should still be in l2cap_chan_close().
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
Applied, thanks.
Gustavo
* Mat Martineau <[email protected]> [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?
Gustavo
Local busy is encoded in a bitfield, but was not masked out correctly.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d167edd..6d7c5d8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3567,7 +3567,7 @@ static inline int l2cap_data_channel_iframe(struct l2cap_chan *chan, u16 rx_cont
goto drop;
}
- if (chan->conn_state == L2CAP_CONN_LOCAL_BUSY)
+ if (chan->conn_state & L2CAP_CONN_LOCAL_BUSY)
goto drop;
if (chan->conn_state & L2CAP_CONN_SREJ_SENT) {
--
1.7.5.2
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
In order to provide timely responses to REJ, SREJ, and poll input from
the remote device, it helps to reduce the number of ERTM data frames
in the HCI TX queue at one time. If a full TX window of data is in the
HCI TX queue, any responses to REJ, SREJ, or polls must wait in line
behind all previously queued data. This latency leads to disconnects,
and will be more severe with extended window sizes.
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/l2cap.h | 5 +++++
net/bluetooth/l2cap_core.c | 31 +++++++++++++++++++++++++++++--
3 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index af930a3..ad8ab1c 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -158,6 +158,7 @@ struct bt_skb_cb {
__u8 sar;
unsigned short channel;
__u8 force_active;
+ struct l2cap_chan *chan;
};
#define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 0529d27..ac87965 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -39,6 +39,8 @@
#define L2CAP_DEFAULT_ACK_TO 200
#define L2CAP_LOCAL_BUSY_TRIES 12
#define L2CAP_LE_DEFAULT_MTU 23
+#define L2CAP_MAX_ERTM_QUEUED 5
+#define L2CAP_MIN_ERTM_QUEUED 2
#define L2CAP_CONN_TIMEOUT (40000) /* 40 seconds */
#define L2CAP_INFO_TIMEOUT (4000) /* 4 seconds */
@@ -341,6 +343,8 @@ struct l2cap_chan {
__u8 remote_max_tx;
__u16 remote_mps;
+ atomic_t ertm_queued;
+
struct timer_list chan_timer;
struct timer_list retrans_timer;
struct timer_list monitor_timer;
@@ -350,6 +354,7 @@ struct l2cap_chan {
struct sk_buff_head srej_q;
struct sk_buff_head busy_q;
struct work_struct busy_work;
+ struct work_struct tx_work;
struct list_head srej_l;
struct list_head list;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6f9daf8..d167edd 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1127,7 +1127,8 @@ int __l2cap_wait_ack(struct sock *sk)
int timeo = HZ/5;
add_wait_queue(sk_sleep(sk), &wait);
- while ((chan->unacked_frames > 0 && chan->conn)) {
+ while (chan->unacked_frames > 0 && chan->conn &&
+ atomic_read(&chan->ertm_queued)) {
set_current_state(TASK_INTERRUPTIBLE);
if (!timeo)
@@ -1292,6 +1293,16 @@ static void l2cap_retransmit_one_frame(struct l2cap_chan *chan, u8 tx_seq)
l2cap_do_send(chan, tx_skb);
}
+static void l2cap_skb_destructor(struct sk_buff *skb)
+{
+ struct l2cap_chan *chan = bt_cb(skb)->chan;
+ int queued;
+
+ queued = atomic_sub_return(1, &chan->ertm_queued);
+ if (queued < L2CAP_MIN_ERTM_QUEUED)
+ queue_work(_l2cap_wq, &chan->tx_work);
+}
+
int l2cap_ertm_send(struct l2cap_chan *chan)
{
struct sk_buff *skb, *tx_skb;
@@ -1302,7 +1313,8 @@ int l2cap_ertm_send(struct l2cap_chan *chan)
if (sk->sk_state != BT_CONNECTED)
return -ENOTCONN;
- while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
+ while ((skb = chan->tx_send_head) && !l2cap_tx_window_full(chan) &&
+ atomic_read(&chan->ertm_queued) < L2CAP_MAX_ERTM_QUEUED) {
if (chan->remote_max_tx &&
bt_cb(skb)->retries == chan->remote_max_tx) {
@@ -1331,6 +1343,10 @@ int l2cap_ertm_send(struct l2cap_chan *chan)
put_unaligned_le16(fcs, skb->data + tx_skb->len - 2);
}
+ bt_cb(tx_skb)->chan = chan;
+ tx_skb->destructor = l2cap_skb_destructor;
+ atomic_inc(&chan->ertm_queued);
+
l2cap_do_send(chan, tx_skb);
__mod_retrans_timer();
@@ -1354,6 +1370,16 @@ int l2cap_ertm_send(struct l2cap_chan *chan)
return nsent;
}
+static void l2cap_ertm_tx_worker(struct work_struct *work)
+{
+ struct l2cap_chan *chan =
+ container_of(work, struct l2cap_chan, tx_work);
+
+ lock_sock(chan->sk);
+ l2cap_ertm_send(chan);
+ release_sock(chan->sk);
+}
+
static int l2cap_retransmit_frames(struct l2cap_chan *chan)
{
int ret;
@@ -1870,6 +1896,7 @@ static inline void l2cap_ertm_init(struct l2cap_chan *chan)
INIT_LIST_HEAD(&chan->srej_l);
INIT_WORK(&chan->busy_work, l2cap_busy_work);
+ INIT_WORK(&chan->tx_work, l2cap_ertm_tx_worker);
sk->sk_backlog_rcv = l2cap_ertm_data_rcv;
}
--
1.7.5.2
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
This workqueue will be used for general L2CAP work, not just dealing
with "busy" frames.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index dc5c654..6f9daf8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -60,7 +60,7 @@ int disable_ertm;
static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
static u8 l2cap_fixed_chan[8] = { 0x02, };
-static struct workqueue_struct *_busy_wq;
+static struct workqueue_struct *_l2cap_wq;
static LIST_HEAD(chan_list);
static DEFINE_RWLOCK(chan_list_lock);
@@ -3351,7 +3351,7 @@ static int l2cap_push_rx_skb(struct l2cap_chan *chan, struct sk_buff *skb, u16 c
del_timer(&chan->ack_timer);
- queue_work(_busy_wq, &chan->busy_work);
+ queue_work(_l2cap_wq, &chan->busy_work);
return err;
}
@@ -4398,8 +4398,8 @@ int __init l2cap_init(void)
if (err < 0)
return err;
- _busy_wq = create_singlethread_workqueue("l2cap");
- if (!_busy_wq) {
+ _l2cap_wq = create_singlethread_workqueue("l2cap");
+ if (!_l2cap_wq) {
err = -ENOMEM;
goto error;
}
@@ -4421,7 +4421,7 @@ int __init l2cap_init(void)
return 0;
error:
- destroy_workqueue(_busy_wq);
+ destroy_workqueue(_l2cap_wq);
l2cap_cleanup_sockets();
return err;
}
@@ -4430,8 +4430,8 @@ void l2cap_exit(void)
{
debugfs_remove(l2cap_debugfs);
- flush_workqueue(_busy_wq);
- destroy_workqueue(_busy_wq);
+ flush_workqueue(_l2cap_wq);
+ destroy_workqueue(_l2cap_wq);
if (hci_unregister_proto(&l2cap_hci_proto) < 0)
BT_ERR("L2CAP protocol unregistration failed");
--
1.7.5.2
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum