[RFC 0/5 v2] prioritizing data over HCI

[RFC 0/5 v2] prioritizing data over HCI

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This incorporate some suggestions like removal of fixed amount of queues in
favor of one per L2CAP channel using HCI Channel abstraction (hci_chan) so
packet order is maintained, SCO/ESCO packets are no longer affected and
some other minor things like promoting starving channels directly to
maximum possible priority (6).

Priorities are unchanged, so anything bigger than 6 requires extra
capabilities and are meant for guaranteed channels or time critical packets
e.g. RFCOMM commands.

In addition to that I decide to maintain the queue per hci_conn so hci_chan
is only created when L2CAP connection completes.

Luiz Augusto von Dentz (5):
  Bluetooth: make use of connection number to optimize the scheduler
  Bluetooth: set skbuffer priority based on L2CAP socket priority
  Bluetooth: make use sk_priority to priritize RFCOMM packets
  Bluetooth: prioritizing data over HCI
  Bluetooth: recalculate priorities when channels are starving

 include/net/bluetooth/hci_core.h |   61 +++++++++
 include/net/bluetooth/l2cap.h    |    4 +-
 net/bluetooth/hci_conn.c         |   59 +++++++++
 net/bluetooth/hci_core.c         |  256 +++++++++++++++++++++++++++++++++++---
 net/bluetooth/l2cap_core.c       |   52 ++++++--
 net/bluetooth/l2cap_sock.c       |    2 +-
 net/bluetooth/rfcomm/core.c      |   51 +++++---
 net/bluetooth/rfcomm/sock.c      |    2 +
 8 files changed, 437 insertions(+), 50 deletions(-)

-- 
1.7.6

[RFC 1/5 v2] Bluetooth: make use of connection number to optimize the scheduler

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This checks if there is any existing connection according to its type
before start iterating in the list and immediately stop iterating when
reaching the number of connections.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |   16 ++++++++++++++++
 net/bluetooth/hci_core.c         |   15 +++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8f441b8..e2ba4d6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -395,6 +395,22 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 	}
 }
 
+static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type)
+{
+	struct hci_conn_hash *h = &hdev->conn_hash;
+	switch (type) {
+	case ACL_LINK:
+		return h->acl_num;
+	case LE_LINK:
+		return h->le_num;
+	case SCO_LINK:
+	case ESCO_LINK:
+		return h->sco_num;
+	default:
+		return 0;
+	}
+}
+
 static inline struct hci_conn *hci_conn_hash_lookup_handle(struct hci_dev *hdev,
 								__u16 handle)
 {
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ec0bc3f..9574752 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2075,6 +2075,9 @@ static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int
 			min  = c->sent;
 			conn = c;
 		}
+
+		if (hci_conn_num(hdev, type) == num)
+			break;
 	}
 
 	if (conn) {
@@ -2132,6 +2135,9 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	if (!hci_conn_num(hdev, ACL_LINK))
+		return;
+
 	if (!test_bit(HCI_RAW, &hdev->flags)) {
 		/* ACL tx timeout must be longer than maximum
 		 * link supervision timeout (40.9 seconds) */
@@ -2163,6 +2169,9 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	if (!hci_conn_num(hdev, SCO_LINK))
+		return;
+
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
@@ -2183,6 +2192,9 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	if (!hci_conn_num(hdev, ESCO_LINK))
+		return;
+
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
@@ -2203,6 +2215,9 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	if (!hci_conn_num(hdev, LE_LINK))
+		return;
+
 	if (!test_bit(HCI_RAW, &hdev->flags)) {
 		/* LE tx timeout must be longer than maximum
 		 * link supervision timeout (40.9 seconds) */
-- 
1.7.6

[RFC 2/5 v2] Bluetooth: set skbuffer priority based on L2CAP socket priority

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This uses SO_PRIORITY to set the skbuffer priority field

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |    3 ++
 include/net/bluetooth/l2cap.h    |    3 +-
 net/bluetooth/l2cap_core.c       |   45 +++++++++++++++++++++++++++-----------
 net/bluetooth/l2cap_sock.c       |    2 +-
 4 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e2ba4d6..0742828 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -32,6 +32,9 @@
 #define HCI_PROTO_L2CAP	0
 #define HCI_PROTO_SCO	1
 
+/* HCI priority */
+#define HCI_PRIO_MAX	7
+
 /* HCI Core structures */
 struct inquiry_data {
 	bdaddr_t	bdaddr;
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 4f34ad2..f018e5d 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -511,7 +511,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk);
 void l2cap_chan_close(struct l2cap_chan *chan, int reason);
 void l2cap_chan_destroy(struct l2cap_chan *chan);
 int l2cap_chan_connect(struct l2cap_chan *chan);
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
+int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
+								u32 priority);
 void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
 
 #endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3204ba8..3af0569 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1245,7 +1245,8 @@ void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
 	struct hci_conn *hcon = chan->conn->hcon;
 	u16 flags;
 
-	BT_DBG("chan %p, skb %p len %d", chan, skb, skb->len);
+	BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
+							skb->priority);
 
 	if (!chan->flushable && lmp_no_flush_capable(hcon->hdev))
 		flags = ACL_START_NO_FLUSH;
@@ -1451,6 +1452,8 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
 			return -EFAULT;
 
+		(*frag)->priority = skb->priority;
+
 		sent += count;
 		len  -= count;
 
@@ -1460,7 +1463,9 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
 	return sent;
 }
 
-struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
+						struct msghdr *msg, size_t len,
+						u32 priority)
 {
 	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
@@ -1468,7 +1473,7 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
 	int err, count, hlen = L2CAP_HDR_SIZE + 2;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
 
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
 	skb = bt_skb_send_alloc(sk, count + hlen,
@@ -1476,6 +1481,8 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
 	if (!skb)
 		return ERR_PTR(err);
 
+	skb->priority = priority;
+
 	/* Create L2CAP header */
 	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
 	lh->cid = cpu_to_le16(chan->dcid);
@@ -1490,7 +1497,9 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
 	return skb;
 }
 
-struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
+						struct msghdr *msg, size_t len,
+						u32 priority)
 {
 	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
@@ -1506,6 +1515,8 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
 	if (!skb)
 		return ERR_PTR(err);
 
+	skb->priority = priority;
+
 	/* Create L2CAP header */
 	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
 	lh->cid = cpu_to_le16(chan->dcid);
@@ -1519,7 +1530,10 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
 	return skb;
 }
 
-struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len, u16 control, u16 sdulen)
+static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
+						struct msghdr *msg, size_t len,
+						u32 priority, u16 control,
+						u16 sdulen)
 {
 	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
@@ -1527,7 +1541,7 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
 	int err, count, hlen = L2CAP_HDR_SIZE + 2;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
 
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
@@ -1544,6 +1558,8 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
 	if (!skb)
 		return ERR_PTR(err);
 
+	skb->priority = priority;
+
 	/* Create L2CAP header */
 	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
 	lh->cid = cpu_to_le16(chan->dcid);
@@ -1574,7 +1590,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
 
 	skb_queue_head_init(&sar_queue);
 	control = L2CAP_SDU_START;
-	skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
+	skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps,
+						HCI_PRIO_MAX, control, len);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
@@ -1593,7 +1610,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
 			buflen = len;
 		}
 
-		skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
+		skb = l2cap_create_iframe_pdu(chan, msg, buflen, HCI_PRIO_MAX,
+								control, 0);
 		if (IS_ERR(skb)) {
 			skb_queue_purge(&sar_queue);
 			return PTR_ERR(skb);
@@ -1610,7 +1628,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
 	return size;
 }
 
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
+								u32 priority)
 {
 	struct sk_buff *skb;
 	u16 control;
@@ -1618,7 +1637,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 
 	/* Connectionless channel */
 	if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
-		skb = l2cap_create_connless_pdu(chan, msg, len);
+		skb = l2cap_create_connless_pdu(chan, msg, len, priority);
 		if (IS_ERR(skb))
 			return PTR_ERR(skb);
 
@@ -1633,7 +1652,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 			return -EMSGSIZE;
 
 		/* Create a basic PDU */
-		skb = l2cap_create_basic_pdu(chan, msg, len);
+		skb = l2cap_create_basic_pdu(chan, msg, len, priority);
 		if (IS_ERR(skb))
 			return PTR_ERR(skb);
 
@@ -1646,8 +1665,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 		/* Entire SDU fits into one PDU */
 		if (len <= chan->remote_mps) {
 			control = L2CAP_SDU_UNSEGMENTED;
-			skb = l2cap_create_iframe_pdu(chan, msg, len, control,
-									0);
+			skb = l2cap_create_iframe_pdu(chan, msg, len, priority,
+								control, 0);
 			if (IS_ERR(skb))
 				return PTR_ERR(skb);
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 5c36b3e..abe302b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -706,7 +706,7 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
 		return -ENOTCONN;
 	}
 
-	err = l2cap_chan_send(chan, msg, len);
+	err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
 
 	release_sock(sk);
 	return err;
-- 
1.7.6

[RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
 net/bluetooth/rfcomm/sock.c |    2 +
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 5759bb7..3ec98bd 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -66,7 +66,8 @@ static unsigned long rfcomm_event;
 
 static LIST_HEAD(session_list);
 
-static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
+							u32 priority);
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
 static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
 static int rfcomm_queue_disc(struct rfcomm_dlc *d);
@@ -751,19 +752,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
 }
 
 /* ---- RFCOMM frame sending ---- */
-static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
+							u32 priority)
 {
 	struct socket *sock = s->sock;
+	struct sock *sk = sock->sk;
 	struct kvec iv = { data, len };
 	struct msghdr msg;
 
-	BT_DBG("session %p len %d", s, len);
+	BT_DBG("session %p len %d priority %u", s, len, priority);
+
+	if (sk->sk_priority != priority) {
+		lock_sock(sk);
+		sk->sk_priority = priority;
+		release_sock(sk);
+	}
 
 	memset(&msg, 0, sizeof(msg));
 
 	return kernel_sendmsg(sock, &msg, &iv, 1, len);
 }
 
+static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
+{
+	BT_DBG("%p cmd %u", s, cmd->ctrl);
+
+	return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
+}
+
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci)
 {
 	struct rfcomm_cmd cmd;
@@ -775,7 +791,7 @@ static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_ua(struct rfcomm_session *s, u8 dlci)
@@ -789,7 +805,7 @@ static int rfcomm_send_ua(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci)
@@ -803,7 +819,7 @@ static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_queue_disc(struct rfcomm_dlc *d)
@@ -817,6 +833,8 @@ static int rfcomm_queue_disc(struct rfcomm_dlc *d)
 	if (!skb)
 		return -ENOMEM;
 
+	skb->priority = HCI_PRIO_MAX;
+
 	cmd = (void *) __skb_put(skb, sizeof(*cmd));
 	cmd->addr = d->addr;
 	cmd->ctrl = __ctrl(RFCOMM_DISC, 1);
@@ -839,7 +857,7 @@ static int rfcomm_send_dm(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type)
@@ -864,7 +882,7 @@ static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d)
@@ -906,7 +924,7 @@ static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci,
@@ -944,7 +962,7 @@ int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci,
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status)
@@ -971,7 +989,7 @@ static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig)
@@ -998,7 +1016,7 @@ static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
@@ -1020,7 +1038,7 @@ static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
@@ -1042,7 +1060,7 @@ static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_test(struct rfcomm_session *s, int cr, u8 *pattern, int len)
@@ -1093,7 +1111,7 @@ static int rfcomm_send_credits(struct rfcomm_session *s, u8 addr, u8 credits)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
@@ -1771,7 +1789,8 @@ static inline int rfcomm_process_tx(struct rfcomm_dlc *d)
 		return skb_queue_len(&d->tx_queue);
 
 	while (d->tx_credits && (skb = skb_dequeue(&d->tx_queue))) {
-		err = rfcomm_send_frame(d->session, skb->data, skb->len);
+		err = rfcomm_send_frame(d->session, skb->data, skb->len,
+							skb->priority);
 		if (err < 0) {
 			skb_queue_head(&d->tx_queue, skb);
 			break;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 8f01e6b..de8c386 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -597,6 +597,8 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 			break;
 		}
 
+		skb->priority = sk->sk_priority;
+
 		err = rfcomm_dlc_send(d, skb);
 		if (err < 0) {
 			kfree_skb(skb);
-- 
1.7.6

[RFC 4/5 v2] Bluetooth: prioritizing data over HCI

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This implement priority based scheduler using skbuffer priority set via
SO_PRIORITY socket option.

It introduces hci_chan_hash (list of HCI Channel/hci_chan) per connection,
each item in this list refer to a L2CAP connection and it is used to
queue the data for transmission.

Each connection continue to have a queue but it is only used for time
critical packets such the L2CAP commands and it is always processed
before the channels thus it can be considered the highest priority.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |   42 +++++++++
 include/net/bluetooth/l2cap.h    |    1 +
 net/bluetooth/hci_conn.c         |   59 ++++++++++++
 net/bluetooth/hci_core.c         |  180 ++++++++++++++++++++++++++++++++++----
 net/bluetooth/l2cap_core.c       |    5 +-
 5 files changed, 269 insertions(+), 18 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0742828..e0d29eb 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -67,6 +67,12 @@ struct hci_conn_hash {
 	unsigned int     le_num;
 };
 
+struct hci_chan_hash {
+	struct list_head list;
+	spinlock_t       lock;
+	unsigned int     num;
+};
+
 struct bdaddr_list {
 	struct list_head list;
 	bdaddr_t bdaddr;
@@ -278,6 +284,7 @@ struct hci_conn {
 	unsigned int	sent;
 
 	struct sk_buff_head data_q;
+	struct hci_chan_hash chan_hash;
 
 	struct timer_list disc_timer;
 	struct timer_list idle_timer;
@@ -300,6 +307,14 @@ struct hci_conn {
 	void (*disconn_cfm_cb)	(struct hci_conn *conn, u8 reason);
 };
 
+struct hci_chan {
+	struct list_head list;
+
+	struct hci_conn *conn;
+	struct sk_buff_head data_q;
+	unsigned int	sent;
+};
+
 extern struct hci_proto *hci_proto[];
 extern struct list_head hci_dev_list;
 extern struct list_head hci_cb_list;
@@ -459,6 +474,28 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
 	return NULL;
 }
 
+static inline void hci_chan_hash_init(struct hci_conn *c)
+{
+	struct hci_chan_hash *h = &c->chan_hash;
+	INIT_LIST_HEAD(&h->list);
+	spin_lock_init(&h->lock);
+	h->num = 0;
+}
+
+static inline void hci_chan_hash_add(struct hci_conn *c, struct hci_chan *chan)
+{
+	struct hci_chan_hash *h = &c->chan_hash;
+	list_add(&chan->list, &h->list);
+	h->num++;
+}
+
+static inline void hci_chan_hash_del(struct hci_conn *c, struct hci_chan *chan)
+{
+	struct hci_chan_hash *h = &c->chan_hash;
+	list_del(&chan->list);
+	h->num--;
+}
+
 void hci_acl_connect(struct hci_conn *conn);
 void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
 void hci_add_sco(struct hci_conn *conn, __u16 handle);
@@ -470,6 +507,10 @@ int hci_conn_del(struct hci_conn *conn);
 void hci_conn_hash_flush(struct hci_dev *hdev);
 void hci_conn_check_pending(struct hci_dev *hdev);
 
+struct hci_chan *hci_chan_create(struct hci_conn *conn);
+int hci_chan_del(struct hci_chan *chan);
+void hci_chan_hash_flush(struct hci_conn *conn);
+
 struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 						__u8 sec_level, __u8 auth_type);
 int hci_conn_check_link_mode(struct hci_conn *conn);
@@ -839,6 +880,7 @@ int hci_unregister_notifier(struct notifier_block *nb);
 
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
 void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags);
+void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
 
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index f018e5d..1e4dd6b 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -301,6 +301,7 @@ struct srej_list {
 struct l2cap_chan {
 	struct sock *sk;
 
+	struct hci_chan		*hchan;
 	struct l2cap_conn	*conn;
 
 	__u8		state;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ea7f031..19ae6b4 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -374,6 +374,8 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 
 	skb_queue_head_init(&conn->data_q);
 
+	hci_chan_hash_init(conn);
+
 	setup_timer(&conn->disc_timer, hci_conn_timeout, (unsigned long)conn);
 	setup_timer(&conn->idle_timer, hci_conn_idle, (unsigned long)conn);
 	setup_timer(&conn->auto_accept_timer, hci_conn_auto_accept,
@@ -432,6 +434,8 @@ int hci_conn_del(struct hci_conn *conn)
 
 	tasklet_disable(&hdev->tx_task);
 
+	hci_chan_hash_flush(conn);
+
 	hci_conn_hash_del(hdev, conn);
 	if (hdev->notify)
 		hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
@@ -956,3 +960,58 @@ int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
 
 	return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
 }
+
+struct hci_chan *hci_chan_create(struct hci_conn *conn)
+{
+	struct hci_dev *hdev = conn->hdev;
+	struct hci_chan *chan;
+
+	BT_DBG("%s conn %p", hdev->name, conn);
+
+	chan = kzalloc(sizeof(struct hci_chan), GFP_ATOMIC);
+	if (!chan)
+		return NULL;
+
+	chan->conn = conn;
+	skb_queue_head_init(&chan->data_q);
+
+	tasklet_disable(&hdev->tx_task);
+	hci_chan_hash_add(conn, chan);
+	tasklet_enable(&hdev->tx_task);
+
+	return chan;
+}
+
+int hci_chan_del(struct hci_chan *chan)
+{
+	struct hci_conn *conn = chan->conn;
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("%s conn %p chan %p", hdev->name, conn, chan);
+
+	tasklet_disable(&hdev->tx_task);
+	hci_chan_hash_del(conn, chan);
+	tasklet_enable(&hdev->tx_task);
+
+	skb_queue_purge(&chan->data_q);
+	kfree(chan);
+
+	return 0;
+}
+
+void hci_chan_hash_flush(struct hci_conn *conn)
+{
+	struct hci_chan_hash *h = &conn->chan_hash;
+	struct list_head *p;
+
+	BT_DBG("conn %p", conn);
+	p = h->list.next;
+	while (p != &h->list) {
+		struct hci_chan *chan;
+
+		chan = list_entry(p, struct hci_chan, list);
+		p = p->next;
+
+		hci_chan_del(chan);
+	}
+}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9574752..3a4c3a2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1975,23 +1975,18 @@ static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
 	hdr->dlen   = cpu_to_le16(len);
 }
 
-void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
+static void hci_queue_acl(struct hci_conn *conn, struct sk_buff_head *queue,
+				struct sk_buff *skb, __u16 flags)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct sk_buff *list;
 
-	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
-
-	skb->dev = (void *) hdev;
-	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
-	hci_add_acl_hdr(skb, conn->handle, flags);
-
 	list = skb_shinfo(skb)->frag_list;
 	if (!list) {
 		/* Non fragmented */
 		BT_DBG("%s nonfrag skb %p len %d", hdev->name, skb, skb->len);
 
-		skb_queue_tail(&conn->data_q, skb);
+		skb_queue_tail(queue, skb);
 	} else {
 		/* Fragmented */
 		BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
@@ -1999,9 +1994,9 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
 		skb_shinfo(skb)->frag_list = NULL;
 
 		/* Queue all fragments atomically */
-		spin_lock_bh(&conn->data_q.lock);
+		spin_lock_bh(&queue->lock);
 
-		__skb_queue_tail(&conn->data_q, skb);
+		__skb_queue_tail(queue, skb);
 
 		flags &= ~ACL_START;
 		flags |= ACL_CONT;
@@ -2014,16 +2009,46 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
 
 			BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
 
-			__skb_queue_tail(&conn->data_q, skb);
+			__skb_queue_tail(queue, skb);
 		} while (list);
 
-		spin_unlock_bh(&conn->data_q.lock);
+		spin_unlock_bh(&queue->lock);
 	}
+}
+
+void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
+
+	skb->dev = (void *) hdev;
+	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
+	hci_add_acl_hdr(skb, conn->handle, flags);
+
+	hci_queue_acl(conn, &conn->data_q, skb, flags);
 
 	tasklet_schedule(&hdev->tx_task);
 }
 EXPORT_SYMBOL(hci_send_acl);
 
