2011-12-20 19:10:51

by Ulisses Furquim

[permalink] [raw]
Subject: [PATCH] Bluetooth: Remove global mutex hci_task_lock

The hci_task_lock mutex (previously a lock) was supposed to protect the
register/unregister of HCI protocols against RX/TX tasks. This will not
be needed anymore because SCO and L2CAP will always be compiled.

Moreover, with the recent move of RX/TX to workqueues per device the
global hci_task_lock was causing starvation between different HCI
devices.

Signed-off-by: Ulisses Furquim <[email protected]>
---
net/bluetooth/hci_core.c | 21 +--------------------
1 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6382db..b45b745 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -61,8 +61,6 @@ static void hci_rx_work(struct work_struct *work);
static void hci_cmd_work(struct work_struct *work);
static void hci_tx_work(struct work_struct *work);

-static DEFINE_MUTEX(hci_task_lock);
-
/* HCI device list */
LIST_HEAD(hci_dev_list);
DEFINE_RWLOCK(hci_dev_list_lock);
@@ -1800,8 +1798,7 @@ EXPORT_SYMBOL(hci_recv_stream_fragment);

/* ---- Interface to upper protocols ---- */

-/* Register/Unregister protocols.
- * hci_task_lock is used to ensure that no tasks are running. */
+/* Register/Unregister protocols. */
int hci_register_proto(struct hci_proto *hp)
{
int err = 0;
@@ -1811,15 +1808,11 @@ int hci_register_proto(struct hci_proto *hp)
if (hp->id >= HCI_MAX_PROTO)
return -EINVAL;

- mutex_lock(&hci_task_lock);
-
if (!hci_proto[hp->id])
hci_proto[hp->id] = hp;
else
err = -EEXIST;

- mutex_unlock(&hci_task_lock);
-
return err;
}
EXPORT_SYMBOL(hci_register_proto);
@@ -1833,15 +1826,11 @@ int hci_unregister_proto(struct hci_proto *hp)
if (hp->id >= HCI_MAX_PROTO)
return -EINVAL;

- mutex_lock(&hci_task_lock);
-
if (hci_proto[hp->id])
hci_proto[hp->id] = NULL;
else
err = -ENOENT;

- mutex_unlock(&hci_task_lock);
-
return err;
}
EXPORT_SYMBOL(hci_unregister_proto);
@@ -2407,8 +2396,6 @@ static void hci_tx_work(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev, tx_work);
struct sk_buff *skb;

- mutex_lock(&hci_task_lock);
-
BT_DBG("%s acl %d sco %d le %d", hdev->name, hdev->acl_cnt,
hdev->sco_cnt, hdev->le_cnt);

@@ -2425,8 +2412,6 @@ static void hci_tx_work(struct work_struct *work)
/* Send next queued raw (unknown type) packet */
while ((skb = skb_dequeue(&hdev->raw_q)))
hci_send_frame(skb);
-
- mutex_unlock(&hci_task_lock);
}

/* ----- HCI RX task (incoming data processing) ----- */
@@ -2514,8 +2499,6 @@ static void hci_rx_work(struct work_struct *work)

BT_DBG("%s", hdev->name);

- mutex_lock(&hci_task_lock);
-
while ((skb = skb_dequeue(&hdev->rx_q))) {
if (atomic_read(&hdev->promisc)) {
/* Send copy to the sockets */
@@ -2559,8 +2542,6 @@ static void hci_rx_work(struct work_struct *work)
break;
}
}
-
- mutex_unlock(&hci_task_lock);
}

static void hci_cmd_work(struct work_struct *work)
--
1.7.8.rc4



2011-12-21 04:20:20

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

Hi Ulisses,

* Ulisses Furquim <[email protected]> [2011-12-20 17:10:51 -0200]:

> The hci_task_lock mutex (previously a lock) was supposed to protect the
> register/unregister of HCI protocols against RX/TX tasks. This will not
> be needed anymore because SCO and L2CAP will always be compiled.
>
> Moreover, with the recent move of RX/TX to workqueues per device the
> global hci_task_lock was causing starvation between different HCI
> devices.
>
> Signed-off-by: Ulisses Furquim <[email protected]>
> ---
> net/bluetooth/hci_core.c | 21 +--------------------
> 1 files changed, 1 insertions(+), 20 deletions(-)

Patch has been applied, thanks.

Gustavo

2011-12-20 21:02:29

by Ulisses Furquim

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

Hi Marcel,

