From: "Gustavo F. Padovan" <[email protected]>
No local_bh_disable is needed there once we run everything in process
context. The same goes for the replacement of bh_lock_sock() by
lock_sock().
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/af_bluetooth.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 062124c..cdcfcab 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -199,15 +199,14 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
BT_DBG("parent %p", parent);
- local_bh_disable();
list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
- bh_lock_sock(sk);
+ lock_sock(sk);
/* FIXME: Is this check still needed */
if (sk->sk_state == BT_CLOSED) {
- bh_unlock_sock(sk);
+ release_sock(sk);
bt_accept_unlink(sk);
continue;
}
@@ -218,14 +217,12 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
if (newsock)
sock_graft(sk, newsock);
- bh_unlock_sock(sk);
- local_bh_enable();
+ release_sock(sk);
return sk;
}
- bh_unlock_sock(sk);
+ release_sock(sk);
}
- local_bh_enable();
return NULL;
}
--
1.7.6.4
Hi Luiz,
> >> RFCOMM needs a proper priority mechanism inside itself and not try to use
> >> l2cap priority to fix its own problem.
> >
> > <snip>
> >
> >> static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
> >> @@ -1786,8 +1776,15 @@ 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,
> >> - skb->priority);
> >> + struct socket *sock = d->session->sock;
> >> + struct sock *sk = sock->sk;
> >> +
> >> + if (sk->sk_priority != skb->priority) {
> >> + lock_sock(sk);
> >> + sk->sk_priority = skb->priority;
> >> + release_sock(sk);
> >> + }
> >> + err = rfcomm_send_frame(d->session, skb->data, skb->len);
> >> if (err < 0) {
> >> skb_queue_head(&d->tx_queue, skb);
> >> break;
> >
> > coming to think about this, we might not even bother setting the
> > sk->sk_priority at all here. Lets just forget about RFCOMM.
>
> I guess you guys don't remember, but I tried to explain why the
> priority has to be set even for RFCOMM, any frame sent by the kernel
> with a timeout needs to be scheduled as soon as possible otherwise it
> may timeout due to quote being consumed by other channels with higher
> priority, so in this case if an A2DP stream in ongoing you will never
> be able to complete a RFCOMM connection because there wont be enough
> quote to schedule them before they timeout (usually 10 out 10 attempt
> end up like that in my setup).
>
> Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that
> should get rid of this resetting sk_priority on every frame, but that
> means not depending on socket API at all to pass frames from RFCOMM to
> L2CAP, so we need an API to create L2CAP frames directly in RFCOMM
> modules and queue them in its hci_chan.
yes, we need exactly this. Not using the L2CAP socket and interfacing
with L2CAP directly is a good idea. Same came up with A2MP as well.
Regards
Marcel
Hi Gustavo, Marcel,
On Tue, Dec 20, 2011 at 11:01 PM, Marcel Holtmann <[email protected]> wro=
te:
> Hi Gustavo,
>
>> RFCOMM needs a proper priority mechanism inside itself and not try to us=
e
>> l2cap priority to fix its own problem.
>
> <snip>
>
>> =A0static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
>> @@ -1786,8 +1776,15 @@ static inline int rfcomm_process_tx(struct rfcomm=
_dlc *d)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return skb_queue_len(&d->tx_queue);
>>
>> =A0 =A0 =A0 while (d->tx_credits && (skb =3D skb_dequeue(&d->tx_queue)))=
{
>> - =A0 =A0 =A0 =A0 =A0 =A0 err =3D rfcomm_send_frame(d->session, skb->dat=
a, skb->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 =A0 skb->priority);
>> + =A0 =A0 =A0 =A0 =A0 =A0 struct socket *sock =3D d->session->sock;
>> + =A0 =A0 =A0 =A0 =A0 =A0 struct sock *sk =3D sock->sk;
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (sk->sk_priority !=3D skb->priority) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 lock_sock(sk);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D skb->prior=
ity;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_sock(sk);
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D rfcomm_send_frame(d->session, skb->dat=
a, skb->len);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err < 0) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_queue_head(&d->tx_queue,=
skb);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>
> coming to think about this, we might not even bother setting the
> sk->sk_priority at all here. Lets just forget about RFCOMM.
I guess you guys don't remember, but I tried to explain why the
priority has to be set even for RFCOMM, any frame sent by the kernel
with a timeout needs to be scheduled as soon as possible otherwise it
may timeout due to quote being consumed by other channels with higher
priority, so in this case if an A2DP stream in ongoing you will never
be able to complete a RFCOMM connection because there wont be enough
quote to schedule them before they timeout (usually 10 out 10 attempt
end up like that in my setup).
Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that
should get rid of this resetting sk_priority on every frame, but that
means not depending on socket API at all to pass frames from RFCOMM to
L2CAP, so we need an API to create L2CAP frames directly in RFCOMM
modules and queue them in its hci_chan.
--=20
Luiz Augusto von Dentz
Hi Gustavo,
> RFCOMM needs a proper priority mechanism inside itself and not try to use
> l2cap priority to fix its own problem.
<snip>
> static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
> @@ -1786,8 +1776,15 @@ 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,
> - skb->priority);
> + struct socket *sock = d->session->sock;
> + struct sock *sk = sock->sk;
> +
> + if (sk->sk_priority != skb->priority) {
> + lock_sock(sk);
> + sk->sk_priority = skb->priority;
> + release_sock(sk);
> + }
> + err = rfcomm_send_frame(d->session, skb->data, skb->len);
> if (err < 0) {
> skb_queue_head(&d->tx_queue, skb);
> break;
coming to think about this, we might not even bother setting the
sk->sk_priority at all here. Lets just forget about RFCOMM.
Regards
Marcel
Hi Gustavo,
> No local_bh_disable is needed there once we run everything in process
> context. The same goes for the replacement of bh_lock_sock() by
> lock_sock().
>
> Signed-off-by: Gustavo F. Padovan <[email protected]>
> ---
> net/bluetooth/af_bluetooth.c | 11 ++++-------
> 1 files changed, 4 insertions(+), 7 deletions(-)
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
From: "Gustavo F. Padovan" <[email protected]>
RFCOMM needs a proper priority mechanism inside itself and not try to use
l2cap priority to fix its own problem.
Acked-by: Marcel Holtmann <[email protected]>
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/rfcomm/core.c | 47 ++++++++++++++++++++----------------------
1 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index be6288c..a795d96 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -65,8 +65,7 @@ static DEFINE_MUTEX(rfcomm_mutex);
static LIST_HEAD(session_list);
-static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
- u32 priority);
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
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);
@@ -748,32 +747,23 @@ 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,
- u32 priority)
+static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
{
- struct socket *sock = s->sock;
- struct sock *sk = sock->sk;
struct kvec iv = { data, len };
struct msghdr msg;
- 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);
- }
+ BT_DBG("session %p len %d", s, len);
memset(&msg, 0, sizeof(msg));
- return kernel_sendmsg(sock, &msg, &iv, 1, len);
+ return kernel_sendmsg(s->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);
+ return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd));
}
static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci)
@@ -878,7 +868,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d)
@@ -920,7 +910,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci,
@@ -958,7 +948,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status)
@@ -985,7 +975,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig)
@@ -1012,7 +1002,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
@@ -1034,7 +1024,7 @@ static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr)
*ptr = __fcs(buf); ptr++;
- return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
@@ -1056,7 +1046,7 @@ static int rfcomm_send_fcon(struct rfcomm_session *s, int cr)
*ptr = __fcs(buf); ptr++;
- return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static int rfcomm_send_test(struct rfcomm_session *s, int cr, u8 *pattern, int len)
@@ -1107,7 +1097,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, HCI_PRIO_MAX);
+ return rfcomm_send_frame(s, buf, ptr - buf);
}
static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
@@ -1786,8 +1776,15 @@ 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,
- skb->priority);
+ struct socket *sock = d->session->sock;
+ struct sock *sk = sock->sk;
+
+ if (sk->sk_priority != skb->priority) {
+ lock_sock(sk);
+ sk->sk_priority = skb->priority;
+ release_sock(sk);
+ }
+ err = rfcomm_send_frame(d->session, skb->data, skb->len);
if (err < 0) {
skb_queue_head(&d->tx_queue, skb);
break;
--
1.7.6.4
Hi Luiz,
> >> I guess you guys don't remember, but I tried to explain why the
> >> priority has to be set even for RFCOMM, any frame sent by the kernel
> >> with a timeout needs to be scheduled as soon as possible otherwise it
> >> may timeout due to quote being consumed by other channels with higher
> >> priority, so in this case if an A2DP stream in ongoing you will never
> >> be able to complete a RFCOMM connection because there wont be enough
> >> quote to schedule them before they timeout (usually 10 out 10 attempt
> >> end up like that in my setup).
> >>
> >> Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that
> >> should get rid of this resetting sk_priority on every frame, but that
> >> means not depending on socket API at all to pass frames from RFCOMM to
> >> L2CAP, so we need an API to create L2CAP frames directly in RFCOMM
> >> modules and queue them in its hci_chan.
> >
> > yes, we need exactly this. Not using the L2CAP socket and interfacing
> > with L2CAP directly is a good idea. Same came up with A2MP as well.
>
> Right, so I will work on this next, but in the meantime I think we
> should keep the current logic (it doesn't seems to cause any problem
> although the locking is pretty bad) until we got a proper solution and
> at least it is able to complete connections.
the locking is broken with the move to workqueues. So feel free to
figure out a locking that works, but right now it is screwed up.
Be my guest to set a MAX_PRIO - 1 by default for RFCOMM and see how that
works out.
Regards
Marcel
Hi Marcel,
On Thu, Dec 29, 2011 at 7:45 PM, Marcel Holtmann <[email protected]> wrote:
>>
>> I guess you guys don't remember, but I tried to explain why the
>> priority has to be set even for RFCOMM, any frame sent by the kernel
>> with a timeout needs to be scheduled as soon as possible otherwise it
>> may timeout due to quote being consumed by other channels with higher
>> priority, so in this case if an A2DP stream in ongoing you will never
>> be able to complete a RFCOMM connection because there wont be enough
>> quote to schedule them before they timeout (usually 10 out 10 attempt
>> end up like that in my setup).
>>
>> Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that
>> should get rid of this resetting sk_priority on every frame, but that
>> means not depending on socket API at all to pass frames from RFCOMM to
>> L2CAP, so we need an API to create L2CAP frames directly in RFCOMM
>> modules and queue them in its hci_chan.
>
> yes, we need exactly this. Not using the L2CAP socket and interfacing
> with L2CAP directly is a good idea. Same came up with A2MP as well.
Right, so I will work on this next, but in the meantime I think we
should keep the current logic (it doesn't seems to cause any problem
although the locking is pretty bad) until we got a proper solution and
at least it is able to complete connections.
--
Luiz Augusto von Dentz