+void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags)
+{
+	struct hci_conn *conn = chan->conn;
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("%s chan %p flags 0x%x", hdev->name, chan, flags);
+
+	skb->dev = (void *) hdev;
+	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
+	hci_add_acl_hdr(skb, conn->handle, flags);
+
+	hci_queue_acl(conn, &chan->data_q, skb, flags);
+
+	tasklet_schedule(&hdev->tx_task);
+}
+EXPORT_SYMBOL(hci_chan_send_acl);
+
 /* Send SCO data */
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb)
 {
@@ -2127,11 +2152,95 @@ static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
 	}
 }
 
+static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
+						int *quote)
+{
+	struct hci_conn_hash *h = &hdev->conn_hash;
+	struct hci_chan *chan = NULL;
+	int num = 0, min = ~0, cur_prio = 0;
+	struct list_head *p, *c;
+	int cnt, q, conn_num = 0;
+
+	BT_DBG("%s", hdev->name);
+
+	list_for_each(p, &h->list) {
+		struct hci_conn *conn;
+		struct hci_chan_hash *ch;
+
+		conn = list_entry(p, struct hci_conn, list);
+		ch = &conn->chan_hash;
+
+		if (conn->type != type)
+			continue;
+
+		if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+			continue;
+
+		conn_num++;
+
+		list_for_each(c, &ch->list) {
+			struct hci_chan *tmp;
+			struct sk_buff *skb;
+
+			tmp = list_entry(c, struct hci_chan, list);
+
+			if (skb_queue_empty(&tmp->data_q))
+				continue;
+
+			skb = skb_peek(&tmp->data_q);
+			if (skb->priority < cur_prio)
+				continue;
+
+			if (skb->priority > cur_prio) {
+				num = 0;
+				min = ~0;
+				cur_prio = skb->priority;
+			}
+
+			num++;
+
+			if (conn->sent < min) {
+				min  = conn->sent;
+				chan = tmp;
+			}
+		}
+
+		if (hci_conn_num(hdev, type) == conn_num)
+			break;
+	}
+
+	if (!chan)
+		return NULL;
+
+	switch (chan->conn->type) {
+	case ACL_LINK:
+		cnt = hdev->acl_cnt;
+		break;
+	case SCO_LINK:
+	case ESCO_LINK:
+		cnt = hdev->sco_cnt;
+		break;
+	case LE_LINK:
+		cnt = hdev->le_mtu ? hdev->le_cnt : hdev->acl_cnt;
+		break;
+	default:
+		cnt = 0;
+		BT_ERR("Unknown link type");
+	}
+
+	q = cnt / num;
+	*quote = q ? q : 1;
+	BT_DBG("chan %p quote %d", chan, *quote);
+	return chan;
+}
+
 static inline void hci_sched_acl(struct hci_dev *hdev)
 {
 	struct hci_conn *conn;
+	struct hci_chan *chan;
 	struct sk_buff *skb;
 	int quote;
+	unsigned int cnt;
 
 	BT_DBG("%s", hdev->name);
 
@@ -2145,9 +2254,10 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 			hci_link_tx_to(hdev, ACL_LINK);
 	}
 
-	while (hdev->acl_cnt && (conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
+	while (hdev->acl_cnt &&
+			(conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
-			BT_DBG("skb %p len %d", skb, skb->len);
+			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
 
 			hci_conn_enter_active_mode(conn, bt_cb(skb)->force_active);
 
@@ -2158,6 +2268,26 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 			conn->sent++;
 		}
 	}
+
+	cnt = hdev->acl_cnt;
+
+	while (hdev->acl_cnt &&
+			(chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
+		while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
+			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
+					skb->len, skb->priority);
+
+			hci_conn_enter_active_mode(chan->conn,
+						bt_cb(skb)->force_active);
+
+			hci_send_frame(skb);
+			hdev->acl_last_tx = jiffies;
+
+			hdev->acl_cnt--;
+			chan->sent++;
+			chan->conn->sent++;
+		}
+	}
 }
 
 /* Schedule SCO */
@@ -2174,7 +2304,7 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
 
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
-			BT_DBG("skb %p len %d", skb, skb->len);
+			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
 			hci_send_frame(skb);
 
 			conn->sent++;
@@ -2197,7 +2327,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
 
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
-			BT_DBG("skb %p len %d", skb, skb->len);
+			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
 			hci_send_frame(skb);
 
 			conn->sent++;
@@ -2210,6 +2340,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
 static inline void hci_sched_le(struct hci_dev *hdev)
 {
 	struct hci_conn *conn;
+	struct hci_chan *chan;
 	struct sk_buff *skb;
 	int quote, cnt;
 
@@ -2229,7 +2360,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 	cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
 	while (cnt && (conn = hci_low_sent(hdev, LE_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
-			BT_DBG("skb %p len %d", skb, skb->len);
+			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
 
 			hci_send_frame(skb);
 			hdev->le_last_tx = jiffies;
@@ -2238,6 +2369,21 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 			conn->sent++;
 		}
 	}
+
+	while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
+		while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
+			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
+					skb->len, skb->priority);
+
+			hci_send_frame(skb);
+			hdev->le_last_tx = jiffies;
+
+			cnt--;
+			chan->sent++;
+			chan->conn->sent++;
+		}
+	}
+
 	if (hdev->le_pkts)
 		hdev->le_cnt = cnt;
 	else
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3af0569..d30cb36 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -313,6 +313,7 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 	conn->disc_reason = 0x13;
 
 	chan->conn = conn;
+	chan->hchan = hci_chan_create(conn->hcon);
 
 	if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED) {
 		if (conn->hcon->type == LE_LINK) {
@@ -357,6 +358,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	if (conn) {
 		/* Delete from channel list */
 		write_lock_bh(&conn->chan_lock);
+		hci_chan_del(chan->hchan);
+		chan->hchan = NULL;
 		list_del(&chan->list);
 		write_unlock_bh(&conn->chan_lock);
 		chan_put(chan);
@@ -1254,7 +1257,7 @@ void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
 		flags = ACL_START;
 
 	bt_cb(skb)->force_active = chan->force_active;
-	hci_send_acl(hcon, skb, flags);
+	hci_chan_send_acl(chan->hchan, skb, flags);
 }
 
 void l2cap_streaming_send(struct l2cap_chan *chan)
-- 
1.7.6

[RFC 5/5 v2] Bluetooth: recalculate priorities when channels are starving

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

To avoid starvation the priority is recalculated so that the starving
channels are promoted to HCI_PRIO_MAX - 1 (6).

HCI_PRIO_MAX (7) is considered special, because it requires CAP_NET_ADMIN
capability which can be used to provide more guaranties, so it is not used
when promoting.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_core.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3a4c3a2..eabe84e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2184,7 +2184,7 @@ static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
 
 			tmp = list_entry(c, struct hci_chan, list);
 
-			if (skb_queue_empty(&tmp->data_q))
+			if (tmp->sent || skb_queue_empty(&tmp->data_q))
 				continue;
 
 			skb = skb_peek(&tmp->data_q);
@@ -2234,6 +2234,58 @@ static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
 	return chan;
 }
 
+static void hci_prio_recalculate(struct hci_dev *hdev, __u8 type)
+{
+	struct hci_conn_hash *h = &hdev->conn_hash;
+	struct list_head *p, *c;
+	int num = 0;
+
+	BT_DBG("%s", hdev->name);
+
+	list_for_each(p, &h->list) {
+		struct hci_conn *conn;
+		struct hci_chan_hash *ch;
+
+		conn = list_entry(p, struct hci_conn, list);
+		ch = &conn->chan_hash;
+
+		if (conn->type != type)
+			continue;
+
+		if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+			continue;
+
+		num++;
+
+		list_for_each(c, &ch->list) {
+			struct hci_chan *chan;
+			struct sk_buff *skb;
+
+			chan = list_entry(c, struct hci_chan, list);
+
+			if (chan->sent) {
+				chan->sent = 0;
+				continue;
+			}
+
+			if (skb_queue_empty(&chan->data_q))
+				continue;
+
+			skb = skb_peek(&chan->data_q);
+			if (skb->priority >= HCI_PRIO_MAX - 1)
+				continue;
+
+			skb->priority = HCI_PRIO_MAX - 1;
+
+			BT_DBG("chan %p skb %p promoted to %d", chan, skb,
+								skb->priority);
+		}
+
+		if (hci_conn_num(hdev, type) == num)
+			break;
+	}
+}
+
 static inline void hci_sched_acl(struct hci_dev *hdev)
 {
 	struct hci_conn *conn;
@@ -2288,6 +2340,9 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 			chan->conn->sent++;
 		}
 	}
+
+	if (cnt != hdev->acl_cnt)
+		hci_prio_recalculate(hdev, ACL_LINK);
 }
 
 /* Schedule SCO */
@@ -2342,7 +2397,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 	struct hci_conn *conn;
 	struct hci_chan *chan;
 	struct sk_buff *skb;
-	int quote, cnt;
+	int quote, cnt, tmp;
 
 	BT_DBG("%s", hdev->name);
 
@@ -2370,6 +2425,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 		}
 	}
 
+	tmp = cnt;
 	while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
 			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
@@ -2388,6 +2444,9 @@ static inline void hci_sched_le(struct hci_dev *hdev)
 		hdev->le_cnt = cnt;
 	else
 		hdev->acl_cnt = cnt;
+
+	if (cnt != tmp)
+		hci_prio_recalculate(hdev, LE_LINK);
 }
 
 static void hci_tx_task(unsigned long arg)
-- 
1.7.6

Re: [RFC 2/5 v2] Bluetooth: set skbuffer priority based on L2CAP socket priority

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:01 +0300]:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This uses SO_PRIORITY to set the skbuffer priority field
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    3 ++
>  include/net/bluetooth/l2cap.h    |    3 +-
>  net/bluetooth/l2cap_core.c       |   45 +++++++++++++++++++++++++++-----------
>  net/bluetooth/l2cap_sock.c       |    2 +-
>  4 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e2ba4d6..0742828 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -32,6 +32,9 @@
>  #define HCI_PROTO_L2CAP	0
>  #define HCI_PROTO_SCO	1
>  
> +/* HCI priority */
> +#define HCI_PRIO_MAX	7
> +
>  /* HCI Core structures */
>  struct inquiry_data {
>  	bdaddr_t	bdaddr;
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 4f34ad2..f018e5d 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -511,7 +511,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk);
>  void l2cap_chan_close(struct l2cap_chan *chan, int reason);
>  void l2cap_chan_destroy(struct l2cap_chan *chan);
>  int l2cap_chan_connect(struct l2cap_chan *chan);
> -int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
> +int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> +								u32 priority);

I think priority could be part of struct l2cap_chan, so we avoid an extra
parameter in some calls here. Priority is also accessible from chan->sk, but I
would like to avoid more references to sk in l2cap core.

