[PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[Marcel Holtmann]

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e17849e..005f9ce 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -109,12 +109,14 @@ struct hci_dev {
 
 	struct workqueue_struct	*workqueue;
 
+	struct work_struct	evt_work;
 	struct tasklet_struct	cmd_task;
 	struct tasklet_struct	rx_task;
 	struct tasklet_struct	tx_task;
 
 	struct sk_buff_head	rx_q;
 	struct sk_buff_head	raw_q;
+	struct sk_buff_head	evt_q;
 	struct sk_buff_head	cmd_q;
 
 	struct sk_buff		*sent_cmd;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8b4baac..7e3fc60 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -50,6 +50,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+static void hci_evt_work(struct work_struct *work);
 static void hci_cmd_task(unsigned long arg);
 static void hci_rx_task(unsigned long arg);
 static void hci_tx_task(unsigned long arg);
@@ -521,7 +522,9 @@ int hci_dev_open(__u16 dev)
 		tasklet_kill(&hdev->rx_task);
 		tasklet_kill(&hdev->tx_task);
 		tasklet_kill(&hdev->cmd_task);
+		cancel_work_sync(&hdev->evt_work);
 
+		skb_queue_purge(&hdev->evt_q);
 		skb_queue_purge(&hdev->cmd_q);
 		skb_queue_purge(&hdev->rx_q);
 
@@ -570,6 +573,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 		hdev->flush(hdev);
 
 	/* Reset device */
+	skb_queue_purge(&hdev->evt_q);
 	skb_queue_purge(&hdev->cmd_q);
 	atomic_set(&hdev->cmd_cnt, 1);
 	if (!test_bit(HCI_RAW, &hdev->flags)) {
@@ -579,11 +583,13 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 		clear_bit(HCI_INIT, &hdev->flags);
 	}
 
-	/* Kill cmd task */
+	/* Kill cmd task and evt work */
 	tasklet_kill(&hdev->cmd_task);
+	cancel_work_sync(&hdev->evt_work);
 
 	/* Drop queues */
 	skb_queue_purge(&hdev->rx_q);
+	skb_queue_purge(&hdev->evt_q);
 	skb_queue_purge(&hdev->cmd_q);
 	skb_queue_purge(&hdev->raw_q);
 
@@ -634,6 +640,7 @@ int hci_dev_reset(__u16 dev)
 
 	/* Drop queues */
 	skb_queue_purge(&hdev->rx_q);
+	skb_queue_purge(&hdev->evt_q);
 	skb_queue_purge(&hdev->cmd_q);
 
 	hci_dev_lock_bh(hdev);
@@ -905,11 +912,14 @@ int hci_register_dev(struct hci_dev *hdev)
 	hdev->sniff_max_interval = 800;
 	hdev->sniff_min_interval = 80;
 
+	INIT_WORK(&hdev->evt_work, hci_evt_work);
+
 	tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
 	tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
 	tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
 
 	skb_queue_head_init(&hdev->rx_q);
+	skb_queue_head_init(&hdev->evt_q);
 	skb_queue_head_init(&hdev->cmd_q);
 	skb_queue_head_init(&hdev->raw_q);
 
@@ -1022,9 +1032,19 @@ int hci_recv_frame(struct sk_buff *skb)
 	/* Time stamp */
 	__net_timestamp(skb);
 
-	/* Queue frame for rx task */
-	skb_queue_tail(&hdev->rx_q, skb);
-	tasklet_schedule(&hdev->rx_task);
+	switch (bt_cb(skb)->pkt_type) {
+	case HCI_EVENT_PKT:
+		/* Queue frame for event processing */
+		skb_queue_tail(&hdev->evt_q, skb);
+		queue_work(hdev->workqueue, &hdev->evt_work);
+		break;
+
+	default:
+		/* Queue frame for rx task */
+		skb_queue_tail(&hdev->rx_q, skb);
+		tasklet_schedule(&hdev->rx_task);
+		break;
+	}
 
 	return 0;
 }
@@ -1614,10 +1634,6 @@ static void hci_rx_task(unsigned long arg)
 
 		/* Process frame */
 		switch (bt_cb(skb)->pkt_type) {
-		case HCI_EVENT_PKT:
-			hci_event_packet(hdev, skb);
-			break;
-
 		case HCI_ACLDATA_PKT:
 			BT_DBG("%s ACL data packet", hdev->name);
 			hci_acldata_packet(hdev, skb);
@@ -1663,3 +1679,29 @@ static void hci_cmd_task(unsigned long arg)
 		}
 	}
 }
