* [RFC 2/5 v3] Bluetooth: mark l2cap_create_iframe_pdu as static
2011-09-12 17:00 [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Luiz Augusto von Dentz
@ 2011-09-12 17:00 ` Luiz Augusto von Dentz
2011-09-19 21:17 ` Gustavo Padovan
2011-09-12 17:00 ` [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets Luiz Augusto von Dentz
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-12 17:00 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
l2cap_create_iframe_pdu is only used in l2cap_core.c
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/l2cap_core.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b6e1ae..afd5455 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1531,7 +1531,9 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
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,
+ u16 control, u16 sdulen)
{
struct sock *sk = chan->sk;
struct l2cap_conn *conn = chan->conn;
--
1.7.6.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-12 17:00 [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Luiz Augusto von Dentz
2011-09-12 17:00 ` [RFC 2/5 v3] Bluetooth: mark l2cap_create_iframe_pdu as static Luiz Augusto von Dentz
@ 2011-09-12 17:00 ` Luiz Augusto von Dentz
2011-09-19 21:45 ` Gustavo Padovan
2011-09-12 17:00 ` [RFC 4/5 v3] Bluetooth: prioritizing data over HCI Luiz Augusto von Dentz
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-12 17:00 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-12 17:00 ` [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets Luiz Augusto von Dentz
@ 2011-09-19 21:45 ` Gustavo Padovan
2011-09-20 9:10 ` Luiz Augusto von Dentz
2011-09-20 13:04 ` Marcel Holtmann
0 siblings, 2 replies; 20+ messages in thread
From: Gustavo Padovan @ 2011-09-19 21:45 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-19 21:45 ` Gustavo Padovan
@ 2011-09-20 9:10 ` Luiz Augusto von Dentz
2011-09-20 13:04 ` Marcel Holtmann
1 sibling, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-20 9:10 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-19 21:45 ` Gustavo Padovan
2011-09-20 9:10 ` Luiz Augusto von Dentz
@ 2011-09-20 13:04 ` Marcel Holtmann
2011-09-20 13:46 ` tim.howes
2011-09-20 14:34 ` Luiz Augusto von Dentz
1 sibling, 2 replies; 20+ messages in thread
From: Marcel Holtmann @ 2011-09-20 13:04 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: Luiz Augusto von Dentz, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 13:04 ` Marcel Holtmann
@ 2011-09-20 13:46 ` tim.howes
2011-09-20 14:34 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 20+ messages in thread
From: tim.howes @ 2011-09-20 13:46 UTC (permalink / raw)
To: marcel, padovan; +Cc: luiz.dentz, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 13:04 ` Marcel Holtmann
2011-09-20 13:46 ` tim.howes
@ 2011-09-20 14:34 ` Luiz Augusto von Dentz
2011-09-20 15:11 ` Marcel Holtmann
1 sibling, 1 reply; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-20 14:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Gustavo Padovan, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 14:34 ` Luiz Augusto von Dentz
@ 2011-09-20 15:11 ` Marcel Holtmann
2011-09-20 16:06 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 20+ messages in thread
From: Marcel Holtmann @ 2011-09-20 15:11 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Gustavo Padovan, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 15:11 ` Marcel Holtmann
@ 2011-09-20 16:06 ` Luiz Augusto von Dentz
2011-09-20 16:59 ` Gustavo Padovan
0 siblings, 1 reply; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-20 16:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Gustavo Padovan, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 16:06 ` Luiz Augusto von Dentz
@ 2011-09-20 16:59 ` Gustavo Padovan
2011-09-21 11:20 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 20+ messages in thread
From: Gustavo Padovan @ 2011-09-20 16:59 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-09-20 16:59 ` Gustavo Padovan
@ 2011-09-21 11:20 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-21 11:20 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Marcel Holtmann, linux-bluetooth
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 4/5 v3] Bluetooth: prioritizing data over HCI
2011-09-12 17:00 [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Luiz Augusto von Dentz
2011-09-12 17:00 ` [RFC 2/5 v3] Bluetooth: mark l2cap_create_iframe_pdu as static Luiz Augusto von Dentz
2011-09-12 17:00 ` [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets Luiz Augusto von Dentz
@ 2011-09-12 17:00 ` Luiz Augusto von Dentz
2011-09-15 23:34 ` Mat Martineau
2011-09-12 17:00 ` [RFC 5/5 v3] Bluetooth: recalculate priorities when channels are starving Luiz Augusto von Dentz
2011-09-19 21:36 ` [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Gustavo Padovan
4 siblings, 1 reply; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-12 17:00 UTC (permalink / raw)
To: linux-bluetooth
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 | 2 +
net/bluetooth/hci_conn.c | 59 ++++++++++++++
net/bluetooth/hci_core.c | 163 ++++++++++++++++++++++++++++++++------
net/bluetooth/l2cap_core.c | 21 ++++-
5 files changed, 258 insertions(+), 29 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..0899f96 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;
@@ -388,6 +389,7 @@ struct l2cap_ops {
struct l2cap_conn {
struct hci_conn *hcon;
+ struct hci_chan *hchan;
bdaddr_t *dst;
bdaddr_t *src;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 581cc42..1dc8c38 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);
@@ -950,3 +954,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 94e916d..821b69b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1958,23 +1958,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);
@@ -1982,9 +1977,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;
@@ -1997,16 +1992,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)
{
@@ -2104,11 +2129,90 @@ static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
}
}
-static inline void hci_sched_acl(struct hci_dev *hdev)
+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 hci_conn *conn;
+ int cnt, q, conn_num = 0;
+
+ BT_DBG("%s", hdev->name);
+
+ list_for_each_entry(conn, &h->list, list) {
+ struct hci_chan_hash *ch;
+ struct hci_chan *tmp;
+
+ if (conn->type != type)
+ continue;
+
+ if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+ continue;
+
+ conn_num++;
+
+ ch = &conn->chan_hash;
+
+ list_for_each_entry(tmp, &ch->list, list) {
+ struct sk_buff *skb;
+
+ 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_chan *chan;
struct sk_buff *skb;
int quote;
+ unsigned int cnt;
BT_DBG("%s", hdev->name);
@@ -2122,17 +2226,23 @@ 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, "e))) {
- while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
- BT_DBG("skb %p len %d", skb, skb->len);
+ cnt = hdev->acl_cnt;
- hci_conn_enter_active_mode(conn, bt_cb(skb)->force_active);
+ while (hdev->acl_cnt &&
+ (chan = hci_chan_sent(hdev, ACL_LINK, "e))) {
+ 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--;
- conn->sent++;
+ chan->sent++;
+ chan->conn->sent++;
}
}
}
@@ -2151,7 +2261,7 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, "e))) {
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++;
@@ -2174,7 +2284,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, "e))) {
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++;
@@ -2186,7 +2296,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;
@@ -2204,17 +2314,20 @@ 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, "e))) {
- while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
- BT_DBG("skb %p len %d", skb, skb->len);
+ while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
+ 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--;
- conn->sent++;
+ 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 afd5455..28994b7 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);
@@ -558,7 +561,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
bt_cb(skb)->force_active = BT_POWER_FORCE_ACTIVE_ON;
skb->priority = HCI_PRIO_MAX;
- hci_send_acl(conn->hcon, skb, flags);
+ hci_chan_send_acl(conn->hchan, skb, flags);
}
static inline void l2cap_send_sframe(struct l2cap_chan *chan, u16 control)
@@ -984,6 +987,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
chan->ops->close(chan->data);
}
+ hci_chan_del(conn->hchan);
+
if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
del_timer_sync(&conn->info_timer);
@@ -1004,18 +1009,26 @@ static void security_timeout(unsigned long arg)
static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
{
struct l2cap_conn *conn = hcon->l2cap_data;
+ struct hci_chan *hchan;
if (conn || status)
return conn;
+ hchan = hci_chan_create(hcon);
+ if (!hchan)
+ return NULL;
+
conn = kzalloc(sizeof(struct l2cap_conn), GFP_ATOMIC);
- if (!conn)
+ if (!conn) {
+ hci_chan_del(hchan);
return NULL;
+ }
hcon->l2cap_data = conn;
conn->hcon = hcon;
+ conn->hchan = hchan;
- BT_DBG("hcon %p conn %p", hcon, conn);
+ BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
if (hcon->hdev->le_mtu && hcon->type == LE_LINK)
conn->mtu = hcon->hdev->le_mtu;
@@ -1255,7 +1268,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.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [RFC 4/5 v3] Bluetooth: prioritizing data over HCI
2011-09-12 17:00 ` [RFC 4/5 v3] Bluetooth: prioritizing data over HCI Luiz Augusto von Dentz
@ 2011-09-15 23:34 ` Mat Martineau
2011-09-16 7:35 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 20+ messages in thread
From: Mat Martineau @ 2011-09-15 23:34 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
On Mon, 12 Sep 2011, Luiz Augusto von Dentz wrote:
> 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.
For a given L2CAP channel, I think we want commands and data to use
the same queue.
With AMP channel move signaling, it's important to maintain ordering
between L2CAP commands and data. This ensures that data gets flushed
out of the queues and is not being sent during the channel move
process and AMP link establishment. You also want to make sure queued
data is sent before a disconnect request.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 42 ++++++++++
> include/net/bluetooth/l2cap.h | 2 +
> net/bluetooth/hci_conn.c | 59 ++++++++++++++
> net/bluetooth/hci_core.c | 163 ++++++++++++++++++++++++++++++++------
> net/bluetooth/l2cap_core.c | 21 ++++-
> 5 files changed, 258 insertions(+), 29 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..0899f96 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;
> @@ -388,6 +389,7 @@ struct l2cap_ops {
>
> struct l2cap_conn {
> struct hci_conn *hcon;
> + struct hci_chan *hchan;
>
> bdaddr_t *dst;
> bdaddr_t *src;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 581cc42..1dc8c38 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);
> @@ -950,3 +954,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 94e916d..821b69b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1958,23 +1958,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);
> @@ -1982,9 +1977,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;
> @@ -1997,16 +1992,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)
> {
> @@ -2104,11 +2129,90 @@ static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
> }
> }
>
> -static inline void hci_sched_acl(struct hci_dev *hdev)
> +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 hci_conn *conn;
> + int cnt, q, conn_num = 0;
> +
> + BT_DBG("%s", hdev->name);
> +
> + list_for_each_entry(conn, &h->list, list) {
> + struct hci_chan_hash *ch;
> + struct hci_chan *tmp;
> +
> + if (conn->type != type)
> + continue;
> +
> + if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
> + continue;
> +
> + conn_num++;
> +
> + ch = &conn->chan_hash;
> +
> + list_for_each_entry(tmp, &ch->list, list) {
> + struct sk_buff *skb;
> +
> + 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;
> + }
Only the priority of the first skb in each queue is considered. What
if there's a higher priority skb deeper in a queue?
Would it work to track priority in hci_chan instead of the skb? That
would simplify patch 1 of this series.
> +
> + 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_chan *chan;
> struct sk_buff *skb;
> int quote;
> + unsigned int cnt;
>
> BT_DBG("%s", hdev->name);
>
> @@ -2122,17 +2226,23 @@ 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, "e))) {
> - while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> - BT_DBG("skb %p len %d", skb, skb->len);
> + cnt = hdev->acl_cnt;
>
> - hci_conn_enter_active_mode(conn, bt_cb(skb)->force_active);
> + while (hdev->acl_cnt &&
> + (chan = hci_chan_sent(hdev, ACL_LINK, "e))) {
> + 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);
Where are conn->data_q skbs dequeued now?
>
> hci_send_frame(skb);
> hdev->acl_last_tx = jiffies;
>
> hdev->acl_cnt--;
> - conn->sent++;
> + chan->sent++;
> + chan->conn->sent++;
> }
> }
> }
> @@ -2151,7 +2261,7 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
>
> while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, "e))) {
> 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++;
> @@ -2174,7 +2284,7 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
>
> while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, "e))) {
> 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++;
> @@ -2186,7 +2296,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;
>
> @@ -2204,17 +2314,20 @@ 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, "e))) {
> - while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> - BT_DBG("skb %p len %d", skb, skb->len);
> + while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
> + 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--;
> - conn->sent++;
> + 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 afd5455..28994b7 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);
> @@ -558,7 +561,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
> bt_cb(skb)->force_active = BT_POWER_FORCE_ACTIVE_ON;
> skb->priority = HCI_PRIO_MAX;
>
> - hci_send_acl(conn->hcon, skb, flags);
> + hci_chan_send_acl(conn->hchan, skb, flags);
> }
>
> static inline void l2cap_send_sframe(struct l2cap_chan *chan, u16 control)
> @@ -984,6 +987,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> chan->ops->close(chan->data);
> }
>
> + hci_chan_del(conn->hchan);
> +
> if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> del_timer_sync(&conn->info_timer);
>
> @@ -1004,18 +1009,26 @@ static void security_timeout(unsigned long arg)
> static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> {
> struct l2cap_conn *conn = hcon->l2cap_data;
> + struct hci_chan *hchan;
>
> if (conn || status)
> return conn;
>
> + hchan = hci_chan_create(hcon);
> + if (!hchan)
> + return NULL;
> +
> conn = kzalloc(sizeof(struct l2cap_conn), GFP_ATOMIC);
> - if (!conn)
> + if (!conn) {
> + hci_chan_del(hchan);
> return NULL;
> + }
>
> hcon->l2cap_data = conn;
> conn->hcon = hcon;
> + conn->hchan = hchan;
>
> - BT_DBG("hcon %p conn %p", hcon, conn);
> + BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
>
> if (hcon->hdev->le_mtu && hcon->type == LE_LINK)
> conn->mtu = hcon->hdev->le_mtu;
> @@ -1255,7 +1268,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.1
Thanks for looking at improving the HCI scheduler. Will you be at the
Bluetooth summit in Prague? That might be a good chance to talk about
overall changes to the HCI layer.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 4/5 v3] Bluetooth: prioritizing data over HCI
2011-09-15 23:34 ` Mat Martineau
@ 2011-09-16 7:35 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-16 7:35 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth
Hi Mat,
On Fri, Sep 16, 2011 at 2:34 AM, Mat Martineau <mathewm@codeaurora.org> wrote:
>
>
> On Mon, 12 Sep 2011, Luiz Augusto von Dentz wrote:
>
>> 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.
>
> For a given L2CAP channel, I think we want commands and data to use the same
> queue.
Yep, I forgot to remove the old hci_chan from l2cap_chan, but the idea
is to use only one queue per channel to maintain ordering.
> With AMP channel move signaling, it's important to maintain ordering between
> L2CAP commands and data. This ensures that data gets flushed out of the
> queues and is not being sent during the channel move process and AMP link
> establishment. You also want to make sure queued data is sent before a
> disconnect request.
Yep.
>
>>
>> -static inline void hci_sched_acl(struct hci_dev *hdev)
>> +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 hci_conn *conn;
>> + int cnt, q, conn_num = 0;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + list_for_each_entry(conn, &h->list, list) {
>> + struct hci_chan_hash *ch;
>> + struct hci_chan *tmp;
>> +
>> + if (conn->type != type)
>> + continue;
>> +
>> + if (conn->state != BT_CONNECTED && conn->state !=
>> BT_CONFIG)
>> + continue;
>> +
>> + conn_num++;
>> +
>> + ch = &conn->chan_hash;
>> +
>> + list_for_each_entry(tmp, &ch->list, list) {
>> + struct sk_buff *skb;
>> +
>> + 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;
>> + }
>
>
> Only the priority of the first skb in each queue is considered. What if
> there's a higher priority skb deeper in a queue?
Can be done at cost of redoing the quote calculation every time
skb>priority < quote priority, but I considered that too expensive for
the little impact it cause as priority inversion, also note that it
will not handle the case you have presented that a higher priority is
deeper in the queue because it is not really a problem since we have
to maintain the ordering we cannot sent it before the low priority skb
so it has to wait until those are sent.
> Would it work to track priority in hci_chan instead of the skb? That would
> simplify patch 1 of this series.
Nope, because RFCOMM connections share the same L2CAP channel, so
currently Im afraid it has to be per skb.
>
>> +
>> + 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_chan *chan;
>> struct sk_buff *skb;
>> int quote;
>> + unsigned int cnt;
>>
>> BT_DBG("%s", hdev->name);
>>
>> @@ -2122,17 +2226,23 @@ 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,
>> "e))) {
>> - while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>> - BT_DBG("skb %p len %d", skb, skb->len);
>> + cnt = hdev->acl_cnt;
>>
>> - hci_conn_enter_active_mode(conn,
>> bt_cb(skb)->force_active);
>> + while (hdev->acl_cnt &&
>> + (chan = hci_chan_sent(hdev, ACL_LINK, "e))) {
>> + 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);
>
>
> Where are conn->data_q skbs dequeued now?
Ive removed as suggested by Gustavo to unify the algorithm used for
ACL, SCO/ESCO still use it, gonna update the commit message to reflect
it.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 5/5 v3] Bluetooth: recalculate priorities when channels are starving
2011-09-12 17:00 [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Luiz Augusto von Dentz
` (2 preceding siblings ...)
2011-09-12 17:00 ` [RFC 4/5 v3] Bluetooth: prioritizing data over HCI Luiz Augusto von Dentz
@ 2011-09-12 17:00 ` Luiz Augusto von Dentz
2011-09-19 21:36 ` [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Gustavo Padovan
4 siblings, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-12 17:00 UTC (permalink / raw)
To: linux-bluetooth
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 | 56 +++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 821b69b..0fd4fc3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2207,6 +2207,53 @@ 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 hci_conn *conn;
+ int num = 0;
+
+ BT_DBG("%s", hdev->name);
+
+ list_for_each_entry(conn, &h->list, list) {
+ struct hci_chan_hash *ch;
+ struct hci_chan *chan;
+
+ if (conn->type != type)
+ continue;
+
+ if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+ continue;
+
+ num++;
+
+ ch = &conn->chan_hash;
+ list_for_each_entry(chan, &ch->list, list) {
+ struct sk_buff *skb;
+
+ 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_chan *chan;
@@ -2245,6 +2292,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 */
@@ -2298,7 +2348,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
{
struct hci_chan *chan;
struct sk_buff *skb;
- int quote, cnt;
+ int quote, cnt, tmp;
BT_DBG("%s", hdev->name);
@@ -2314,6 +2364,7 @@ static inline void hci_sched_le(struct hci_dev *hdev)
}
cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
+ tmp = cnt;
while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
while (quote-- && (skb = skb_dequeue(&chan->data_q))) {
BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
@@ -2332,6 +2383,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.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority
2011-09-12 17:00 [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Luiz Augusto von Dentz
` (3 preceding siblings ...)
2011-09-12 17:00 ` [RFC 5/5 v3] Bluetooth: recalculate priorities when channels are starving Luiz Augusto von Dentz
@ 2011-09-19 21:36 ` Gustavo Padovan
2011-09-20 8:15 ` Luiz Augusto von Dentz
4 siblings, 1 reply; 20+ messages in thread
From: Gustavo Padovan @ 2011-09-19 21:36 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-12 20:00:49 +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 | 27 ++++++++++++++++++++-------
> net/bluetooth/l2cap_sock.c | 2 +-
> 4 files changed, 26 insertions(+), 9 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 b3bdb48..0b6e1ae 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -556,6 +556,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
> flags = ACL_START;
>
> bt_cb(skb)->force_active = BT_POWER_FORCE_ACTIVE_ON;
> + skb->priority = HCI_PRIO_MAX;
As Mat pointed out we can't prioritize commands over data, this could to
misbehaviours and loss of data. However, if the command is not related to any
cid, like the information request/reponse we can put HCI_PRIO_MAX on it.
Gustavo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority
2011-09-19 21:36 ` [RFC 1/5 v3] Bluetooth: set skbuffer priority based on L2CAP socket priority Gustavo Padovan
@ 2011-09-20 8:15 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-09-20 8:15 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth
Hi Gustavo,
On Tue, Sep 20, 2011 at 12:36 AM, Gustavo Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-12 20:00:49 +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 | 27 ++++++++++++++++++++-------
>> net/bluetooth/l2cap_sock.c | 2 +-
>> 4 files changed, 26 insertions(+), 9 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 b3bdb48..0b6e1ae 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -556,6 +556,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
>> flags = ACL_START;
>>
>> bt_cb(skb)->force_active = BT_POWER_FORCE_ACTIVE_ON;
>> + skb->priority = HCI_PRIO_MAX;
>
> As Mat pointed out we can't prioritize commands over data, this could to
> misbehaviours and loss of data. However, if the command is not related to any
> cid, like the information request/reponse we can put HCI_PRIO_MAX on it.
Yep, I got it fixed already so we don't have a separated queue for
commands anymore, this alone should already guarantee packet ordering
of the channel, but Im afraid we need to keep HCI_PRIO_MAX for
commands since those may have a timer associated to them (either
locally or remotely) so if the priority is not set according the timer
may expire before the frame is even sent, actually I already faced
this a couple of times when trying to connect to multiple devices
simultaneously a high priority socket prevented other channels
commands to be sent causing a disconnection (connect timeout even
though we succeed to page and create an ACL link).
The key problem here is that priority may increase latency and that is
not something we want to happen for commands because it may cause IOP
problems, so the use of HCI_PRIO_MAX is to minimize the latency.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets
2011-08-17 13:22 [RFC 0/5 v2] prioritizing data over HCI Luiz Augusto von Dentz
@ 2011-08-17 13:23 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 20+ messages in thread
From: Luiz Augusto von Dentz @ 2011-08-17 13:23 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 20+ messages in thread