>  void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
>  
>  #endif /* __L2CAP_H */
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 3204ba8..3af0569 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1245,7 +1245,8 @@ void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
>  	struct hci_conn *hcon = chan->conn->hcon;
>  	u16 flags;
>  
> -	BT_DBG("chan %p, skb %p len %d", chan, skb, skb->len);
> +	BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
> +							skb->priority);
>  
>  	if (!chan->flushable && lmp_no_flush_capable(hcon->hdev))
>  		flags = ACL_START_NO_FLUSH;
> @@ -1451,6 +1452,8 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
>  		if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
>  			return -EFAULT;
>  
> +		(*frag)->priority = skb->priority;
> +
>  		sent += count;
>  		len  -= count;
>  
> @@ -1460,7 +1463,9 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
>  	return sent;
>  }
>  
> -struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
> +static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
> +						struct msghdr *msg, size_t len,
> +						u32 priority)
>  {
>  	struct sock *sk = chan->sk;
>  	struct l2cap_conn *conn = chan->conn;
> @@ -1468,7 +1473,7 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>  	int err, count, hlen = L2CAP_HDR_SIZE + 2;
>  	struct l2cap_hdr *lh;
>  
> -	BT_DBG("sk %p len %d", sk, (int)len);
> +	BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
>  
>  	count = min_t(unsigned int, (conn->mtu - hlen), len);
>  	skb = bt_skb_send_alloc(sk, count + hlen,
> @@ -1476,6 +1481,8 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>  	if (!skb)
>  		return ERR_PTR(err);
>  
> +	skb->priority = priority;
> +
>  	/* Create L2CAP header */
>  	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>  	lh->cid = cpu_to_le16(chan->dcid);
> @@ -1490,7 +1497,9 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>  	return skb;
>  }
>  
> -struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
> +static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> +						struct msghdr *msg, size_t len,
> +						u32 priority)
>  {
>  	struct sock *sk = chan->sk;
>  	struct l2cap_conn *conn = chan->conn;
> @@ -1506,6 +1515,8 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
>  	if (!skb)
>  		return ERR_PTR(err);
>  
> +	skb->priority = priority;
> +
>  	/* Create L2CAP header */
>  	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>  	lh->cid = cpu_to_le16(chan->dcid);
> @@ -1519,7 +1530,10 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
>  	return skb;
>  }
>  
> -struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len, u16 control, u16 sdulen)
> +static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
> +						struct msghdr *msg, size_t len,
> +						u32 priority, u16 control,
> +						u16 sdulen)
>  {
>  	struct sock *sk = chan->sk;
>  	struct l2cap_conn *conn = chan->conn;
> @@ -1527,7 +1541,7 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
>  	int err, count, hlen = L2CAP_HDR_SIZE + 2;
>  	struct l2cap_hdr *lh;
>  
> -	BT_DBG("sk %p len %d", sk, (int)len);
> +	BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
>  
>  	if (!conn)
>  		return ERR_PTR(-ENOTCONN);
> @@ -1544,6 +1558,8 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
>  	if (!skb)
>  		return ERR_PTR(err);
>  
> +	skb->priority = priority;
> +
>  	/* Create L2CAP header */
>  	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>  	lh->cid = cpu_to_le16(chan->dcid);
> @@ -1574,7 +1590,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
>  
>  	skb_queue_head_init(&sar_queue);
>  	control = L2CAP_SDU_START;
> -	skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
> +	skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps,
> +						HCI_PRIO_MAX, control, len);


HCI_PRIO_MAX, why?


>  	if (IS_ERR(skb))
>  		return PTR_ERR(skb);
>  
> @@ -1593,7 +1610,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
>  			buflen = len;
>  		}
>  
> -		skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
> +		skb = l2cap_create_iframe_pdu(chan, msg, buflen, HCI_PRIO_MAX,
> +								control, 0);

same here.

	Gustavo

Re: [RFC 4/5 v2] Bluetooth: prioritizing data over HCI

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:03 +0300]:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This implement priority based scheduler using skbuffer priority set via
> SO_PRIORITY socket option.
> 
> It introduces hci_chan_hash (list of HCI Channel/hci_chan) per connection,
> each item in this list refer to a L2CAP connection and it is used to
> queue the data for transmission.
> 
> Each connection continue to have a queue but it is only used for time
> critical packets such the L2CAP commands and it is always processed
> before the channels thus it can be considered the highest priority.

I think we can drop the connection and queue create a channel by default for
the special L2CAP channels, BR/EDR and LE signalling, SMP and AMP Manager.
Those will have high priority of course. Standardize this in just one way to
send packets would be better. It simplifies the sending logic and reduces the
code.

> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |   42 +++++++++
>  include/net/bluetooth/l2cap.h    |    1 +
>  net/bluetooth/hci_conn.c         |   59 ++++++++++++
>  net/bluetooth/hci_core.c         |  180 ++++++++++++++++++++++++++++++++++----
>  net/bluetooth/l2cap_core.c       |    5 +-
>  5 files changed, 269 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 0742828..e0d29eb 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -67,6 +67,12 @@ struct hci_conn_hash {
>  	unsigned int     le_num;
>  };
>  
> +struct hci_chan_hash {
> +	struct list_head list;
> +	spinlock_t       lock;
> +	unsigned int     num;
> +};
> +
>  struct bdaddr_list {
>  	struct list_head list;
>  	bdaddr_t bdaddr;
> @@ -278,6 +284,7 @@ struct hci_conn {
>  	unsigned int	sent;
>  
>  	struct sk_buff_head data_q;
> +	struct hci_chan_hash chan_hash;
>  
>  	struct timer_list disc_timer;
>  	struct timer_list idle_timer;
> @@ -300,6 +307,14 @@ struct hci_conn {
>  	void (*disconn_cfm_cb)	(struct hci_conn *conn, u8 reason);
>  };
>  
> +struct hci_chan {
> +	struct list_head list;
> +
> +	struct hci_conn *conn;
> +	struct sk_buff_head data_q;
> +	unsigned int	sent;
> +};
> +
>  extern struct hci_proto *hci_proto[];
>  extern struct list_head hci_dev_list;
>  extern struct list_head hci_cb_list;
> @@ -459,6 +474,28 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
>  	return NULL;
>  }
>  
> +static inline void hci_chan_hash_init(struct hci_conn *c)
> +{
> +	struct hci_chan_hash *h = &c->chan_hash;
> +	INIT_LIST_HEAD(&h->list);
> +	spin_lock_init(&h->lock);
> +	h->num = 0;
> +}
> +
> +static inline void hci_chan_hash_add(struct hci_conn *c, struct hci_chan *chan)
> +{
> +	struct hci_chan_hash *h = &c->chan_hash;
> +	list_add(&chan->list, &h->list);
> +	h->num++;
> +}
> +
> +static inline void hci_chan_hash_del(struct hci_conn *c, struct hci_chan *chan)
> +{
> +	struct hci_chan_hash *h = &c->chan_hash;
> +	list_del(&chan->list);
> +	h->num--;
> +}
> +
>  void hci_acl_connect(struct hci_conn *conn);
>  void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
>  void hci_add_sco(struct hci_conn *conn, __u16 handle);
> @@ -470,6 +507,10 @@ int hci_conn_del(struct hci_conn *conn);
>  void hci_conn_hash_flush(struct hci_dev *hdev);
>  void hci_conn_check_pending(struct hci_dev *hdev);
>  
> +struct hci_chan *hci_chan_create(struct hci_conn *conn);
> +int hci_chan_del(struct hci_chan *chan);
> +void hci_chan_hash_flush(struct hci_conn *conn);
> +
>  struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
>  						__u8 sec_level, __u8 auth_type);
>  int hci_conn_check_link_mode(struct hci_conn *conn);
> @@ -839,6 +880,7 @@ int hci_unregister_notifier(struct notifier_block *nb);
>  
>  int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
>  void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags);
> +void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);

So we should have only one function to send data to HCI here, I propose keep
the hci_send_acl() name and just change its parameter to hci_chan.

>  void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
>  
>  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index f018e5d..1e4dd6b 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -301,6 +301,7 @@ struct srej_list {
>  struct l2cap_chan {
>  	struct sock *sk;
>  
> +	struct hci_chan		*hchan;
>  	struct l2cap_conn	*conn;
>  
>  	__u8		state;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index ea7f031..19ae6b4 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -374,6 +374,8 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>  
>  	skb_queue_head_init(&conn->data_q);
>  
> +	hci_chan_hash_init(conn);
> +
>  	setup_timer(&conn->disc_timer, hci_conn_timeout, (unsigned long)conn);
>  	setup_timer(&conn->idle_timer, hci_conn_idle, (unsigned long)conn);
>  	setup_timer(&conn->auto_accept_timer, hci_conn_auto_accept,
> @@ -432,6 +434,8 @@ int hci_conn_del(struct hci_conn *conn)
>  
>  	tasklet_disable(&hdev->tx_task);
>  
> +	hci_chan_hash_flush(conn);
> +
>  	hci_conn_hash_del(hdev, conn);
>  	if (hdev->notify)
>  		hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
> @@ -956,3 +960,58 @@ int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
>  
>  	return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
>  }
> +
> +struct hci_chan *hci_chan_create(struct hci_conn *conn)
> +{
> +	struct hci_dev *hdev = conn->hdev;
> +	struct hci_chan *chan;
> +
> +	BT_DBG("%s conn %p", hdev->name, conn);
> +
> +	chan = kzalloc(sizeof(struct hci_chan), GFP_ATOMIC);
> +	if (!chan)
> +		return NULL;
> +
> +	chan->conn = conn;
> +	skb_queue_head_init(&chan->data_q);
> +
> +	tasklet_disable(&hdev->tx_task);
> +	hci_chan_hash_add(conn, chan);
> +	tasklet_enable(&hdev->tx_task);
> +
> +	return chan;
> +}
> +
> +int hci_chan_del(struct hci_chan *chan)
> +{
> +	struct hci_conn *conn = chan->conn;
> +	struct hci_dev *hdev = conn->hdev;
> +
> +	BT_DBG("%s conn %p chan %p", hdev->name, conn, chan);
> +
> +	tasklet_disable(&hdev->tx_task);
> +	hci_chan_hash_del(conn, chan);
> +	tasklet_enable(&hdev->tx_task);
> +
> +	skb_queue_purge(&chan->data_q);
> +	kfree(chan);
> +
> +	return 0;
> +}
> +
> +void hci_chan_hash_flush(struct hci_conn *conn)
> +{
> +	struct hci_chan_hash *h = &conn->chan_hash;
> +	struct list_head *p;
> +
> +	BT_DBG("conn %p", conn);
> +	p = h->list.next;
> +	while (p != &h->list) {
> +		struct hci_chan *chan;
> +
> +		chan = list_entry(p, struct hci_chan, list);

Use list_for_each_entry() here, instead while + list_entry


> +		p = p->next;
> +
> +		hci_chan_del(chan);
> +	}
> +}
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 9574752..3a4c3a2 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1975,23 +1975,18 @@ static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
>  	hdr->dlen   = cpu_to_le16(len);
>  }
>  
> -void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
> +static void hci_queue_acl(struct hci_conn *conn, struct sk_buff_head *queue,
> +				struct sk_buff *skb, __u16 flags)
>  {
>  	struct hci_dev *hdev = conn->hdev;
>  	struct sk_buff *list;
>  
> -	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
> -
> -	skb->dev = (void *) hdev;
> -	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
> -	hci_add_acl_hdr(skb, conn->handle, flags);
> -
>  	list = skb_shinfo(skb)->frag_list;
>  	if (!list) {
>  		/* Non fragmented */
>  		BT_DBG("%s nonfrag skb %p len %d", hdev->name, skb, skb->len);
>  
> -		skb_queue_tail(&conn->data_q, skb);
> +		skb_queue_tail(queue, skb);
>  	} else {
>  		/* Fragmented */
>  		BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
> @@ -1999,9 +1994,9 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>  		skb_shinfo(skb)->frag_list = NULL;
>  
>  		/* Queue all fragments atomically */
> -		spin_lock_bh(&conn->data_q.lock);
> +		spin_lock_bh(&queue->lock);
>  
> -		__skb_queue_tail(&conn->data_q, skb);
> +		__skb_queue_tail(queue, skb);
>  
>  		flags &= ~ACL_START;
>  		flags |= ACL_CONT;
> @@ -2014,16 +2009,46 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>  
>  			BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>  
> -			__skb_queue_tail(&conn->data_q, skb);
> +			__skb_queue_tail(queue, skb);
>  		} while (list);
>  
> -		spin_unlock_bh(&conn->data_q.lock);
> +		spin_unlock_bh(&queue->lock);
>  	}
> +}
> +
> +void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
> +{
> +	struct hci_dev *hdev = conn->hdev;
> +
> +	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
> +
> +	skb->dev = (void *) hdev;
> +	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
> +	hci_add_acl_hdr(skb, conn->handle, flags);
> +
> +	hci_queue_acl(conn, &conn->data_q, skb, flags);
>  
>  	tasklet_schedule(&hdev->tx_task);
>  }
>  EXPORT_SYMBOL(hci_send_acl);
>  
> +void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags)
> +{
> +	struct hci_conn *conn = chan->conn;
> +	struct hci_dev *hdev = conn->hdev;
> +
> +	BT_DBG("%s chan %p flags 0x%x", hdev->name, chan, flags);
> +
> +	skb->dev = (void *) hdev;
> +	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
> +	hci_add_acl_hdr(skb, conn->handle, flags);
> +
> +	hci_queue_acl(conn, &chan->data_q, skb, flags);
> +
> +	tasklet_schedule(&hdev->tx_task);
> +}
> +EXPORT_SYMBOL(hci_chan_send_acl);
> +
>  /* Send SCO data */
>  void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb)
>  {
> @@ -2127,11 +2152,95 @@ static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
>  	}
>  }
>  
> +static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
> +						int *quote)
> +{
> +	struct hci_conn_hash *h = &hdev->conn_hash;
> +	struct hci_chan *chan = NULL;
> +	int num = 0, min = ~0, cur_prio = 0;
> +	struct list_head *p, *c;
> +	int cnt, q, conn_num = 0;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	list_for_each(p, &h->list) {
> +		struct hci_conn *conn;
> +		struct hci_chan_hash *ch;
> +
> +		conn = list_entry(p, struct hci_conn, list);
> +		ch = &conn->chan_hash;
> +
> +		if (conn->type != type)
> +			continue;
> +
> +		if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
> +			continue;
> +
> +		conn_num++;
> +
> +		list_for_each(c, &ch->list) {
> +			struct hci_chan *tmp;
> +			struct sk_buff *skb;
> +
> +			tmp = list_entry(c, struct hci_chan, list);
> +
> +			if (skb_queue_empty(&tmp->data_q))
> +				continue;
> +
> +			skb = skb_peek(&tmp->data_q);
> +			if (skb->priority < cur_prio)
> +				continue;
> +
> +			if (skb->priority > cur_prio) {
> +				num = 0;
> +				min = ~0;
> +				cur_prio = skb->priority;
> +			}
> +
> +			num++;
> +
> +			if (conn->sent < min) {
> +				min  = conn->sent;
> +				chan = tmp;
> +			}
> +		}
> +
> +		if (hci_conn_num(hdev, type) == conn_num)
> +			break;
> +	}
> +
> +	if (!chan)
> +		return NULL;
> +
> +	switch (chan->conn->type) {
> +	case ACL_LINK:
> +		cnt = hdev->acl_cnt;
> +		break;
> +	case SCO_LINK:
> +	case ESCO_LINK:
> +		cnt = hdev->sco_cnt;
> +		break;
> +	case LE_LINK:
> +		cnt = hdev->le_mtu ? hdev->le_cnt : hdev->acl_cnt;
> +		break;
> +	default:
> +		cnt = 0;
> +		BT_ERR("Unknown link type");
> +	}
> +
> +	q = cnt / num;
> +	*quote = q ? q : 1;
> +	BT_DBG("chan %p quote %d", chan, *quote);
> +	return chan;
> +}
> +
>  static inline void hci_sched_acl(struct hci_dev *hdev)
>  {
>  	struct hci_conn *conn;
> +	struct hci_chan *chan;
>  	struct sk_buff *skb;
>  	int quote;
> +	unsigned int cnt;
>  
>  	BT_DBG("%s", hdev->name);
>  
> @@ -2145,9 +2254,10 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>  			hci_link_tx_to(hdev, ACL_LINK);
>  	}
>  
> -	while (hdev->acl_cnt && (conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
> +	while (hdev->acl_cnt &&
> +			(conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
>  		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -			BT_DBG("skb %p len %d", skb, skb->len);
> +			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>  
>  			hci_conn_enter_active_mode(conn, bt_cb(skb)->force_active);
>  
> @@ -2158,6 +2268,26 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>  			conn->sent++;
>  		}
>  	}
> +
> +	cnt = hdev->acl_cnt;
> +
> +	while (hdev->acl_cnt &&
> +			(chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
> +		while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
> +			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
> +					skb->len, skb->priority);
> +
> +			hci_conn_enter_active_mode(chan->conn,
> +						bt_cb(skb)->force_active);
> +
> +			hci_send_frame(skb);
> +			hdev->acl_last_tx = jiffies;
> +
> +			hdev->acl_cnt--;
> +			chan->sent++;
> +			chan->conn->sent++;
> +		}
> +	}
>  }
>  
>  /* Schedule SCO */
> @@ -2174,7 +2304,7 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
>  
>  	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
>  		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -			BT_DBG("skb %p len %d", skb, skb->len);
> +			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>  			hci_send_frame(skb);
>  
>  			conn->sent++;
> @@ -2197,7 +2327,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
>  
>  	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, &quote))) {
>  		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -			BT_DBG("skb %p len %d", skb, skb->len);
> +			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>  			hci_send_frame(skb);
>  
>  			conn->sent++;
> @@ -2210,6 +2340,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
>  static inline void hci_sched_le(struct hci_dev *hdev)
>  {
>  	struct hci_conn *conn;
> +	struct hci_chan *chan;
>  	struct sk_buff *skb;
>  	int quote, cnt;
>  
> @@ -2229,7 +2360,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
>  	cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
>  	while (cnt && (conn = hci_low_sent(hdev, LE_LINK, &quote))) {
>  		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -			BT_DBG("skb %p len %d", skb, skb->len);
> +			BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>  
>  			hci_send_frame(skb);
>  			hdev->le_last_tx = jiffies;
> @@ -2238,6 +2369,21 @@ static inline void hci_sched_le(struct hci_dev *hdev)
>  			conn->sent++;
>  		}
>  	}
> +
> +	while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
> +		while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
> +			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
> +					skb->len, skb->priority);
> +
> +			hci_send_frame(skb);
> +			hdev->le_last_tx = jiffies;
> +
> +			cnt--;
> +			chan->sent++;
> +			chan->conn->sent++;
> +		}
> +	}
> +