+
+static void hci_evt_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, evt_work);
+	struct sk_buff *skb;
+
+	BT_DBG("%s", hdev->name);
+
+	read_lock(&hci_task_lock);
+
+	while ((skb = skb_dequeue(&hdev->evt_q))) {
+		if (atomic_read(&hdev->promisc)) {
+			/* Send copy to the sockets */
+			hci_send_to_sock(hdev, skb);
+		}
+
+		if (test_bit(HCI_RAW, &hdev->flags)) {
+			kfree_skb(skb);
+			continue;
+		}
+
+		hci_event_packet(hdev, skb);
+	}
+
+	read_unlock(&hci_task_lock);
+}
-- 
1.6.6.1

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[Marcel Holtmann]

Hi everybody,

> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 52 insertions(+), 8 deletions(-)

so I stuffed this now into bluetooth-testing tree and would like to see
some extra testing exposure. So far this has only been tested by myself.

If there are no regression then this should make a lot of HCI and L2CAP
handling a lot simpler.

Regards

Marcel

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[David Vrabel]

Marcel Holtmann wrote:
> Hi everybody,
> 
>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>> ---
>>  include/net/bluetooth/hci_core.h |    2 +
>>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
>>  2 files changed, 52 insertions(+), 8 deletions(-)
> 
> so I stuffed this now into bluetooth-testing tree and would like to see
> some extra testing exposure. So far this has only been tested by myself.
> 
> If there are no regression then this should make a lot of HCI and L2CAP
> handling a lot simple.

This may result in packets being processed in a different order to that 
which they were received in.

e.g., what happens to an ACL packet processed before the connection 
complete event for that connection?

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[Marcel Holtmann]

Hi David,

> >> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> >> ---
> >>  include/net/bluetooth/hci_core.h |    2 +
> >>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
> >>  2 files changed, 52 insertions(+), 8 deletions(-)
> > 
> > so I stuffed this now into bluetooth-testing tree and would like to see
> > some extra testing exposure. So far this has only been tested by myself.
> > 
> > If there are no regression then this should make a lot of HCI and L2CAP
> > handling a lot simple.
> 
> This may result in packets being processed in a different order to that 
> which they were received in.
> 
> e.g., what happens to an ACL packet processed before the connection 
> complete event for that connection?

good point. So we would either a) need to disable the RX tasklet when we
receive an event and schedule it for processing or b) process the ACL
data also in a workqueue.

Regards

Marcel

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[David Vrabel]

Marcel Holtmann wrote:
> Hi David,
> 
>>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>>> ---
>>>>  include/net/bluetooth/hci_core.h |    2 +
>>>>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
>>>>  2 files changed, 52 insertions(+), 8 deletions(-)
>>> so I stuffed this now into bluetooth-testing tree and would like to see
>>> some extra testing exposure. So far this has only been tested by myself.
>>>
>>> If there are no regression then this should make a lot of HCI and L2CAP
>>> handling a lot simple.
>> This may result in packets being processed in a different order to that 
>> which they were received in.
>>
>> e.g., what happens to an ACL packet processed before the connection 
>> complete event for that connection?
> 
> good point. So we would either a) need to disable the RX tasklet when we
> receive an event and schedule it for processing or b) process the ACL
> data also in a workqueue.

I think my preferred solution would be for the rx function called by the 
HCI transport drivers to be called in a thread context.  There should be 
a helper function that is callable in atomic context that queues the 
packets to be processed in a workqueue.

Our UWB drivers already process packets in a thread so would prefer to 
avoid another thread context switch.

Shall I prepare a patch?

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[Marcel Holtmann]

Hi David,

> >>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> >>>> ---
> >>>>  include/net/bluetooth/hci_core.h |    2 +
> >>>>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
> >>>>  2 files changed, 52 insertions(+), 8 deletions(-)
> >>> so I stuffed this now into bluetooth-testing tree and would like to see
> >>> some extra testing exposure. So far this has only been tested by myself.
> >>>
> >>> If there are no regression then this should make a lot of HCI and L2CAP
> >>> handling a lot simple.
> >> This may result in packets being processed in a different order to that 
> >> which they were received in.
> >>
> >> e.g., what happens to an ACL packet processed before the connection 
> >> complete event for that connection?
> > 
> > good point. So we would either a) need to disable the RX tasklet when we
> > receive an event and schedule it for processing or b) process the ACL
> > data also in a workqueue.
> 
> I think my preferred solution would be for the rx function called by the 
> HCI transport drivers to be called in a thread context.  There should be 
> a helper function that is callable in atomic context that queues the 
> packets to be processed in a workqueue.
> 
> Our UWB drivers already process packets in a thread so would prefer to 
> avoid another thread context switch.
> 
> Shall I prepare a patch?

