Hi,
On 6/16/2010 9:56 PM, Nick Pelly wrote:
On Wed, Jun 16, 2010 at 8:14 AM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
Hi,
On Wed, Jun 16, 2010 at 3:04 PM, Suraj<suraj@xxxxxxxxxxx> wrote:
Hi Luis,
On 6/16/2010 5:10 PM, Luiz Augusto von Dentz wrote:
Hi,
On Tue, Mar 9, 2010 at 11:45 PM, Marcel Holtmann<marcel@xxxxxxxxxxxx>
wrote:
Hi Nick,
Right now Bluez always requests flushable ACL packets (but does not
set a flush timeout, so effectively they are non-flushable):
However it is desirable to use an ACL flush timeout on A2DP packets
so
that if the ACL packets block for some reason then the LM can flush
them to make room for newer packets.
Is it reasonable for Bluez to use the 0x00 ACL packet boundary flag
by
default (non-flushable packet), and let userspace request flushable
packets on A2DP L2CAP sockets with the socket option
L2CAP_LM_RELIABLE.
the reliable option has a different meaning. It comes back from the
old
Bluetooth 1.1 qualification days where we had to tests on L2CAP that
had
to confirm that we can detect malformed packets and report them.
These
days it is just fine to drop them.
Got it, how about introducing
#define L2CAP_LM_FLUSHABLE 0x0040
that l2cap_sock_setsockopt_old() sets this didn't give you a hint that
we might wanna deprecate this socket options ;)
I need to read up on the flushable stuff, but in the end it deserves
its
own socket option. Also an ioctl() to actually trigger Enhanced flush
might be needed.
struct l2cap_pinfo {
...
__u8 flushable;
}
Sure. In the long run we need to turn this into a bitmask. We are just
wasting memory here.
Attached is an updated patch, that checks the LMP features bitmask
before using the new non-flushable packet type.
I am still using L2CAP_LM_FLUSHABLE socket option in
l2cap_sock_setsockopt_old(), which I don't think you are happy with.
So how about a new option:
SOL_L2CAP, L2CAP_ACL_FLUSH
which has a default value of 0, and can be set to 1 to make the ACL
data sent by this L2CAP socket flushable.
In a later commit we would then add
SOL_ACL, ACL_FLUSH_TIMEOUT
That is used to set an automatic flush timeout for the ACL link on a
L2CAP socket. Note that SOL_ACL is new.
But maybe this is not what you had in mind, so I'm looking for your
advice before I implement this.
Attached an updated patch for 2.6.32 kernel. We've been using this
patch successfully on production devices.
can see anything wrong with that patch. However we need to use
SOL_BLUETOOTH for it of course. So we need to come up with something to
make this simple.
An additional change I like to see is to use flags for booleans like
flushable in the structures. Can you work on changing that.
Also do we have decoding support for this in hcidump. It might be nice
to include some really simple examples in the commit message.
Regards
I would like to play a little bit with this, so is there any missing
updates?
This is not exactly something related to your question, but there is another
side effect for the current implementation.
Assume you have 2 ACL links, FTP and A2DP. A2DP streaming and FTP doing FTP
Put.
When the A2DP packets start blocking, it effectively start blocking the
packets available for FTP too. But, the host has no idea about it and keep
pumping in A2DP data until all available buffers are blocked. Effectively
blocking both A2DP and FTP.
So at the user level you will see your FTP connection stalling as long your
A2DP connection is stalled (out of range). FTP will restart as soon as A2DP
comes back in range.
I had raised this issue sometime before, but could not follow it up.
I got the impression that we can still control which packets are
Automatically-Flushable and which are not, so even thought we set the
timeout in a per ACL link fashion we can still mark which packets
should be flushable in a per socket basis.
Is that correct, Nick?
Yes, as Suraj explains, my patch will solve the problem of a stalled
A2DP link preventing FTP packets on another ACL - because the A2DP
packets will be marked flushable.
However its worth bringing up that my patch does not solve the reverse
problem - if an FTP link is stalled then all ACL packets will be
temporarily stalled - because you would not normally mark FTP packets
as flushable. For this you would need the kernel to set a high level
watermark on a per-ACL-link or per-socket basis, so that a single ACL
link or a single socket can not use all available ACL buffers
regardless of whether they are flushable or not.
Nick
Exactly, this was the problem I raised sometime before.
Attached is a patch that works for me.
Not sure if this is the best solution. I can send a formal patch if this
is worth a try.
Regards
Suraj
>From cbcb5fd9d126e490a7b12469a0a1e1fb09ce535b Mon Sep 17 00:00:00 2001
From: root <root@xxxxxxxxxxxxxxxxxxxxx>
Date: Tue, 4 May 2010 13:52:04 +0530
Subject: [PATCH] Fix for Link starvation if device goes out of range
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 5 +++++
net/bluetooth/hci_event.c | 4 ++++
3 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 25b8a03..4216a7b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -178,6 +178,8 @@ struct hci_conn {
unsigned int sent;
+ unsigned int avail_buffer;
+
struct sk_buff_head data_q;
struct timer_list disc_timer;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 406ad07..f8a99cf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1325,6 +1325,8 @@ static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int
if (c->state != BT_CONNECTED && c->state != BT_CONFIG)
continue;
+ if (c->avail_buffer == 0)
+ continue;
num++;
@@ -1390,6 +1392,9 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
hdev->acl_cnt--;
conn->sent++;
+
+ if (conn->avail_buffer != 0)
+ conn->avail_buffer--;
}
}
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e99fe38..2f3b6dc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -900,6 +900,9 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
if (conn->type == ACL_LINK) {
struct hci_cp_read_remote_features cp;
cp.handle = ev->handle;
+
+ conn->avail_buffer = hdev->acl_pkts;
+
hci_send_cmd(hdev, HCI_OP_READ_REMOTE_FEATURES,
sizeof(cp), &cp);
}
@@ -1447,6 +1450,7 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
if (conn->type == ACL_LINK) {
if ((hdev->acl_cnt += count) > hdev->acl_pkts)
hdev->acl_cnt = hdev->acl_pkts;
+ conn->avail_buffer = count;
} else {
if ((hdev->sco_cnt += count) > hdev->sco_pkts)
hdev->sco_cnt = hdev->sco_pkts;
--
1.6.0.6