IMHO when le_pkts equals zero(i.e., ACL and LE use the same buffer) we should
handle LE sending together with the ACL sending. This way we are prioritizing
ACL data over LE if we call hci_sched_acl before hci_sched_le.

	Gustavo

Re: [RFC 1/5 v2] Bluetooth: make use of connection number to optimize the scheduler

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:00 +0300]:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This checks if there is any existing connection according to its type
> before start iterating in the list and immediately stop iterating when
> reaching the number of connections.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |   16 ++++++++++++++++
>  net/bluetooth/hci_core.c         |   15 +++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)

Applied, thanks.

	Gustavo

Re: [RFC 2/5 v2] Bluetooth: set skbuffer priority based on L2CAP socket priority

[Luiz Augusto von Dentz]

Hi Gustavo,

On Wed, Aug 24, 2011 at 10:37 PM, Gustavo Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:01 +0300]:
>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This uses SO_PRIORITY to set the skbuffer priority field
>>
>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> ---
>>  include/net/bluetooth/hci_core.h |    3 ++
>>  include/net/bluetooth/l2cap.h    |    3 +-
>>  net/bluetooth/l2cap_core.c       |   45 +++++++++++++++++++++++++++-----------
>>  net/bluetooth/l2cap_sock.c       |    2 +-
>>  4 files changed, 38 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index e2ba4d6..0742828 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -32,6 +32,9 @@
>>  #define HCI_PROTO_L2CAP      0
>>  #define HCI_PROTO_SCO        1
>>
>> +/* HCI priority */
>> +#define HCI_PRIO_MAX 7
>> +
>>  /* HCI Core structures */
>>  struct inquiry_data {
>>       bdaddr_t        bdaddr;
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 4f34ad2..f018e5d 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -511,7 +511,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk);
>>  void l2cap_chan_close(struct l2cap_chan *chan, int reason);
>>  void l2cap_chan_destroy(struct l2cap_chan *chan);
>>  int l2cap_chan_connect(struct l2cap_chan *chan);
>> -int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
>> +int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>> +                                                             u32 priority);
>
> I think priority could be part of struct l2cap_chan, so we avoid an extra
> parameter in some calls here. Priority is also accessible from chan->sk, but I
> would like to avoid more references to sk in l2cap core.

It would be great and simplify a lot the scheduler, but Im afraid have
to do it per skbuffer because of RFCOMM, besides it gives more
flexibility if we latter need to move to another queueing discipline.

>>  void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
>>
>>  #endif /* __L2CAP_H */
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 3204ba8..3af0569 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1245,7 +1245,8 @@ void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
>>       struct hci_conn *hcon = chan->conn->hcon;
>>       u16 flags;
>>
>> -     BT_DBG("chan %p, skb %p len %d", chan, skb, skb->len);
>> +     BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
>> +                                                     skb->priority);
>>
>>       if (!chan->flushable && lmp_no_flush_capable(hcon->hdev))
>>               flags = ACL_START_NO_FLUSH;
>> @@ -1451,6 +1452,8 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
>>               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
>>                       return -EFAULT;
>>
>> +             (*frag)->priority = skb->priority;
>> +
>>               sent += count;
>>               len  -= count;
>>
>> @@ -1460,7 +1463,9 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
>>       return sent;
>>  }
>>
>> -struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
>> +static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
>> +                                             struct msghdr *msg, size_t len,
>> +                                             u32 priority)
>>  {
>>       struct sock *sk = chan->sk;
>>       struct l2cap_conn *conn = chan->conn;
>> @@ -1468,7 +1473,7 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>>       int err, count, hlen = L2CAP_HDR_SIZE + 2;
>>       struct l2cap_hdr *lh;
>>
>> -     BT_DBG("sk %p len %d", sk, (int)len);
>> +     BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
>>
>>       count = min_t(unsigned int, (conn->mtu - hlen), len);
>>       skb = bt_skb_send_alloc(sk, count + hlen,
>> @@ -1476,6 +1481,8 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>>       if (!skb)
>>               return ERR_PTR(err);
>>
>> +     skb->priority = priority;
>> +
>>       /* Create L2CAP header */
>>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>>       lh->cid = cpu_to_le16(chan->dcid);
>> @@ -1490,7 +1497,9 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
>>       return skb;
>>  }
>>
>> -struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
>> +static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
>> +                                             struct msghdr *msg, size_t len,
>> +                                             u32 priority)
>>  {
>>       struct sock *sk = chan->sk;
>>       struct l2cap_conn *conn = chan->conn;
>> @@ -1506,6 +1515,8 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
>>       if (!skb)
>>               return ERR_PTR(err);
>>
>> +     skb->priority = priority;
>> +
>>       /* Create L2CAP header */
>>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>>       lh->cid = cpu_to_le16(chan->dcid);
>> @@ -1519,7 +1530,10 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
>>       return skb;
>>  }
>>
>> -struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len, u16 control, u16 sdulen)
>> +static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
>> +                                             struct msghdr *msg, size_t len,
>> +                                             u32 priority, u16 control,
>> +                                             u16 sdulen)
>>  {
>>       struct sock *sk = chan->sk;
>>       struct l2cap_conn *conn = chan->conn;
>> @@ -1527,7 +1541,7 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
>>       int err, count, hlen = L2CAP_HDR_SIZE + 2;
>>       struct l2cap_hdr *lh;
>>
>> -     BT_DBG("sk %p len %d", sk, (int)len);
>> +     BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
>>
>>       if (!conn)
>>               return ERR_PTR(-ENOTCONN);
>> @@ -1544,6 +1558,8 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
>>       if (!skb)
>>               return ERR_PTR(err);
>>
>> +     skb->priority = priority;
>> +
>>       /* Create L2CAP header */
>>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
>>       lh->cid = cpu_to_le16(chan->dcid);
>> @@ -1574,7 +1590,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
>>
>>       skb_queue_head_init(&sar_queue);
>>       control = L2CAP_SDU_START;
>> -     skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
>> +     skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps,
>> +                                             HCI_PRIO_MAX, control, len);
>
>
> HCI_PRIO_MAX, why?

The idea is that everything that is time critical, has a timer/timeout
associated, should use HCI_PRIO_MAX, I thought this was the case here.

>
>>       if (IS_ERR(skb))
>>               return PTR_ERR(skb);
>>
>> @@ -1593,7 +1610,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
>>                       buflen = len;
>>               }
>>
>> -             skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
>> +             skb = l2cap_create_iframe_pdu(chan, msg, buflen, HCI_PRIO_MAX,
>> +                                                             control, 0);
>
> same here.

Here too.

-- 
Luiz Augusto von Dentz

Re: [RFC 4/5 v2] Bluetooth: prioritizing data over HCI

[Luiz Augusto von Dentz]

Hi Gustavo,

On Wed, Aug 24, 2011 at 11:04 PM, Gustavo Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:03 +0300]:
>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This implement priority based scheduler using skbuffer priority set via
>> SO_PRIORITY socket option.
>>
>> It introduces hci_chan_hash (list of HCI Channel/hci_chan) per connection,
>> each item in this list refer to a L2CAP connection and it is used to
>> queue the data for transmission.
>>
>> Each connection continue to have a queue but it is only used for time
>> critical packets such the L2CAP commands and it is always processed
>> before the channels thus it can be considered the highest priority.
>
> I think we can drop the connection and queue create a channel by default for
> the special L2CAP channels, BR/EDR and LE signalling, SMP and AMP Manager.
> Those will have high priority of course. Standardize this in just one way to
> send packets would be better. It simplifies the sending logic and reduces the
> code.

Sure, but then we need something to identify this special hci_chan and
it needs to be added to the list to be processed together, not sure if
it will simplify that much in the end.