On Tue, Dec 20, 2011 at 6:58 PM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Ulisses,
>
>> The hci_task_lock mutex (previously a lock) was supposed to protect the
>> register/unregister of HCI protocols against RX/TX tasks. This will not
>> be needed anymore because SCO and L2CAP will always be compiled.
>>
>> Moreover, with the recent move of RX/TX to workqueues per device the
>> global hci_task_lock was causing starvation between different HCI
>> devices.
>>
>> Signed-off-by: Ulisses Furquim <[email protected]>
>> ---
>> =A0net/bluetooth/hci_core.c | =A0 21 +--------------------
>> =A01 files changed, 1 insertions(+), 20 deletions(-)
>
> Acked-by: Marcel Holtmann <[email protected]>
>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index d6382db..b45b745 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -61,8 +61,6 @@ static void hci_rx_work(struct work_struct *work);
>> =A0static void hci_cmd_work(struct work_struct *work);
>> =A0static void hci_tx_work(struct work_struct *work);
>>
>> -static DEFINE_MUTEX(hci_task_lock);
>> -
>> =A0/* HCI device list */
>> =A0LIST_HEAD(hci_dev_list);
>> =A0DEFINE_RWLOCK(hci_dev_list_lock);
>> @@ -1800,8 +1798,7 @@ EXPORT_SYMBOL(hci_recv_stream_fragment);
>>
>> =A0/* ---- Interface to upper protocols ---- */
>>
>> -/* Register/Unregister protocols.
>> - * hci_task_lock is used to ensure that no tasks are running. */
>> +/* Register/Unregister protocols. */
>> =A0int hci_register_proto(struct hci_proto *hp)
>> =A0{
>> =A0 =A0 =A0 int err =3D 0;
>> @@ -1811,15 +1808,11 @@ int hci_register_proto(struct hci_proto *hp)
>> =A0 =A0 =A0 if (hp->id >=3D HCI_MAX_PROTO)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>>
>> - =A0 =A0 mutex_lock(&hci_task_lock);
>> -
>> =A0 =A0 =A0 if (!hci_proto[hp->id])
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_proto[hp->id] =3D hp;
>> =A0 =A0 =A0 else
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EEXIST;
>>
>> - =A0 =A0 mutex_unlock(&hci_task_lock);
>> -
>> =A0 =A0 =A0 return err;
>> =A0}
>
> This only works fine because we are registering the protocols from the
> module init. So what I like to see is that we get rid of this altogether
> and just call directly into the specific event functions for L2CAP and
> SCO since there are the only users anyway. Making them replaceable was a
> nice idea, but it is not practical anymore.

Exactly, I'm on it right now.

> Regards
>
> Marcel

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

2011-12-20 20:58:02

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

Hi Ulisses,

> The hci_task_lock mutex (previously a lock) was supposed to protect the
> register/unregister of HCI protocols against RX/TX tasks. This will not
> be needed anymore because SCO and L2CAP will always be compiled.
>
> Moreover, with the recent move of RX/TX to workqueues per device the
> global hci_task_lock was causing starvation between different HCI
> devices.
>
> Signed-off-by: Ulisses Furquim <[email protected]>
> ---
> net/bluetooth/hci_core.c | 21 +--------------------
> 1 files changed, 1 insertions(+), 20 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d6382db..b45b745 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -61,8 +61,6 @@ static void hci_rx_work(struct work_struct *work);
> static void hci_cmd_work(struct work_struct *work);
> static void hci_tx_work(struct work_struct *work);
>
> -static DEFINE_MUTEX(hci_task_lock);
> -
> /* HCI device list */
> LIST_HEAD(hci_dev_list);
> DEFINE_RWLOCK(hci_dev_list_lock);
> @@ -1800,8 +1798,7 @@ EXPORT_SYMBOL(hci_recv_stream_fragment);
>
> /* ---- Interface to upper protocols ---- */
>
> -/* Register/Unregister protocols.
> - * hci_task_lock is used to ensure that no tasks are running. */
> +/* Register/Unregister protocols. */
> int hci_register_proto(struct hci_proto *hp)
> {
> int err = 0;
> @@ -1811,15 +1808,11 @@ int hci_register_proto(struct hci_proto *hp)
> if (hp->id >= HCI_MAX_PROTO)
> return -EINVAL;
>
> - mutex_lock(&hci_task_lock);
> -
> if (!hci_proto[hp->id])
> hci_proto[hp->id] = hp;
> else
> err = -EEXIST;
>
> - mutex_unlock(&hci_task_lock);
> -
> return err;
> }

This only works fine because we are registering the protocols from the
module init. So what I like to see is that we get rid of this altogether
and just call directly into the specific event functions for L2CAP and
SCO since there are the only users anyway. Making them replaceable was a
nice idea, but it is not practical anymore.

Regards

Marcel