[PATCH 1/4] Bluetooth: Restore accidentally-deleted line

[PATCH 1/4] Bluetooth: Restore accidentally-deleted line

[Mat Martineau]

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 <mathewm@codeaurora.org>
---
 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

[PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Mat Martineau]

This workqueue will be used for general L2CAP work, not just dealing
with "busy" frames.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 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

[PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Mat Martineau]

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 <mathewm@codeaurora.org>
---
 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

[PATCH 4/4] Bluetooth: Fix check for the ERTM local busy state

[Mat Martineau]

Local busy is encoded in a bitfield, but was not masked out correctly.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Gustavo F. Padovan]

* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 1/4] Bluetooth: Restore accidentally-deleted line

[Gustavo F. Padovan]

* Mat Martineau <mathewm@codeaurora.org> [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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCH 4/4] Bluetooth: Fix check for the ERTM local busy state

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [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 <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Mat Martineau]

On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:

> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Gustavo F. Padovan]

* Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:

> 
> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> 
> >* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Mat Martineau]

On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:

> * Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:
>
>>
>> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
>>
>>> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Mat Martineau]

Gustavo,

On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Mat Martineau]

Hi Gustavo -

On Thu, 9 Jun 2011, Mat Martineau wrote:

>
> Gustavo,
>
> On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:
>
>> Hi Mat,
>> 
>> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-06-09 16:36:29 -0700]:

> 
> Gustavo,
> 
> On Wed, 8 Jun 2011, Gustavo F. Padovan wrote:
> 
> >Hi Mat,
> >
> >* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Gustavo F. Padovan]

* Mat Martineau <mathewm@codeaurora.org> [2011-06-09 16:09:52 -0700]:

> 
> On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> 
> >* Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:
> >
> >>
> >>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> >>
> >>>* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Mat Martineau]

On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:

> * Mat Martineau <mathewm@codeaurora.org> [2011-06-09 16:09:52 -0700]:
>
>>
>> On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
>>
>>> * Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:
>>>
>>>>
>>>> On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
>>>>
>>>>> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 3/4] Bluetooth: Limit depth of the HCI TX queue with ERTM mode

[Mat Martineau]

Gustavo -

On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-06-16 14:48:03 -0700]:

> 
> On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
> 
> >* Mat Martineau <mathewm@codeaurora.org> [2011-06-09 16:09:52 -0700]:
> >
> >>
> >>On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> >>
> >>>* Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:
> >>>
> >>>>
> >>>>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> >>>>
> >>>>>* Mat Martineau <mathewm@codeaurora.org> [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

Re: [PATCH 2/4] Bluetooth: Rename _busy_wq to _l2cap_wq

[Gustavo F. Padovan]

* Gustavo F. Padovan <padovan@profusion.mobi> [2011-06-20 16:23:46 -0300]:

> Hi Mat,
> 
> * Mat Martineau <mathewm@codeaurora.org> [2011-06-16 14:48:03 -0700]:
> 
> > 
> > On Tue, 14 Jun 2011, Gustavo F. Padovan wrote:
> > 
> > >* Mat Martineau <mathewm@codeaurora.org> [2011-06-09 16:09:52 -0700]:
> > >
> > >>
> > >>On Thu, 9 Jun 2011, Gustavo F. Padovan wrote:
> > >>
> > >>>* Mat Martineau <mathewm@codeaurora.org> [2011-06-08 16:24:05 -0700]:
> > >>>
> > >>>>
> > >>>>On Mon, 6 Jun 2011, Gustavo F. Padovan wrote:
> > >>>>
> > >>>>>* Mat Martineau <mathewm@codeaurora.org> [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