>>
>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> ---
>>  include/net/bluetooth/hci_core.h |   42 +++++++++
>>  include/net/bluetooth/l2cap.h    |    1 +
>>  net/bluetooth/hci_conn.c         |   59 ++++++++++++
>>  net/bluetooth/hci_core.c         |  180 ++++++++++++++++++++++++++++++++++----
>>  net/bluetooth/l2cap_core.c       |    5 +-
>>  5 files changed, 269 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 0742828..e0d29eb 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -67,6 +67,12 @@ struct hci_conn_hash {
>>       unsigned int     le_num;
>>  };
>>
>> +struct hci_chan_hash {
>> +     struct list_head list;
>> +     spinlock_t       lock;
>> +     unsigned int     num;
>> +};
>> +
>>  struct bdaddr_list {
>>       struct list_head list;
>>       bdaddr_t bdaddr;
>> @@ -278,6 +284,7 @@ struct hci_conn {
>>       unsigned int    sent;
>>
>>       struct sk_buff_head data_q;
>> +     struct hci_chan_hash chan_hash;
>>
>>       struct timer_list disc_timer;
>>       struct timer_list idle_timer;
>> @@ -300,6 +307,14 @@ struct hci_conn {
>>       void (*disconn_cfm_cb)  (struct hci_conn *conn, u8 reason);
>>  };
>>
>> +struct hci_chan {
>> +     struct list_head list;
>> +
>> +     struct hci_conn *conn;
>> +     struct sk_buff_head data_q;
>> +     unsigned int    sent;
>> +};
>> +
>>  extern struct hci_proto *hci_proto[];
>>  extern struct list_head hci_dev_list;
>>  extern struct list_head hci_cb_list;
>> @@ -459,6 +474,28 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
>>       return NULL;
>>  }
>>
>> +static inline void hci_chan_hash_init(struct hci_conn *c)
>> +{
>> +     struct hci_chan_hash *h = &c->chan_hash;
>> +     INIT_LIST_HEAD(&h->list);
>> +     spin_lock_init(&h->lock);
>> +     h->num = 0;
>> +}
>> +
>> +static inline void hci_chan_hash_add(struct hci_conn *c, struct hci_chan *chan)
>> +{
>> +     struct hci_chan_hash *h = &c->chan_hash;
>> +     list_add(&chan->list, &h->list);
>> +     h->num++;
>> +}
>> +
>> +static inline void hci_chan_hash_del(struct hci_conn *c, struct hci_chan *chan)
>> +{
>> +     struct hci_chan_hash *h = &c->chan_hash;
>> +     list_del(&chan->list);
>> +     h->num--;
>> +}
>> +
>>  void hci_acl_connect(struct hci_conn *conn);
>>  void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
>>  void hci_add_sco(struct hci_conn *conn, __u16 handle);
>> @@ -470,6 +507,10 @@ int hci_conn_del(struct hci_conn *conn);
>>  void hci_conn_hash_flush(struct hci_dev *hdev);
>>  void hci_conn_check_pending(struct hci_dev *hdev);
>>
>> +struct hci_chan *hci_chan_create(struct hci_conn *conn);
>> +int hci_chan_del(struct hci_chan *chan);
>> +void hci_chan_hash_flush(struct hci_conn *conn);
>> +
>>  struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
>>                                               __u8 sec_level, __u8 auth_type);
>>  int hci_conn_check_link_mode(struct hci_conn *conn);
>> @@ -839,6 +880,7 @@ int hci_unregister_notifier(struct notifier_block *nb);
>>
>>  int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
>>  void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags);
>> +void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
>
> So we should have only one function to send data to HCI here, I propose keep
> the hci_send_acl() name and just change its parameter to hci_chan.
>
>>  void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
>>
>>  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index f018e5d..1e4dd6b 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -301,6 +301,7 @@ struct srej_list {
>>  struct l2cap_chan {
>>       struct sock *sk;
>>
>> +     struct hci_chan         *hchan;
>>       struct l2cap_conn       *conn;
>>
>>       __u8            state;
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index ea7f031..19ae6b4 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -374,6 +374,8 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>>
>>       skb_queue_head_init(&conn->data_q);
>>
>> +     hci_chan_hash_init(conn);
>> +
>>       setup_timer(&conn->disc_timer, hci_conn_timeout, (unsigned long)conn);
>>       setup_timer(&conn->idle_timer, hci_conn_idle, (unsigned long)conn);
>>       setup_timer(&conn->auto_accept_timer, hci_conn_auto_accept,
>> @@ -432,6 +434,8 @@ int hci_conn_del(struct hci_conn *conn)
>>
>>       tasklet_disable(&hdev->tx_task);
>>
>> +     hci_chan_hash_flush(conn);
>> +
>>       hci_conn_hash_del(hdev, conn);
>>       if (hdev->notify)
>>               hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
>> @@ -956,3 +960,58 @@ int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
>>
>>       return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
>>  }
>> +
>> +struct hci_chan *hci_chan_create(struct hci_conn *conn)
>> +{
>> +     struct hci_dev *hdev = conn->hdev;
>> +     struct hci_chan *chan;
>> +
>> +     BT_DBG("%s conn %p", hdev->name, conn);
>> +
>> +     chan = kzalloc(sizeof(struct hci_chan), GFP_ATOMIC);
>> +     if (!chan)
>> +             return NULL;
>> +
>> +     chan->conn = conn;
>> +     skb_queue_head_init(&chan->data_q);
>> +
>> +     tasklet_disable(&hdev->tx_task);
>> +     hci_chan_hash_add(conn, chan);
>> +     tasklet_enable(&hdev->tx_task);
>> +
>> +     return chan;
>> +}
>> +
>> +int hci_chan_del(struct hci_chan *chan)
>> +{
>> +     struct hci_conn *conn = chan->conn;
>> +     struct hci_dev *hdev = conn->hdev;
>> +
>> +     BT_DBG("%s conn %p chan %p", hdev->name, conn, chan);
>> +
>> +     tasklet_disable(&hdev->tx_task);
>> +     hci_chan_hash_del(conn, chan);
>> +     tasklet_enable(&hdev->tx_task);
>> +
>> +     skb_queue_purge(&chan->data_q);
>> +     kfree(chan);
>> +
>> +     return 0;
>> +}
>> +
>> +void hci_chan_hash_flush(struct hci_conn *conn)
>> +{
>> +     struct hci_chan_hash *h = &conn->chan_hash;
>> +     struct list_head *p;
>> +
>> +     BT_DBG("conn %p", conn);
>> +     p = h->list.next;
>> +     while (p != &h->list) {
>> +             struct hci_chan *chan;
>> +
>> +             chan = list_entry(p, struct hci_chan, list);
>
> Use list_for_each_entry() here, instead while + list_entry

Sure.

>> +             p = p->next;
>> +
>> +             hci_chan_del(chan);
>> +     }
>> +}
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 9574752..3a4c3a2 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1975,23 +1975,18 @@ static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
>>       hdr->dlen   = cpu_to_le16(len);
>>  }
>>
>> -void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>> +static void hci_queue_acl(struct hci_conn *conn, struct sk_buff_head *queue,
>> +                             struct sk_buff *skb, __u16 flags)
>>  {
>>       struct hci_dev *hdev = conn->hdev;
>>       struct sk_buff *list;
>>
>> -     BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
>> -
>> -     skb->dev = (void *) hdev;
>> -     bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> -     hci_add_acl_hdr(skb, conn->handle, flags);
>> -
>>       list = skb_shinfo(skb)->frag_list;
>>       if (!list) {
>>               /* Non fragmented */
>>               BT_DBG("%s nonfrag skb %p len %d", hdev->name, skb, skb->len);
>>
>> -             skb_queue_tail(&conn->data_q, skb);
>> +             skb_queue_tail(queue, skb);
>>       } else {
>>               /* Fragmented */
>>               BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>> @@ -1999,9 +1994,9 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>>               skb_shinfo(skb)->frag_list = NULL;
>>
>>               /* Queue all fragments atomically */
>> -             spin_lock_bh(&conn->data_q.lock);
>> +             spin_lock_bh(&queue->lock);
>>
>> -             __skb_queue_tail(&conn->data_q, skb);
>> +             __skb_queue_tail(queue, skb);
>>
>>               flags &= ~ACL_START;
>>               flags |= ACL_CONT;
>> @@ -2014,16 +2009,46 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>>
>>                       BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>>
>> -                     __skb_queue_tail(&conn->data_q, skb);
>> +                     __skb_queue_tail(queue, skb);
>>               } while (list);
>>
>> -             spin_unlock_bh(&conn->data_q.lock);
>> +             spin_unlock_bh(&queue->lock);
>>       }
>> +}
>> +
>> +void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>> +{
>> +     struct hci_dev *hdev = conn->hdev;
>> +
>> +     BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
>> +
>> +     skb->dev = (void *) hdev;
>> +     bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> +     hci_add_acl_hdr(skb, conn->handle, flags);
>> +
>> +     hci_queue_acl(conn, &conn->data_q, skb, flags);
>>
>>       tasklet_schedule(&hdev->tx_task);
>>  }
>>  EXPORT_SYMBOL(hci_send_acl);
>>
>> +void hci_chan_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags)
>> +{
>> +     struct hci_conn *conn = chan->conn;
>> +     struct hci_dev *hdev = conn->hdev;
>> +
>> +     BT_DBG("%s chan %p flags 0x%x", hdev->name, chan, flags);
>> +
>> +     skb->dev = (void *) hdev;
>> +     bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> +     hci_add_acl_hdr(skb, conn->handle, flags);
>> +
>> +     hci_queue_acl(conn, &chan->data_q, skb, flags);
>> +
>> +     tasklet_schedule(&hdev->tx_task);
>> +}
>> +EXPORT_SYMBOL(hci_chan_send_acl);
>> +
>>  /* Send SCO data */
>>  void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb)
>>  {
>> @@ -2127,11 +2152,95 @@ static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
>>       }
>>  }
>>
>> +static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
>> +                                             int *quote)
>> +{
>> +     struct hci_conn_hash *h = &hdev->conn_hash;
>> +     struct hci_chan *chan = NULL;
>> +     int num = 0, min = ~0, cur_prio = 0;
>> +     struct list_head *p, *c;
>> +     int cnt, q, conn_num = 0;
>> +
>> +     BT_DBG("%s", hdev->name);
>> +
>> +     list_for_each(p, &h->list) {
>> +             struct hci_conn *conn;
>> +             struct hci_chan_hash *ch;
>> +
>> +             conn = list_entry(p, struct hci_conn, list);
>> +             ch = &conn->chan_hash;
>> +
>> +             if (conn->type != type)
>> +                     continue;
>> +
>> +             if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
>> +                     continue;
>> +
>> +             conn_num++;
>> +
>> +             list_for_each(c, &ch->list) {
>> +                     struct hci_chan *tmp;
>> +                     struct sk_buff *skb;
>> +
>> +                     tmp = list_entry(c, struct hci_chan, list);
>> +
>> +                     if (skb_queue_empty(&tmp->data_q))
>> +                             continue;
>> +
>> +                     skb = skb_peek(&tmp->data_q);
>> +                     if (skb->priority < cur_prio)
>> +                             continue;
>> +
>> +                     if (skb->priority > cur_prio) {
>> +                             num = 0;
>> +                             min = ~0;
>> +                             cur_prio = skb->priority;
>> +                     }
>> +
>> +                     num++;
>> +
>> +                     if (conn->sent < min) {
>> +                             min  = conn->sent;
>> +                             chan = tmp;
>> +                     }
>> +             }
>> +
>> +             if (hci_conn_num(hdev, type) == conn_num)
>> +                     break;
>> +     }
>> +
>> +     if (!chan)
>> +             return NULL;
>> +
>> +     switch (chan->conn->type) {
>> +     case ACL_LINK:
>> +             cnt = hdev->acl_cnt;
>> +             break;
>> +     case SCO_LINK:
>> +     case ESCO_LINK:
>> +             cnt = hdev->sco_cnt;
>> +             break;
>> +     case LE_LINK:
>> +             cnt = hdev->le_mtu ? hdev->le_cnt : hdev->acl_cnt;
>> +             break;
>> +     default:
>> +             cnt = 0;
>> +             BT_ERR("Unknown link type");
>> +     }
>> +
>> +     q = cnt / num;
>> +     *quote = q ? q : 1;
>> +     BT_DBG("chan %p quote %d", chan, *quote);
>> +     return chan;
>> +}
>> +
>>  static inline void hci_sched_acl(struct hci_dev *hdev)
>>  {
>>       struct hci_conn *conn;
>> +     struct hci_chan *chan;
>>       struct sk_buff *skb;
>>       int quote;
>> +     unsigned int cnt;
>>
>>       BT_DBG("%s", hdev->name);
>>
>> @@ -2145,9 +2254,10 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>>                       hci_link_tx_to(hdev, ACL_LINK);
>>       }
>>
>> -     while (hdev->acl_cnt && (conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
>> +     while (hdev->acl_cnt &&
>> +                     (conn = hci_low_sent(hdev, ACL_LINK, &quote))) {
>>               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>> -                     BT_DBG("skb %p len %d", skb, skb->len);
>> +                     BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>>
>>                       hci_conn_enter_active_mode(conn, bt_cb(skb)->force_active);
>>
>> @@ -2158,6 +2268,26 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>>                       conn->sent++;
>>               }
>>       }
>> +
>> +     cnt = hdev->acl_cnt;
>> +
>> +     while (hdev->acl_cnt &&
>> +                     (chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
>> +             while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
>> +                     BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
>> +                                     skb->len, skb->priority);
>> +
>> +                     hci_conn_enter_active_mode(chan->conn,
>> +                                             bt_cb(skb)->force_active);
>> +
>> +                     hci_send_frame(skb);
>> +                     hdev->acl_last_tx = jiffies;
>> +
>> +                     hdev->acl_cnt--;
>> +                     chan->sent++;
>> +                     chan->conn->sent++;
>> +             }
>> +     }
>>  }
>>
>>  /* Schedule SCO */
>> @@ -2174,7 +2304,7 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
>>
>>       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
>>               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>> -                     BT_DBG("skb %p len %d", skb, skb->len);
>> +                     BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>>                       hci_send_frame(skb);
>>
>>                       conn->sent++;
>> @@ -2197,7 +2327,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
>>
>>       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, &quote))) {
>>               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>> -                     BT_DBG("skb %p len %d", skb, skb->len);
>> +                     BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>>                       hci_send_frame(skb);
>>
>>                       conn->sent++;
>> @@ -2210,6 +2340,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
>>  static inline void hci_sched_le(struct hci_dev *hdev)
>>  {
>>       struct hci_conn *conn;
>> +     struct hci_chan *chan;
>>       struct sk_buff *skb;
>>       int quote, cnt;
>>
>> @@ -2229,7 +2360,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
>>       cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
>>       while (cnt && (conn = hci_low_sent(hdev, LE_LINK, &quote))) {
>>               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>> -                     BT_DBG("skb %p len %d", skb, skb->len);
>> +                     BT_DBG("conn %p skb %p len %d", conn, skb, skb->len);
>>
>>                       hci_send_frame(skb);
>>                       hdev->le_last_tx = jiffies;
>> @@ -2238,6 +2369,21 @@ static inline void hci_sched_le(struct hci_dev *hdev)
>>                       conn->sent++;
>>               }
>>       }
>> +
>> +     while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
>> +             while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
>> +                     BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
>> +                                     skb->len, skb->priority);
>> +
>> +                     hci_send_frame(skb);
>> +                     hdev->le_last_tx = jiffies;
>> +
>> +                     cnt--;
>> +                     chan->sent++;
>> +                     chan->conn->sent++;
>> +             }
>> +     }
>> +
>
> IMHO when le_pkts equals zero(i.e., ACL and LE use the same buffer) we should
> handle LE sending together with the ACL sending. This way we are prioritizing
> ACL data over LE if we call hci_sched_acl before hci_sched_le.

iirc Peter has some patch that merges them, the problem is that
conn->type cannot be set to ACL since it is used for other purposes,
but perhaps we gonna have a different type to identify the buffer type
the connection is using.


-- 
Luiz Augusto von Dentz

Re: [RFC 2/5 v2] Bluetooth: set skbuffer priority based on L2CAP socket priority

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-25 00:27:31 +0300]:

> Hi Gustavo,
> 
> On Wed, Aug 24, 2011 at 10:37 PM, Gustavo Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Luiz,
> >
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:01 +0300]:
> >
> >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>
> >> This uses SO_PRIORITY to set the skbuffer priority field
> >>
> >> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >> ---
> >>  include/net/bluetooth/hci_core.h |    3 ++
> >>  include/net/bluetooth/l2cap.h    |    3 +-
> >>  net/bluetooth/l2cap_core.c       |   45 +++++++++++++++++++++++++++-----------
> >>  net/bluetooth/l2cap_sock.c       |    2 +-
> >>  4 files changed, 38 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index e2ba4d6..0742828 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -32,6 +32,9 @@
> >>  #define HCI_PROTO_L2CAP      0
> >>  #define HCI_PROTO_SCO        1
> >>
> >> +/* HCI priority */
> >> +#define HCI_PRIO_MAX 7
> >> +
> >>  /* HCI Core structures */
> >>  struct inquiry_data {
> >>       bdaddr_t        bdaddr;
> >> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >> index 4f34ad2..f018e5d 100644
> >> --- a/include/net/bluetooth/l2cap.h
> >> +++ b/include/net/bluetooth/l2cap.h
> >> @@ -511,7 +511,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk);
> >>  void l2cap_chan_close(struct l2cap_chan *chan, int reason);
> >>  void l2cap_chan_destroy(struct l2cap_chan *chan);
> >>  int l2cap_chan_connect(struct l2cap_chan *chan);
> >> -int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
> >> +int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> >> +                                                             u32 priority);
> >
> > I think priority could be part of struct l2cap_chan, so we avoid an extra
> > parameter in some calls here. Priority is also accessible from chan->sk, but I
> > would like to avoid more references to sk in l2cap core.
> 
> It would be great and simplify a lot the scheduler, but Im afraid have
> to do it per skbuffer because of RFCOMM, besides it gives more
> flexibility if we latter need to move to another queueing discipline.

It looks we still can do that without adding a parameter to l2cap_chan_send(),
if needed we can step back on what I said before and use sk->priority inside
l2cap_core.c.

Actually we need to fix the mess that the interaction between L2CAP and
RFCOMM is, after that we will be able to handle rfcomm channels in the same
way we are going to do for ERTM channels.

> 
> >>  void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
> >>
> >>  #endif /* __L2CAP_H */
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index 3204ba8..3af0569 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -1245,7 +1245,8 @@ void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
> >>       struct hci_conn *hcon = chan->conn->hcon;
> >>       u16 flags;
> >>
> >> -     BT_DBG("chan %p, skb %p len %d", chan, skb, skb->len);
> >> +     BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len,
> >> +                                                     skb->priority);
> >>
> >>       if (!chan->flushable && lmp_no_flush_capable(hcon->hdev))
> >>               flags = ACL_START_NO_FLUSH;
> >> @@ -1451,6 +1452,8 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
> >>               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
> >>                       return -EFAULT;
> >>
> >> +             (*frag)->priority = skb->priority;
> >> +
> >>               sent += count;
> >>               len  -= count;
> >>
> >> @@ -1460,7 +1463,9 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
> >>       return sent;
> >>  }
> >>
> >> -struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
> >> +static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
> >> +                                             struct msghdr *msg, size_t len,
> >> +                                             u32 priority)
> >>  {
> >>       struct sock *sk = chan->sk;
> >>       struct l2cap_conn *conn = chan->conn;
> >> @@ -1468,7 +1473,7 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
> >>       int err, count, hlen = L2CAP_HDR_SIZE + 2;
> >>       struct l2cap_hdr *lh;
> >>
> >> -     BT_DBG("sk %p len %d", sk, (int)len);
> >> +     BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
> >>
> >>       count = min_t(unsigned int, (conn->mtu - hlen), len);
> >>       skb = bt_skb_send_alloc(sk, count + hlen,
> >> @@ -1476,6 +1481,8 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
> >>       if (!skb)
> >>               return ERR_PTR(err);
> >>
> >> +     skb->priority = priority;
> >> +
> >>       /* Create L2CAP header */
> >>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
> >>       lh->cid = cpu_to_le16(chan->dcid);
> >> @@ -1490,7 +1497,9 @@ struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, struct msghdr
> >>       return skb;
> >>  }
> >>
> >> -struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
> >> +static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> >> +                                             struct msghdr *msg, size_t len,
> >> +                                             u32 priority)
> >>  {
> >>       struct sock *sk = chan->sk;
> >>       struct l2cap_conn *conn = chan->conn;
> >> @@ -1506,6 +1515,8 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
> >>       if (!skb)
> >>               return ERR_PTR(err);
> >>
> >> +     skb->priority = priority;
> >> +
> >>       /* Create L2CAP header */
> >>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
> >>       lh->cid = cpu_to_le16(chan->dcid);
> >> @@ -1519,7 +1530,10 @@ struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, struct msghdr *m
> >>       return skb;
> >>  }
> >>
> >> -struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len, u16 control, u16 sdulen)
> >> +static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
> >> +                                             struct msghdr *msg, size_t len,
> >> +                                             u32 priority, u16 control,
> >> +                                             u16 sdulen)
> >>  {
> >>       struct sock *sk = chan->sk;
> >>       struct l2cap_conn *conn = chan->conn;
> >> @@ -1527,7 +1541,7 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
> >>       int err, count, hlen = L2CAP_HDR_SIZE + 2;
> >>       struct l2cap_hdr *lh;
> >>
> >> -     BT_DBG("sk %p len %d", sk, (int)len);
> >> +     BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
> >>
> >>       if (!conn)
> >>               return ERR_PTR(-ENOTCONN);
> >> @@ -1544,6 +1558,8 @@ struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, struct msghdr *
> >>       if (!skb)
> >>               return ERR_PTR(err);
> >>
> >> +     skb->priority = priority;
> >> +
> >>       /* Create L2CAP header */
> >>       lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
> >>       lh->cid = cpu_to_le16(chan->dcid);
> >> @@ -1574,7 +1590,8 @@ int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t le
> >>
> >>       skb_queue_head_init(&sar_queue);
> >>       control = L2CAP_SDU_START;
> >> -     skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
> >> +     skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps,
> >> +                                             HCI_PRIO_MAX, control, len);
> >
> >
> > HCI_PRIO_MAX, why?
> 
> The idea is that everything that is time critical, has a timer/timeout
> associated, should use HCI_PRIO_MAX, I thought this was the case here.

I got that, but this is not the case here. These a re normal ERTM/Streaming
packets ;)

	Gustavo

Re: [RFC 4/5 v2] Bluetooth: prioritizing data over HCI

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-25 00:53:04 +0300]:

