[PATCH] Bluetooth: Remove global mutex hci_task_lock

[PATCH] Bluetooth: Remove global mutex hci_task_lock

[Ulisses Furquim]

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 <ulisses@profusion.mobi>
---
 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

Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

[Marcel Holtmann]

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 <ulisses@profusion.mobi>
> ---
>  net/bluetooth/hci_core.c |   21 +--------------------
>  1 files changed, 1 insertions(+), 20 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

> 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

Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

[Ulisses Furquim]

Hi Marcel,

On Tue, Dec 20, 2011 at 6:58 PM, Marcel Holtmann <marcel@holtmann.org> 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 <ulisses@profusion.mobi>
>> ---
>> =A0net/bluetooth/hci_core.c | =A0 21 +--------------------
>> =A01 files changed, 1 insertions(+), 20 deletions(-)
>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
>> 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

Re: [PATCH] Bluetooth: Remove global mutex hci_task_lock

[Gustavo Padovan]

Hi Ulisses,

* Ulisses Furquim <ulisses@profusion.mobi> [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 <ulisses@profusion.mobi>
> ---
>  net/bluetooth/hci_core.c |   21 +--------------------
>  1 files changed, 1 insertions(+), 20 deletions(-)

Patch has been applied, thanks.

	Gustavo