yes please. I need something to make the event handling simpler.
Otherwise every single subcomponent creates "hundreds" of workqueues to
solve exactly the same problem.

Regards

Marcel

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[David Vrabel]

Marcel Holtmann wrote:
> Hi David,
> 
>>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>>> ---
>>>>  include/net/bluetooth/hci_core.h |    2 +
>>>>  net/bluetooth/hci_core.c         |   58 ++++++++++++++++++++++++++++++++-----
>>>>  2 files changed, 52 insertions(+), 8 deletions(-)
>>> so I stuffed this now into bluetooth-testing tree and would like to see
>>> some extra testing exposure. So far this has only been tested by myself.
>>>
>>> If there are no regression then this should make a lot of HCI and L2CAP
>>> handling a lot simple.
>> This may result in packets being processed in a different order to that 
>> which they were received in.
>>
>> e.g., what happens to an ACL packet processed before the connection 
>> complete event for that connection?
> 
> good point. So we would either a) need to disable the RX tasklet when we
> receive an event and schedule it for processing or b) process the ACL
> data also in a workqueue.

I've thought some more about this and I'm not sure disabling the tasklet
is sufficient to prevent packets being reordered.  Consider a transport
that submits (in the same interrupt handler call) an ACL packet and an
HCI event.  The tasklet will be scheduled and then disabled until the
event is processed in the workqueue.

On the other hand, USB transports do not ensure any ordering between HCI
event and ACL packets because they're received on different USB
endpoints which could be processed in any order.

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet

[Marcel Holtmann]

Hi David,

> >>> so I stuffed this now into bluetooth-testing tree and would like to see
> >>> some extra testing exposure. So far this has only been tested by myself.
> >>>
> >>> If there are no regression then this should make a lot of HCI and L2CAP
> >>> handling a lot simple.
> >> This may result in packets being processed in a different order to that 
> >> which they were received in.
> >>
> >> e.g., what happens to an ACL packet processed before the connection 
> >> complete event for that connection?
> > 
> > good point. So we would either a) need to disable the RX tasklet when we
> > receive an event and schedule it for processing or b) process the ACL
> > data also in a workqueue.
> 
> I've thought some more about this and I'm not sure disabling the tasklet
> is sufficient to prevent packets being reordered.  Consider a transport
> that submits (in the same interrupt handler call) an ACL packet and an
> HCI event.  The tasklet will be scheduled and then disabled until the
> event is processed in the workqueue.
> 
> On the other hand, USB transports do not ensure any ordering between HCI
> event and ACL packets because they're received on different USB
> endpoints which could be processed in any order.

so lets see here. We expect that hci_recv_frame receives the events and
data packets in proper order. If it happens that the USB controller does
a different order, then it is the USB controllers problem. We just can't
fix that in the Bluetooth core since we don't know the expected order.
So we must assume whatever gets received via hci_recv_frame is the
correct order.

        switch (bt_cb(skb)->pkt_type) {
        case HCI_EVENT_PKT:
                /* Queue frame for event processing */
                skb_queue_tail(&hdev->evt_q, skb);
                queue_work(hdev->workqueue, &hdev->evt_work);
                break;

        default:
                /* Queue frame for rx task */
                skb_queue_tail(&hdev->rx_q, skb);
                tasklet_schedule(&hdev->rx_task);
                break;
        }

If we now disable the rx_task before scheduling the evt_work, then the
rx_task gets scheduled, but only run after we enable rx_task again. And
we enable the rx_task after hci_event_packet. So after we have created
any structure to handle the ACL and SCO data packets correctly.

I am not seeing your concern. At least not in the Bluetooth core. There
might be a few driver issues when the controller doesn't send us things
in order, but that is really a controller or driver issue at that point.

Coming to think about this, we might even go one step further here and
only enable the rx_task when we have an ACL link. That way if the
controller sends us an ACL packet before the Connection Complete event
we just queue it up. That would potentially solve issues that we have
seen with Broadcom controllers where the first ACL packet arrives too
early and BlueZ is just too fast.

We could even go one step further with this and only enable the RX task
when we have switched on encryption (for 2.1 with SSP). This might fix
some issues with the stupid controllers sending ACL packets before
notifying us that the link has switched on encryption.

Anyway, I am really open for suggestions here. I just think that moving
all RX and TX processing into a workqueue might be a bit overkill. And
keep processing events in a tasklet will create code complexity in the
future when adding AMP support.

Regards

Marcel