> Hi Gustavo,
> 
> On Wed, Aug 24, 2011 at 11:04 PM, Gustavo Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Luiz,
> >
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-17 16:23:03 +0300]:
> >
> >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>
> >> This implement priority based scheduler using skbuffer priority set via
> >> SO_PRIORITY socket option.
> >>
> >> It introduces hci_chan_hash (list of HCI Channel/hci_chan) per connection,
> >> each item in this list refer to a L2CAP connection and it is used to
> >> queue the data for transmission.
> >>
> >> Each connection continue to have a queue but it is only used for time
> >> critical packets such the L2CAP commands and it is always processed
> >> before the channels thus it can be considered the highest priority.
> >
> > I think we can drop the connection and queue create a channel by default for
> > the special L2CAP channels, BR/EDR and LE signalling, SMP and AMP Manager.
> > Those will have high priority of course. Standardize this in just one way to
> > send packets would be better. It simplifies the sending logic and reduces the
> > code.
> 
> Sure, but then we need something to identify this special hci_chan and
> it needs to be added to the list to be processed together, not sure if
> it will simplify that much in the end.

It could be created and added to the connection hash when we create the
l2cap_conn and then have a l2cap_conn->hchan for it. IMHO this simplifies the
scheduler and the logic.

<...>

> >
> > IMHO when le_pkts equals zero(i.e., ACL and LE use the same buffer) we should
> > handle LE sending together with the ACL sending. This way we are prioritizing
> > ACL data over LE if we call hci_sched_acl before hci_sched_le.
> 
> iirc Peter has some patch that merges them, the problem is that
> conn->type cannot be set to ACL since it is used for other purposes,
> but perhaps we gonna have a different type to identify the buffer type
> the connection is using.

Yeah, I've read it just after reply to this email. ;)

	Gustavo

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

Hi Gustavo,

On Tue, Sep 20, 2011 at 7:59 PM, Gustavo Padovan <padovan@profusion.mobi> w=
rote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-20 19:06:30 +030=
0]:
>
>> Hi Marcel,
>>
>> On Tue, Sep 20, 2011 at 6:11 PM, Marcel Holtmann <marcel@holtmann.org> w=
rote:
>> > Hi Luiz,
>> >
>> >> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> >> >> > ---
>> >> >> > =A0net/bluetooth/rfcomm/core.c | =A0 51 ++++++++++++++++++++++++=
+++++-------------
>> >> >> > =A0net/bluetooth/rfcomm/sock.c | =A0 =A02 +
>> >> >> > =A02 files changed, 37 insertions(+), 16 deletions(-)
>> >> >> >
>> >> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/=
core.c
>> >> >> > index 85580f2..bfc6bce 100644
>> >> >> > --- a/net/bluetooth/rfcomm/core.c
>> >> >> > +++ b/net/bluetooth/rfcomm/core.c
>> >> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
>> >> >> >
>> >> >> > =A0static LIST_HEAD(session_list);
>> >> >> >
>> >> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data=
, int len);
>> >> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data=
, int len,
>> >> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority);
>> >> >> > =A0static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci=
);
>> >> >> > =A0static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci=
);
>> >> >> > =A0static int rfcomm_queue_disc(struct rfcomm_dlc *d);
>> >> >> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_=
session *s, bdaddr_t *src, bdaddr_t *d
>> >> >> > =A0}
>> >> >> >
>> >> >> > =A0/* ---- RFCOMM frame sending ---- */
>> >> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data=
, int len)
>> >> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data=
, int len,
>> >> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority)
>> >> >> > =A0{
>> >> >> > =A0 =A0 struct socket *sock =3D s->sock;
>> >> >> > + =A0 struct sock *sk =3D sock->sk;
>> >> >> > =A0 =A0 struct kvec iv =3D { data, len };
>> >> >> > =A0 =A0 struct msghdr msg;
>> >> >> >
>> >> >> > - =A0 BT_DBG("session %p len %d", s, len);
>> >> >> > + =A0 BT_DBG("session %p len %d priority %u", s, len, priority);
>> >> >> > +
>> >> >> > + =A0 if (sk->sk_priority !=3D priority) {
>> >> >> > + =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
>> >> >> > + =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D priority;
>> >> >> > + =A0 =A0 =A0 =A0 =A0 release_sock(sk);
>> >> >> > + =A0 }
>> >> >> >
>> >> >> > =A0 =A0 memset(&msg, 0, sizeof(msg));
>> >> >> >
>> >> >> > =A0 =A0 return kernel_sendmsg(sock, &msg, &iv, 1, len);
>> >> >> > =A0}
>> >> >> >
>> >> >> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfc=
omm_cmd *cmd)
>> >> >> > +{
>> >> >> > + =A0 BT_DBG("%p cmd %u", s, cmd->ctrl);
>> >> >> > +
>> >> >> > + =A0 return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HC=
I_PRIO_MAX);
>> >> >>
>> >> >>
>> >> >> What's the idea here? Prioritize commands over data? But does this=
 really
>> >> >> happen? Because we have only one queue to receive the data in L2CA=
P. There
>> >> >> is no separation between data and cmd there.
>> >> >>
>> >> >> Also, did you check if we can send RFCOMM commands and data out of=
 order?
>> >> >>
>> >> >> I really would like to rewrite l2cap-rfcomm iteraction before addi=
ng new
>> >> >> features here.
>> >> >
>> >> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt cal=
ls
>> >> > return an error code. Priority on RFCOMM links is also rather point=
less
>> >> > since they all go via the same L2CAP PSM anyway. You would end up
>> >> > prioritizing all RFCOMM connections over any other L2CAP connection=
.
>> >>
>> >> Currently the priority is set per skb not per channel, so it is not a=
s
>> >> broken as you may think it is. Other than that you can't really ignor=
e
>> >> the priority for RFCOMM because as the priority will be not set in it=
s
>> >> skb it will be left 0 so it is low priority which for RFCOMM commands
>> >> may cause problems due latency being increased (remember it not a
>> >> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
>> >> specific and currently no error is return.
>> >
>> > and we have to super careful with any sort of potential re-ordering
>> > here. I rather not touch SKBs coming in from RFCOMM. They should stay =
as
>> > they are.
>>
>> The order is kept, and Im not sure how you want the queue discipline
>> to work but afaik that how other implementation handle it, by setting
>> skb->priority. The other option is to have the hci_chan handling the
>> priority, but that way it will just work as you assume per channel and
>> that cannot support RFCOMM at all.
>
> But in this way we will prioritize all RFCOMM channel at once, and this i=
s not
> the behaviour we want. It will be pointless if we all rfcomm based profil=
es
> are updated in the priority queue at the same time.

Not sure what to meant here, RFCOMM sockets should not support any
priority according to Marcel, it would be always 0, so there is no
updates or anything. The idea is just to make the code simpler since
RFCOMM would not be supported there is no clear benefit of doing the
prioritization per buffer, I still think that command latency for
RFCOMM can be a problem in some cases and that why Ive have the
priority per skb in the first place so we could properly prioritize
them, but as an option we can have it first per hci_chan and then
latter if needed have per skb.

--=20
Luiz Augusto von Dentz

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-20 19:06:30 +0300]:

> Hi Marcel,
>=20
> On Tue, Sep 20, 2011 at 6:11 PM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
> > Hi Luiz,
> >
> >> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >> >> > ---
> >> >> > =A0net/bluetooth/rfcomm/core.c | =A0 51 +++++++++++++++++++++++++=
++++-------------
> >> >> > =A0net/bluetooth/rfcomm/sock.c | =A0 =A02 +
> >> >> > =A02 files changed, 37 insertions(+), 16 deletions(-)
> >> >> >
> >> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/c=
ore.c
> >> >> > index 85580f2..bfc6bce 100644
> >> >> > --- a/net/bluetooth/rfcomm/core.c
> >> >> > +++ b/net/bluetooth/rfcomm/core.c
> >> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
> >> >> >
> >> >> > =A0static LIST_HEAD(session_list);
> >> >> >
> >> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data,=
 int len);
> >> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data,=
 int len,
> >> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority);
> >> >> > =A0static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
> >> >> > =A0static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
> >> >> > =A0static int rfcomm_queue_disc(struct rfcomm_dlc *d);
> >> >> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_s=
ession *s, bdaddr_t *src, bdaddr_t *d
> >> >> > =A0}
> >> >> >
> >> >> > =A0/* ---- RFCOMM frame sending ---- */
> >> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data,=
 int len)
> >> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data,=
 int len,
> >> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority)
> >> >> > =A0{
> >> >> > =A0 =A0 struct socket *sock =3D s->sock;
> >> >> > + =A0 struct sock *sk =3D sock->sk;
> >> >> > =A0 =A0 struct kvec iv =3D { data, len };
> >> >> > =A0 =A0 struct msghdr msg;
> >> >> >
> >> >> > - =A0 BT_DBG("session %p len %d", s, len);
> >> >> > + =A0 BT_DBG("session %p len %d priority %u", s, len, priority);
> >> >> > +
> >> >> > + =A0 if (sk->sk_priority !=3D priority) {
> >> >> > + =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
> >> >> > + =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D priority;
> >> >> > + =A0 =A0 =A0 =A0 =A0 release_sock(sk);
> >> >> > + =A0 }
> >> >> >
> >> >> > =A0 =A0 memset(&msg, 0, sizeof(msg));
> >> >> >
> >> >> > =A0 =A0 return kernel_sendmsg(sock, &msg, &iv, 1, len);
> >> >> > =A0}
> >> >> >
> >> >> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfco=
mm_cmd *cmd)
> >> >> > +{
> >> >> > + =A0 BT_DBG("%p cmd %u", s, cmd->ctrl);
> >> >> > +
> >> >> > + =A0 return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI=
_PRIO_MAX);
> >> >>
> >> >>
> >> >> What's the idea here? Prioritize commands over data? But does this =
really
> >> >> happen? Because we have only one queue to receive the data in L2CAP=
=2E There
> >> >> is no separation between data and cmd there.
> >> >>
> >> >> Also, did you check if we can send RFCOMM commands and data out of =
order?
> >> >>
> >> >> I really would like to rewrite l2cap-rfcomm iteraction before addin=
g new
> >> >> features here.
> >> >
> >> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
> >> > return an error code. Priority on RFCOMM links is also rather pointl=
ess
> >> > since they all go via the same L2CAP PSM anyway. You would end up
> >> > prioritizing all RFCOMM connections over any other L2CAP connection.
> >>
> >> Currently the priority is set per skb not per channel, so it is not as
> >> broken as you may think it is. Other than that you can't really ignore
> >> the priority for RFCOMM because as the priority will be not set in its
> >> skb it will be left 0 so it is low priority which for RFCOMM commands
> >> may cause problems due latency being increased (remember it not a
> >> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
> >> specific and currently no error is return.
> >
> > and we have to super careful with any sort of potential re-ordering
> > here. I rather not touch SKBs coming in from RFCOMM. They should stay as
> > they are.
>=20
> The order is kept, and Im not sure how you want the queue discipline
> to work but afaik that how other implementation handle it, by setting
> skb->priority. The other option is to have the hci_chan handling the
> priority, but that way it will just work as you assume per channel and
> that cannot support RFCOMM at all.

But in this way we will prioritize all RFCOMM channel at once, and this is =
not
the behaviour we want. It will be pointless if we all rfcomm based profiles
are updated in the priority queue at the same time.

	Gustavo

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Sep 20, 2011 at 6:11 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Luiz,
>
>> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> >> > ---
>> >> > =A0net/bluetooth/rfcomm/core.c | =A0 51 +++++++++++++++++++++++++++=
++-------------
>> >> > =A0net/bluetooth/rfcomm/sock.c | =A0 =A02 +
>> >> > =A02 files changed, 37 insertions(+), 16 deletions(-)
>> >> >
>> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/cor=
e.c
>> >> > index 85580f2..bfc6bce 100644
>> >> > --- a/net/bluetooth/rfcomm/core.c
>> >> > +++ b/net/bluetooth/rfcomm/core.c
>> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
>> >> >
>> >> > =A0static LIST_HEAD(session_list);
>> >> >
>> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, i=
nt len);
>> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, i=
nt len,
>> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority);
>> >> > =A0static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
>> >> > =A0static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
>> >> > =A0static int rfcomm_queue_disc(struct rfcomm_dlc *d);
>> >> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_ses=
sion *s, bdaddr_t *src, bdaddr_t *d
>> >> > =A0}
>> >> >
>> >> > =A0/* ---- RFCOMM frame sending ---- */
>> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, i=
nt len)
>> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, i=
nt len,
>> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority)
>> >> > =A0{
>> >> > =A0 =A0 struct socket *sock =3D s->sock;
>> >> > + =A0 struct sock *sk =3D sock->sk;
>> >> > =A0 =A0 struct kvec iv =3D { data, len };
>> >> > =A0 =A0 struct msghdr msg;
>> >> >
>> >> > - =A0 BT_DBG("session %p len %d", s, len);
>> >> > + =A0 BT_DBG("session %p len %d priority %u", s, len, priority);
>> >> > +
>> >> > + =A0 if (sk->sk_priority !=3D priority) {
>> >> > + =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
>> >> > + =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D priority;
>> >> > + =A0 =A0 =A0 =A0 =A0 release_sock(sk);
>> >> > + =A0 }
>> >> >
>> >> > =A0 =A0 memset(&msg, 0, sizeof(msg));
>> >> >
>> >> > =A0 =A0 return kernel_sendmsg(sock, &msg, &iv, 1, len);
>> >> > =A0}
>> >> >
>> >> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm=
_cmd *cmd)
>> >> > +{
>> >> > + =A0 BT_DBG("%p cmd %u", s, cmd->ctrl);
>> >> > +
>> >> > + =A0 return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_P=
RIO_MAX);
>> >>
>> >>
>> >> What's the idea here? Prioritize commands over data? But does this re=
ally
>> >> happen? Because we have only one queue to receive the data in L2CAP. =
There
>> >> is no separation between data and cmd there.
>> >>
>> >> Also, did you check if we can send RFCOMM commands and data out of or=
der?
>> >>
>> >> I really would like to rewrite l2cap-rfcomm iteraction before adding =
new
>> >> features here.
>> >
>> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
>> > return an error code. Priority on RFCOMM links is also rather pointles=
s
>> > since they all go via the same L2CAP PSM anyway. You would end up
>> > prioritizing all RFCOMM connections over any other L2CAP connection.
>>
>> Currently the priority is set per skb not per channel, so it is not as
>> broken as you may think it is. Other than that you can't really ignore
>> the priority for RFCOMM because as the priority will be not set in its
>> skb it will be left 0 so it is low priority which for RFCOMM commands
>> may cause problems due latency being increased (remember it not a
>> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
>> specific and currently no error is return.
>
> and we have to super careful with any sort of potential re-ordering
> here. I rather not touch SKBs coming in from RFCOMM. They should stay as
> they are.

The order is kept, and Im not sure how you want the queue discipline
to work but afaik that how other implementation handle it, by setting
skb->priority. The other option is to have the hci_chan handling the
priority, but that way it will just work as you assume per channel and
that cannot support RFCOMM at all.

>> > So if you try to prioritize HFP then you also prioritize PBAP in the e=
nd
>> > and we are back where we started. We could implement the 27.007 priori=
ty
>> > support inside RFCOMM, but that seems even more pointless endeavor.
>>
>> That is not the way it work, take a look at rfcomm_send_frame:
>>
>> + =A0 =A0 =A0 if (sk->sk_priority !=3D priority) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D priority;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_sock(sk);
>> + =A0 =A0 =A0 }
>>
>> This is suppose to dynamically updates the priority if it changes.
>
> As I said above, I rather not mess with this. Keep the RFCOMM stream as
> it is. L2CAP priority is essentially something different than RFCOMM
> priority. And I do not wanna go there right now.

Ordering is not a problem here just latency of commands, so the
messing is the other way round, if we don't prioritize the commands
its latency is messed.

>> > What we really want is prioritized L2CAP links and hopefully in the
>> > future everything moves over to use L2CAP directly anyway and RFCOMM
>> > will be slowly dying out.
>>
>> The problem here is not really RFCOMM as for L2CAP we also give
>> maximum priority to commands to avoid latency problems, this is
>> specially important when connecting because it will page but if either
>> L2CAP or RFCOMM commands are not properly prioritized they may timeout
>> so all the paging is wasted, btw this was very easy to emulate with
>> hid+a2dp then try to connect anything else.
>
> As pointed our earlier, we have to be really careful to not reorder
> command wrongly. If that happens we have a serious problem with the
> overall system.

