[RFC] Bluetooth: HCI H5 fast retransmit

[RFC] Bluetooth: HCI H5 fast retransmit

[Loic Poulain]

HCI H5 protocol is based on timeout to retransmit dropped packets.
According to the specification, this timeout should be calculated
based on the baudrate and the maximum packet size.

In the current implementation, this timeout is set to 250ms.
Moreover this timer is re-triggered for 250ms at each packet transmission
without taking care of the previous non acked packet timeout.
So, in the worst case, the delay before retransmission of a packet is
tx_win*250ms. Common H5 tx_win is 4.

These delays are not acceptable in case of "real time".
Despite buffering, It may lead to audio cuts with A2DP profile.

Rather than decreasing timeout with a magic value, I propose to implement
a fast retransmit mechanism:
If a sender receives a specified number of invalid acks (basically, duplicate
ACKs), the sender can be reasonably confident that its oldest non-acknowledged
H5 packet was dropped.

This mechanism is out of the H5 specification.
However, without breaking compatibility, it gives good results in practical
tests.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_h5.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index fede8ca..8092daa 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -45,6 +45,8 @@
  */
 #define H5_MAX_LEN (4 + 0xfff + 2)

+#define H5_MAX_INV_ACK 2
+
 /* Convenience macros for reading Three-wire header values */
 #define H5_HDR_SEQ(hdr)		((hdr)[0] & 0x07)
 #define H5_HDR_ACK(hdr)		(((hdr)[0] >> 3) & 0x07)
@@ -74,6 +76,7 @@ struct h5 {
 	struct sk_buff		*rx_skb;	/* Receive buffer */
 	size_t			rx_pending;	/* Expecting more bytes */
 	u8			rx_ack;		/* Last ack number received */
+	u8			rx_inv_ack;	/* Invalid acks counter */

 	int			(*rx_func) (struct hci_uart *hu, u8 c);

@@ -240,8 +243,16 @@ static void h5_pkt_cull(struct h5 *h5)
 		seq = (seq - 1) % 8;
 	}

-	if (seq != h5->rx_ack)
+	if (seq != h5->rx_ack) {
 		BT_ERR("Controller acked invalid packet");
+		if (++h5->rx_inv_ack >= H5_MAX_INV_ACK) {
+			BT_ERR("H5 fast retransmit");
+			h5->rx_inv_ack = 0;
+			mod_timer(&h5->timer, 0);
+		}
+	} else {
+		h5->rx_inv_ack = 0;
+	}

 	i = 0;
 	skb_queue_walk_safe(&h5->unack, skb, tmp) {
--
1.8.3.2

Re: [RFC] Bluetooth: HCI H5 fast retransmit

[Marcel Holtmann]

Hi Loic,

> HCI H5 protocol is based on timeout to retransmit dropped packets.
> According to the specification, this timeout should be calculated
> based on the baudrate and the maximum packet size.
> 
> In the current implementation, this timeout is set to 250ms.
> Moreover this timer is re-triggered for 250ms at each packet transmission
> without taking care of the previous non acked packet timeout.
> So, in the worst case, the delay before retransmission of a packet is
> tx_win*250ms. Common H5 tx_win is 4.
> 
> These delays are not acceptable in case of "real time".
> Despite buffering, It may lead to audio cuts with A2DP profile.
> 
> Rather than decreasing timeout with a magic value, I propose to implement
> a fast retransmit mechanism:
> If a sender receives a specified number of invalid acks (basically, duplicate
> ACKs), the sender can be reasonably confident that its oldest non-acknowledged
> H5 packet was dropped.
> 
> This mechanism is out of the H5 specification.
> However, without breaking compatibility, it gives good results in practical
> tests.

what do you want me to do with this patch. I do not have a device to verify this and so far nobody else has spoken up to ACK or NAK it.

In general I am fine with such an approach. However you might want to add nice comments in the code for what we are doing and why.

I wonder if this feature is so much out-of-scape of H5 that adding an extra flag for it to enable it might be a good idea? Of is it just that this is something we should be doing anyway since it makes sense?

Regards

Marcel

Re: [RFC] Bluetooth: HCI H5 fast retransmit

[Loic Poulain]

Hi Marcel,

This patch is not so much out-of-scope of the H5 because specification
only gives "recommended parameters" regarding timeout calculation.

I tested it with only one H5 controller (rtl8723).
This mechanism is very beneficial with this controller due to a
relative high packet loss.

However I'm going to rework this patch to be more specific (only fast
retransmit if the invalid ack number is a duplicate of the last acked 
packet)
+ comments.