Let me try to be clear, there is no reordering in the channel, it
continue to be a FIFO but it is now one for each channel not one per
connection and Im fine to skip this patch but Im not sure if you guys
really understand what I was trying to solve with it.

--=20
Luiz Augusto von Dentz

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Marcel Holtmann]

Hi Luiz,

> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >> > ---
> >> >  net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
> >> >  net/bluetooth/rfcomm/sock.c |    2 +
> >> >  2 files changed, 37 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> >> > index 85580f2..bfc6bce 100644
> >> > --- a/net/bluetooth/rfcomm/core.c
> >> > +++ b/net/bluetooth/rfcomm/core.c
> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
> >> >
> >> >  static LIST_HEAD(session_list);
> >> >
> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > +                                                   u32 priority);
> >> >  static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
> >> >  static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
> >> >  static int rfcomm_queue_disc(struct rfcomm_dlc *d);
> >> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
> >> >  }
> >> >
> >> >  /* ---- RFCOMM frame sending ---- */
> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > +                                                   u32 priority)
> >> >  {
> >> >     struct socket *sock = s->sock;
> >> > +   struct sock *sk = sock->sk;
> >> >     struct kvec iv = { data, len };
> >> >     struct msghdr msg;
> >> >
> >> > -   BT_DBG("session %p len %d", s, len);
> >> > +   BT_DBG("session %p len %d priority %u", s, len, priority);
> >> > +
> >> > +   if (sk->sk_priority != priority) {
> >> > +           lock_sock(sk);
> >> > +           sk->sk_priority = priority;
> >> > +           release_sock(sk);
> >> > +   }
> >> >
> >> >     memset(&msg, 0, sizeof(msg));
> >> >
> >> >     return kernel_sendmsg(sock, &msg, &iv, 1, len);
> >> >  }
> >> >
> >> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
> >> > +{
> >> > +   BT_DBG("%p cmd %u", s, cmd->ctrl);
> >> > +
> >> > +   return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
> >>
> >>
> >> What's the idea here? Prioritize commands over data? But does this really
> >> happen? Because we have only one queue to receive the data in L2CAP. There
> >> is no separation between data and cmd there.
> >>
> >> Also, did you check if we can send RFCOMM commands and data out of order?
> >>
> >> I really would like to rewrite l2cap-rfcomm iteraction before adding new
> >> features here.
> >
> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
> > return an error code. Priority on RFCOMM links is also rather pointless
> > since they all go via the same L2CAP PSM anyway. You would end up
> > prioritizing all RFCOMM connections over any other L2CAP connection.
> 
> Currently the priority is set per skb not per channel, so it is not as
> broken as you may think it is. Other than that you can't really ignore
> the priority for RFCOMM because as the priority will be not set in its
> skb it will be left 0 so it is low priority which for RFCOMM commands
> may cause problems due latency being increased (remember it not a
> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
> specific and currently no error is return.

and we have to super careful with any sort of potential re-ordering
here. I rather not touch SKBs coming in from RFCOMM. They should stay as
they are.

> > So if you try to prioritize HFP then you also prioritize PBAP in the end
> > and we are back where we started. We could implement the 27.007 priority
> > support inside RFCOMM, but that seems even more pointless endeavor.
> 
> That is not the way it work, take a look at rfcomm_send_frame:
> 
> +       if (sk->sk_priority != priority) {
> +               lock_sock(sk);
> +               sk->sk_priority = priority;
> +               release_sock(sk);
> +       }
> 
> This is suppose to dynamically updates the priority if it changes.

As I said above, I rather not mess with this. Keep the RFCOMM stream as
it is. L2CAP priority is essentially something different than RFCOMM
priority. And I do not wanna go there right now.

> > What we really want is prioritized L2CAP links and hopefully in the
> > future everything moves over to use L2CAP directly anyway and RFCOMM
> > will be slowly dying out.
> 
> The problem here is not really RFCOMM as for L2CAP we also give
> maximum priority to commands to avoid latency problems, this is
> specially important when connecting because it will page but if either
> L2CAP or RFCOMM commands are not properly prioritized they may timeout
> so all the paging is wasted, btw this was very easy to emulate with
> hid+a2dp then try to connect anything else.

As pointed our earlier, we have to be really careful to not reorder
command wrongly. If that happens we have a serious problem with the
overall system.

Regards

Marcel

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Sep 20, 2011 at 4:04 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Gustavo,
>
>> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> > ---
>> > =A0net/bluetooth/rfcomm/core.c | =A0 51 +++++++++++++++++++++++++++++-=
------------
>> > =A0net/bluetooth/rfcomm/sock.c | =A0 =A02 +
>> > =A02 files changed, 37 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> > index 85580f2..bfc6bce 100644
>> > --- a/net/bluetooth/rfcomm/core.c
>> > +++ b/net/bluetooth/rfcomm/core.c
>> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
>> >
>> > =A0static LIST_HEAD(session_list);
>> >
>> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int =
len);
>> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int =
len,
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority);
>> > =A0static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
>> > =A0static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
>> > =A0static int rfcomm_queue_disc(struct rfcomm_dlc *d);
>> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_sessio=
n *s, bdaddr_t *src, bdaddr_t *d
>> > =A0}
>> >
>> > =A0/* ---- RFCOMM frame sending ---- */
>> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int =
len)
>> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int =
len,
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 priority)
>> > =A0{
>> > =A0 =A0 struct socket *sock =3D s->sock;
>> > + =A0 struct sock *sk =3D sock->sk;
>> > =A0 =A0 struct kvec iv =3D { data, len };
>> > =A0 =A0 struct msghdr msg;
>> >
>> > - =A0 BT_DBG("session %p len %d", s, len);
>> > + =A0 BT_DBG("session %p len %d priority %u", s, len, priority);
>> > +
>> > + =A0 if (sk->sk_priority !=3D priority) {
>> > + =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
>> > + =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D priority;
>> > + =A0 =A0 =A0 =A0 =A0 release_sock(sk);
>> > + =A0 }
>> >
>> > =A0 =A0 memset(&msg, 0, sizeof(msg));
>> >
>> > =A0 =A0 return kernel_sendmsg(sock, &msg, &iv, 1, len);
>> > =A0}
>> >
>> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cm=
d *cmd)
>> > +{
>> > + =A0 BT_DBG("%p cmd %u", s, cmd->ctrl);
>> > +
>> > + =A0 return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO=
_MAX);
>>
>>
>> What's the idea here? Prioritize commands over data? But does this reall=
y
>> happen? Because we have only one queue to receive the data in L2CAP. The=
re
>> is no separation between data and cmd there.
>>
>> Also, did you check if we can send RFCOMM commands and data out of order=
?
>>
>> I really would like to rewrite l2cap-rfcomm iteraction before adding new
>> features here.
>
> lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
> return an error code. Priority on RFCOMM links is also rather pointless
> since they all go via the same L2CAP PSM anyway. You would end up
> prioritizing all RFCOMM connections over any other L2CAP connection.

Currently the priority is set per skb not per channel, so it is not as
broken as you may think it is. Other than that you can't really ignore
the priority for RFCOMM because as the priority will be not set in its
skb it will be left 0 so it is low priority which for RFCOMM commands
may cause problems due latency being increased (remember it not a
simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
specific and currently no error is return.

> So if you try to prioritize HFP then you also prioritize PBAP in the end
> and we are back where we started. We could implement the 27.007 priority
> support inside RFCOMM, but that seems even more pointless endeavor.

That is not the way it work, take a look at rfcomm_send_frame:

+       if (sk->sk_priority !=3D priority) {
+               lock_sock(sk);
+               sk->sk_priority =3D priority;
+               release_sock(sk);
+       }

This is suppose to dynamically updates the priority if it changes.

> What we really want is prioritized L2CAP links and hopefully in the
> future everything moves over to use L2CAP directly anyway and RFCOMM
> will be slowly dying out.

The problem here is not really RFCOMM as for L2CAP we also give
maximum priority to commands to avoid latency problems, this is
specially important when connecting because it will page but if either
L2CAP or RFCOMM commands are not properly prioritized they may timeout
so all the paging is wasted, btw this was very easy to emulate with
hid+a2dp then try to connect anything else.

--=20
Luiz Augusto von Dentz

RE: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[tim.howes]

SGkgTWFyY2VsLA0KDQoNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBs
aW51eC1ibHVldG9vdGgtb3duZXJAdmdlci5rZXJuZWwub3JnIFttYWlsdG86bGludXgtYmx1ZXRv
b3RoLQ0KPiBvd25lckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBPZiBNYXJjZWwgSG9sdG1h
bm4NCj4gU2VudDogMjAgU2VwdGVtYmVyIDIwMTEgMTQ6MDUNCj4gVG86IEd1c3Rhdm8gUGFkb3Zh
bg0KPiBDYzogTHVpeiBBdWd1c3RvIHZvbiBEZW50ejsgbGludXgtYmx1ZXRvb3RoQHZnZXIua2Vy
bmVsLm9yZw0KPiBTdWJqZWN0OiBSZTogW1JGQyAzLzUgdjJdIEJsdWV0b290aDogbWFrZSB1c2Ug
c2tfcHJpb3JpdHkgdG8gcHJpcml0aXplDQo+IFJGQ09NTSBwYWNrZXRzDQo+DQo+IEhpIEd1c3Rh
dm8sDQo+DQo+ID4gPiBTaWduZWQtb2ZmLWJ5OiBMdWl6IEF1Z3VzdG8gdm9uIERlbnR6IDxsdWl6
LnZvbi5kZW50ekBpbnRlbC5jb20+DQo+ID4gPiAtLS0NCj4gPiA+ICBuZXQvYmx1ZXRvb3RoL3Jm
Y29tbS9jb3JlLmMgfCAgIDUxICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0NCj4gLS0t
LS0tLS0tLS0NCj4gPiA+ICBuZXQvYmx1ZXRvb3RoL3JmY29tbS9zb2NrLmMgfCAgICAyICsNCj4g
PiA+ICAyIGZpbGVzIGNoYW5nZWQsIDM3IGluc2VydGlvbnMoKyksIDE2IGRlbGV0aW9ucygtKQ0K
PiA+ID4NCj4gPiA+IGRpZmYgLS1naXQgYS9uZXQvYmx1ZXRvb3RoL3JmY29tbS9jb3JlLmMNCj4g
Yi9uZXQvYmx1ZXRvb3RoL3JmY29tbS9jb3JlLmMNCj4gPiA+IGluZGV4IDg1NTgwZjIuLmJmYzZi
Y2UgMTAwNjQ0DQo+ID4gPiAtLS0gYS9uZXQvYmx1ZXRvb3RoL3JmY29tbS9jb3JlLmMNCj4gPiA+
ICsrKyBiL25ldC9ibHVldG9vdGgvcmZjb21tL2NvcmUuYw0KPiA+ID4gQEAgLTY1LDcgKzY1LDgg
QEAgc3RhdGljIERFRklORV9NVVRFWChyZmNvbW1fbXV0ZXgpOw0KPiA+ID4NCj4gPiA+ICBzdGF0
aWMgTElTVF9IRUFEKHNlc3Npb25fbGlzdCk7DQo+ID4gPg0KPiA+ID4gLXN0YXRpYyBpbnQgcmZj
b21tX3NlbmRfZnJhbWUoc3RydWN0IHJmY29tbV9zZXNzaW9uICpzLCB1OCAqZGF0YSwNCj4gaW50
IGxlbik7DQo+ID4gPiArc3RhdGljIGludCByZmNvbW1fc2VuZF9mcmFtZShzdHJ1Y3QgcmZjb21t
X3Nlc3Npb24gKnMsIHU4ICpkYXRhLA0KPiBpbnQgbGVuLA0KPiA+ID4gKyAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB1MzIgcHJpb3JpdHkpOw0KPiA+ID4g
IHN0YXRpYyBpbnQgcmZjb21tX3NlbmRfc2FibShzdHJ1Y3QgcmZjb21tX3Nlc3Npb24gKnMsIHU4
IGRsY2kpOw0KPiA+ID4gIHN0YXRpYyBpbnQgcmZjb21tX3NlbmRfZGlzYyhzdHJ1Y3QgcmZjb21t
X3Nlc3Npb24gKnMsIHU4IGRsY2kpOw0KPiA+ID4gIHN0YXRpYyBpbnQgcmZjb21tX3F1ZXVlX2Rp
c2Moc3RydWN0IHJmY29tbV9kbGMgKmQpOw0KPiA+ID4gQEAgLTc0NywxOSArNzQ4LDM0IEBAIHZv
aWQgcmZjb21tX3Nlc3Npb25fZ2V0YWRkcihzdHJ1Y3QNCj4gcmZjb21tX3Nlc3Npb24gKnMsIGJk
YWRkcl90ICpzcmMsIGJkYWRkcl90ICpkDQo+ID4gPiAgfQ0KPiA+ID4NCj4gPiA+ICAvKiAtLS0t
IFJGQ09NTSBmcmFtZSBzZW5kaW5nIC0tLS0gKi8NCj4gPiA+IC1zdGF0aWMgaW50IHJmY29tbV9z
ZW5kX2ZyYW1lKHN0cnVjdCByZmNvbW1fc2Vzc2lvbiAqcywgdTggKmRhdGEsDQo+IGludCBsZW4p
DQo+ID4gPiArc3RhdGljIGludCByZmNvbW1fc2VuZF9mcmFtZShzdHJ1Y3QgcmZjb21tX3Nlc3Np
b24gKnMsIHU4ICpkYXRhLA0KPiBpbnQgbGVuLA0KPiA+ID4gKyAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICB1MzIgcHJpb3JpdHkpDQo+ID4gPiAgew0KPiA+
ID4gICBzdHJ1Y3Qgc29ja2V0ICpzb2NrID0gcy0+c29jazsNCj4gPiA+ICsgc3RydWN0IHNvY2sg
KnNrID0gc29jay0+c2s7DQo+ID4gPiAgIHN0cnVjdCBrdmVjIGl2ID0geyBkYXRhLCBsZW4gfTsN
Cj4gPiA+ICAgc3RydWN0IG1zZ2hkciBtc2c7DQo+ID4gPg0KPiA+ID4gLSBCVF9EQkcoInNlc3Np
b24gJXAgbGVuICVkIiwgcywgbGVuKTsNCj4gPiA+ICsgQlRfREJHKCJzZXNzaW9uICVwIGxlbiAl
ZCBwcmlvcml0eSAldSIsIHMsIGxlbiwgcHJpb3JpdHkpOw0KPiA+ID4gKw0KPiA+ID4gKyBpZiAo
c2stPnNrX3ByaW9yaXR5ICE9IHByaW9yaXR5KSB7DQo+ID4gPiArICAgICAgICAgbG9ja19zb2Nr
KHNrKTsNCj4gPiA+ICsgICAgICAgICBzay0+c2tfcHJpb3JpdHkgPSBwcmlvcml0eTsNCj4gPiA+
ICsgICAgICAgICByZWxlYXNlX3NvY2soc2spOw0KPiA+ID4gKyB9DQo+ID4gPg0KPiA+ID4gICBt
ZW1zZXQoJm1zZywgMCwgc2l6ZW9mKG1zZykpOw0KPiA+ID4NCj4gPiA+ICAgcmV0dXJuIGtlcm5l
bF9zZW5kbXNnKHNvY2ssICZtc2csICZpdiwgMSwgbGVuKTsNCj4gPiA+ICB9DQo+ID4gPg0KPiA+
ID4gK3N0YXRpYyBpbnQgcmZjb21tX3NlbmRfY21kKHN0cnVjdCByZmNvbW1fc2Vzc2lvbiAqcywg
c3RydWN0DQo+IHJmY29tbV9jbWQgKmNtZCkNCj4gPiA+ICt7DQo+ID4gPiArIEJUX0RCRygiJXAg
Y21kICV1IiwgcywgY21kLT5jdHJsKTsNCj4gPiA+ICsNCj4gPiA+ICsgcmV0dXJuIHJmY29tbV9z
ZW5kX2ZyYW1lKHMsICh2b2lkICopIGNtZCwgc2l6ZW9mKCpjbWQpLA0KPiBIQ0lfUFJJT19NQVgp
Ow0KPiA+DQo+ID4NCj4gPiBXaGF0J3MgdGhlIGlkZWEgaGVyZT8gUHJpb3JpdGl6ZSBjb21tYW5k
cyBvdmVyIGRhdGE/IEJ1dCBkb2VzIHRoaXMNCj4gcmVhbGx5DQo+ID4gaGFwcGVuPyBCZWNhdXNl
IHdlIGhhdmUgb25seSBvbmUgcXVldWUgdG8gcmVjZWl2ZSB0aGUgZGF0YSBpbiBMMkNBUC4NCj4g
VGhlcmUNCj4gPiBpcyBubyBzZXBhcmF0aW9uIGJldHdlZW4gZGF0YSBhbmQgY21kIHRoZXJlLg0K
PiA+DQo+ID4gQWxzbywgZGlkIHlvdSBjaGVjayBpZiB3ZSBjYW4gc2VuZCBSRkNPTU0gY29tbWFu
ZHMgYW5kIGRhdGEgb3V0IG9mDQo+IG9yZGVyPw0KPiA+DQo+ID4gSSByZWFsbHkgd291bGQgbGlr
ZSB0byByZXdyaXRlIGwyY2FwLXJmY29tbSBpdGVyYWN0aW9uIGJlZm9yZSBhZGRpbmcNCj4gbmV3
DQo+ID4gZmVhdHVyZXMgaGVyZS4NCj4NCj4gbGV0cyBqdXN0IGZvcmdldCBSRkNPTU0gZm9yIG5v
dyBhbmQgbWFrZSBTT19QUklPUklUWSBzZXRzb2Nrb3B0IGNhbGxzDQo+IHJldHVybiBhbiBlcnJv
ciBjb2RlLiBQcmlvcml0eSBvbiBSRkNPTU0gbGlua3MgaXMgYWxzbyByYXRoZXIgcG9pbnRsZXNz
DQo+IHNpbmNlIHRoZXkgYWxsIGdvIHZpYSB0aGUgc2FtZSBMMkNBUCBQU00gYW55d2F5LiBZb3Ug
d291bGQgZW5kIHVwDQo+IHByaW9yaXRpemluZyBhbGwgUkZDT01NIGNvbm5lY3Rpb25zIG92ZXIg
YW55IG90aGVyIEwyQ0FQIGNvbm5lY3Rpb24uDQo+DQo+IFNvIGlmIHlvdSB0cnkgdG8gcHJpb3Jp
dGl6ZSBIRlAgdGhlbiB5b3UgYWxzbyBwcmlvcml0aXplIFBCQVAgaW4gdGhlDQo+IGVuZA0KPiBh
bmQgd2UgYXJlIGJhY2sgd2hlcmUgd2Ugc3RhcnRlZC4gV2UgY291bGQgaW1wbGVtZW50IHRoZSAy
Ny4wMDcNCj4gcHJpb3JpdHkNCj4gc3VwcG9ydCBpbnNpZGUgUkZDT01NLCBidXQgdGhhdCBzZWVt
cyBldmVuIG1vcmUgcG9pbnRsZXNzIGVuZGVhdm9yLg0KPg0KPiBXaGF0IHdlIHJlYWxseSB3YW50
IGlzIHByaW9yaXRpemVkIEwyQ0FQIGxpbmtzIGFuZCBob3BlZnVsbHkgaW4gdGhlDQo+IGZ1dHVy
ZSBldmVyeXRoaW5nIG1vdmVzIG92ZXIgdG8gdXNlIEwyQ0FQIGRpcmVjdGx5IGFueXdheSBhbmQg
UkZDT01NDQo+IHdpbGwgYmUgc2xvd2x5IGR5aW5nIG91dC4NCj4NCj4gUmVnYXJkcw0KPg0KPiBN
YXJjZWwNCg0KWWVwOiBPbmUgb2YgdGhlIG1vdGl2YXRpb25zIGZvciBPQkVYIG92ZXIgTDJDQVAg
aXMgdG8gc29ydCBvdXQgdGhlIG1lc3Mgb2YgdGhlIFJGQ09NTSBtdXggb24gdG9wIG9mIHRoZSBM
MkNBUCBtdXguICBXaGVuIE9CRVggcHJvZmlsZXMgcnVuIG92ZXIgdGhlaXIgb3duIEwyQ0FQIGNo
YW5uZWxzICh3aGljaCBjYW4gYmUgc2VwYXJhdGVseSBwcmlvcml0aXplZCksIGl0IHdpbGwgbGVh
dmUgYmVoaW5kICJvbmx5IiBIRlAgd2hpY2ggd291bGQgaGF2ZSBhbG1vc3QgZXhjbHVzaXZlIHVz
ZSBvZiBSRkNPTU0gKGFuZCBvZiBjb3Vyc2UgUEFOIHRvIGRpc3BsYWNlIERVTi4uLikNCg0KU28s
IEkgYWdyZWU6IGxlYXZlIFJGQ09NTSBhcyBpcywgYW5kIGVuY291cmFnZSBPQkVYIG92ZXIgTDJD
QVAgaW1wbGVtZW50YXRpb25zIG9mIE9CRVggcHJvZmlsZXMgYXMgYSBzb2x1dGlvbiB0byBwcmlv
cml0aXphdGlvbi4NCg0KVGltDQoNCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fDQpU
aGlzIG1lc3NhZ2UgaXMgZm9yIHRoZSBkZXNpZ25hdGVkIHJlY2lwaWVudCBvbmx5IGFuZCBtYXkg
Y29udGFpbiBwcml2aWxlZ2VkLCBwcm9wcmlldGFyeSwgb3Igb3RoZXJ3aXNlIHByaXZhdGUgaW5m
b3JtYXRpb24uIElmIHlvdSBoYXZlIHJlY2VpdmVkIGl0IGluIGVycm9yLCBwbGVhc2Ugbm90aWZ5
IHRoZSBzZW5kZXIgaW1tZWRpYXRlbHkgYW5kIGRlbGV0ZSB0aGUgb3JpZ2luYWwuIEFueSBvdGhl
ciB1c2Ugb2YgdGhlIGVtYWlsIGJ5IHlvdSBpcyBwcm9oaWJpdGVkLg0K

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Marcel Holtmann]