I don't see any obvious H5 incompatibility issue, therefore, extra flag
shouldn't be required.

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [RFC] Bluetooth: HCI H5 fast retransmit

[Marcel Holtmann]

Hi Loic,

> This patch is not so much out-of-scope of the H5 because specification
> only gives "recommended parameters" regarding timeout calculation.
> 
> I tested it with only one H5 controller (rtl8723).
> This mechanism is very beneficial with this controller due to a
> relative high packet loss.
> 
> However I'm going to rework this patch to be more specific (only fast
> retransmit if the invalid ack number is a duplicate of the last acked packet)
> + comments.

sounds good. Go for it.

Regards

Marcel

[RFC] Bluetooth: HCI H5 fast retransmit

[Loic Poulain]

HCI H5 protocol is based on timeout to retransmit dropped packets.
According to the specification, this timeout should be calculated
based on the baudrate and the maximum packet size.

In the current implementation, this timeout is set to 250ms.
Moreover this timer is re-triggered for 250ms at each packet transmission
without taking care of the previous non acked packet timeout.
So, in the worst case, the delay before retransmission of a packet is
tx_win*250ms. Common H5 tx_win is 4.

These delays are not acceptable in case of "real time".
Despite buffering, It may lead to audio cuts with A2DP profile.

Rather than decreasing timeout with a magic value, I propose to implement
a fast retransmit mechanism:
If a sender receives a specified number of invalid acks (basically, duplicate
ACKs), the sender can be reasonably confident that its oldest non-acknowledged
H5 packet was dropped.

This mechanism is out of the H5 specification.
However, without breaking compatibility, it gives good results in practical
tests.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_h5.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index fede8ca..8092daa 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -45,6 +45,8 @@
  */
 #define H5_MAX_LEN (4 + 0xfff + 2)

+#define H5_MAX_INV_ACK 2
+
 /* Convenience macros for reading Three-wire header values */
 #define H5_HDR_SEQ(hdr)		((hdr)[0] & 0x07)
 #define H5_HDR_ACK(hdr)		(((hdr)[0] >> 3) & 0x07)
@@ -74,6 +76,7 @@ struct h5 {
 	struct sk_buff		*rx_skb;	/* Receive buffer */
 	size_t			rx_pending;	/* Expecting more bytes */
 	u8			rx_ack;		/* Last ack number received */
+	u8			rx_inv_ack;	/* Invalid acks counter */

 	int			(*rx_func) (struct hci_uart *hu, u8 c);

@@ -240,8 +243,16 @@ static void h5_pkt_cull(struct h5 *h5)
 		seq = (seq - 1) % 8;
 	}

-	if (seq != h5->rx_ack)
+	if (seq != h5->rx_ack) {
 		BT_ERR("Controller acked invalid packet");
+		if (++h5->rx_inv_ack >= H5_MAX_INV_ACK) {
+			BT_ERR("H5 fast retransmit");
+			h5->rx_inv_ack = 0;
+			mod_timer(&h5->timer, 0);
+		}
+	} else {
+		h5->rx_inv_ack = 0;
+	}

 	i = 0;
 	skb_queue_walk_safe(&h5->unack, skb, tmp) {
--
1.8.3.2