Hi Gustavo,

> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
> >  net/bluetooth/rfcomm/sock.c |    2 +
> >  2 files changed, 37 insertions(+), 16 deletions(-)
> > 
> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> > index 85580f2..bfc6bce 100644
> > --- a/net/bluetooth/rfcomm/core.c
> > +++ b/net/bluetooth/rfcomm/core.c
> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
> >  
> >  static LIST_HEAD(session_list);
> >  
> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> > +							u32 priority);
> >  static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
> >  static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
> >  static int rfcomm_queue_disc(struct rfcomm_dlc *d);
> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
> >  }
> >  
> >  /* ---- RFCOMM frame sending ---- */
> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> > +							u32 priority)
> >  {
> >  	struct socket *sock = s->sock;
> > +	struct sock *sk = sock->sk;
> >  	struct kvec iv = { data, len };
> >  	struct msghdr msg;
> >  
> > -	BT_DBG("session %p len %d", s, len);
> > +	BT_DBG("session %p len %d priority %u", s, len, priority);
> > +
> > +	if (sk->sk_priority != priority) {
> > +		lock_sock(sk);
> > +		sk->sk_priority = priority;
> > +		release_sock(sk);
> > +	}
> >  
> >  	memset(&msg, 0, sizeof(msg));
> >  
> >  	return kernel_sendmsg(sock, &msg, &iv, 1, len);
> >  }
> >  
> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
> > +{
> > +	BT_DBG("%p cmd %u", s, cmd->ctrl);
> > +
> > +	return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
> 
> 
> What's the idea here? Prioritize commands over data? But does this really
> happen? Because we have only one queue to receive the data in L2CAP. There
> is no separation between data and cmd there.
> 
> Also, did you check if we can send RFCOMM commands and data out of order?
> 
> I really would like to rewrite l2cap-rfcomm iteraction before adding new
> features here.

lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
return an error code. Priority on RFCOMM links is also rather pointless
since they all go via the same L2CAP PSM anyway. You would end up
prioritizing all RFCOMM connections over any other L2CAP connection.

So if you try to prioritize HFP then you also prioritize PBAP in the end
and we are back where we started. We could implement the 27.007 priority
support inside RFCOMM, but that seems even more pointless endeavor.

What we really want is prioritized L2CAP links and hopefully in the
future everything moves over to use L2CAP directly anyway and RFCOMM
will be slowly dying out.

Regards

Marcel

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

Hi Gustavo,

On Tue, Sep 20, 2011 at 12:45 AM, Gustavo Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-12 20:00:51 +0300]:
>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> ---
>>  net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
>>  net/bluetooth/rfcomm/sock.c |    2 +
>>  2 files changed, 37 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index 85580f2..bfc6bce 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
>>
>>  static LIST_HEAD(session_list);
>>
>> -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
>> +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
>> +                                                     u32 priority);
>>  static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
>>  static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
>>  static int rfcomm_queue_disc(struct rfcomm_dlc *d);
>> @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
>>  }
>>
>>  /* ---- RFCOMM frame sending ---- */
>> -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
>> +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
>> +                                                     u32 priority)
>>  {
>>       struct socket *sock = s->sock;
>> +     struct sock *sk = sock->sk;
>>       struct kvec iv = { data, len };
>>       struct msghdr msg;
>>
>> -     BT_DBG("session %p len %d", s, len);
>> +     BT_DBG("session %p len %d priority %u", s, len, priority);
>> +
>> +     if (sk->sk_priority != priority) {
>> +             lock_sock(sk);
>> +             sk->sk_priority = priority;
>> +             release_sock(sk);
>> +     }
>>
>>       memset(&msg, 0, sizeof(msg));
>>
>>       return kernel_sendmsg(sock, &msg, &iv, 1, len);
>>  }
>>
>> +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
>> +{
>> +     BT_DBG("%p cmd %u", s, cmd->ctrl);
>> +
>> +     return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
>
>
> What's the idea here? Prioritize commands over data? But does this really
> happen? Because we have only one queue to receive the data in L2CAP. There
> is no separation between data and cmd there.

Latency problems as I explained in the other email.

> Also, did you check if we can send RFCOMM commands and data out of order?

Im not sure about that, but by using only one queue the ordering per
channel is not changed, command got high priority to avoid problems
with latency but we don't sort the queues.

> I really would like to rewrite l2cap-rfcomm iteraction before adding new
> features here.

Im afraid we can't wait in this case, this would hold the whole HCI
prioritization, but the changes to RFCOMM are not that big either it
is more to cope with the changes in the scheduler than introducing new
features.

-- 
Luiz Augusto von Dentz

Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-12 20:00:51 +0300]:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
>  net/bluetooth/rfcomm/sock.c |    2 +
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 85580f2..bfc6bce 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
>  
>  static LIST_HEAD(session_list);
>  
> -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
> +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> +							u32 priority);
>  static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
>  static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
>  static int rfcomm_queue_disc(struct rfcomm_dlc *d);
> @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
>  }
>  
>  /* ---- RFCOMM frame sending ---- */
> -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
> +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> +							u32 priority)
>  {
>  	struct socket *sock = s->sock;
> +	struct sock *sk = sock->sk;
>  	struct kvec iv = { data, len };
>  	struct msghdr msg;
>  
> -	BT_DBG("session %p len %d", s, len);
> +	BT_DBG("session %p len %d priority %u", s, len, priority);
> +
> +	if (sk->sk_priority != priority) {
> +		lock_sock(sk);
> +		sk->sk_priority = priority;
> +		release_sock(sk);
> +	}
>  
>  	memset(&msg, 0, sizeof(msg));
>  
>  	return kernel_sendmsg(sock, &msg, &iv, 1, len);
>  }
>  
> +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
> +{
> +	BT_DBG("%p cmd %u", s, cmd->ctrl);
> +
> +	return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);


What's the idea here? Prioritize commands over data? But does this really
happen? Because we have only one queue to receive the data in L2CAP. There
is no separation between data and cmd there.

Also, did you check if we can send RFCOMM commands and data out of order?

I really would like to rewrite l2cap-rfcomm iteraction before adding new
features here.


	Gustavo

[RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
 net/bluetooth/rfcomm/sock.c |    2 +
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 85580f2..bfc6bce 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
 
 static LIST_HEAD(session_list);
 
-static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
+							u32 priority);
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
 static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
 static int rfcomm_queue_disc(struct rfcomm_dlc *d);
@@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
 }
 
 /* ---- RFCOMM frame sending ---- */
-static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
+							u32 priority)
 {
 	struct socket *sock = s->sock;
+	struct sock *sk = sock->sk;
 	struct kvec iv = { data, len };
 	struct msghdr msg;
 
-	BT_DBG("session %p len %d", s, len);
+	BT_DBG("session %p len %d priority %u", s, len, priority);
+
+	if (sk->sk_priority != priority) {
+		lock_sock(sk);
+		sk->sk_priority = priority;
+		release_sock(sk);
+	}
 
 	memset(&msg, 0, sizeof(msg));
 
 	return kernel_sendmsg(sock, &msg, &iv, 1, len);
 }
 
+static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
+{
+	BT_DBG("%p cmd %u", s, cmd->ctrl);
+
+	return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
+}
+
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci)
 {
 	struct rfcomm_cmd cmd;
@@ -771,7 +787,7 @@ static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_ua(struct rfcomm_session *s, u8 dlci)
@@ -785,7 +801,7 @@ static int rfcomm_send_ua(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci)
@@ -799,7 +815,7 @@ static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_queue_disc(struct rfcomm_dlc *d)
@@ -813,6 +829,8 @@ static int rfcomm_queue_disc(struct rfcomm_dlc *d)
 	if (!skb)
 		return -ENOMEM;
 
+	skb->priority = HCI_PRIO_MAX;
+
 	cmd = (void *) __skb_put(skb, sizeof(*cmd));
 	cmd->addr = d->addr;
 	cmd->ctrl = __ctrl(RFCOMM_DISC, 1);
@@ -835,7 +853,7 @@ static int rfcomm_send_dm(struct rfcomm_session *s, u8 dlci)
 	cmd.len  = __len8(0);
 	cmd.fcs  = __fcs2((u8 *) &cmd);
 
-	return rfcomm_send_frame(s, (void *) &cmd, sizeof(cmd));
+	return rfcomm_send_cmd(s, &cmd);
 }
 
 static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type)
@@ -860,7 +878,7 @@ static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d)
@@ -902,7 +920,7 @@ static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci,
@@ -940,7 +958,7 @@ int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci,
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status)
@@ -967,7 +985,7 @@ static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig)
@@ -994,7 +1012,7 @@ static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
@@ -1016,7 +1034,7 @@ static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
@@ -1038,7 +1056,7 @@ static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static int rfcomm_send_test(struct rfcomm_session *s, int cr, u8 *pattern, int len)
@@ -1089,7 +1107,7 @@ static int rfcomm_send_credits(struct rfcomm_session *s, u8 addr, u8 credits)
 
 	*ptr = __fcs(buf); ptr++;
 
-	return rfcomm_send_frame(s, buf, ptr - buf);
+	return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
 }
 
 static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
@@ -1767,7 +1785,8 @@ static inline int rfcomm_process_tx(struct rfcomm_dlc *d)
 		return skb_queue_len(&d->tx_queue);
 
 	while (d->tx_credits && (skb = skb_dequeue(&d->tx_queue))) {
-		err = rfcomm_send_frame(d->session, skb->data, skb->len);
+		err = rfcomm_send_frame(d->session, skb->data, skb->len,
+							skb->priority);
 		if (err < 0) {
 			skb_queue_head(&d->tx_queue, skb);
 			break;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 482722b..40988e2 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -597,6 +597,8 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 			break;
 		}
 
+		skb->priority = sk->sk_priority;
+
 		err = rfcomm_dlc_send(d, skb);
 		if (err < 0) {
 			kfree_skb(skb);
-- 
1.7.6.1