Re: [PATCH 5/7] Bluetooth: LE scan infra-structure

[Mat Martineau <mathewm@codeaurora.org>]

On Thu, 15 Dec 2011, Andre Guedes wrote:

> Hi Mat,
>
> On Dec 15, 2011, at 4:25 PM, Mat Martineau wrote:
>
>>
>> Marcel & Andre -
>>
>> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>>
>>> Hi Andre,
>>>
>>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>>
>>>> The LE scan is implemented using a work_struct which is enqueued
>>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>>> its results. If commands were executed successfully a timer is set
>>>> to disable the ongoing scanning after some amount of time.
>>>
>>> so I rather hold off on these until we get the tasklet removal patches
>>> merged. The mgmt processing will then also be done in process context
>>> and we can just sleep. This should make code like this a lot simpler.
>>
>>
>> While execution on a workqueue can sleep, it's not a good idea to 
>> block for a long time like this patch does.  A single-threaded 
>> workqueue (like the hdev workqueue) will not run the next scheduled 
>> work until the current work function returns.  If code executing in 
>> a workqueue suspends execution by using a wait queue, like 
>> le_scan_workqueue(), then all other pending work is blocked until 
>> le_scan_workqueue() returns.
>
> Yes, I'm aware of it. This is why we have the schedule_timeout(). Most
> of the time, we sleep for a short time since the controller sends the
> command result event just after we send the command. If the controller
> takes too long or simply doesn't send the command event we timeout
> and abort the le scan work. This way we don't starve other works
> enqueued on hdev->workqueue.

I did some work to verify what I'm asserting about workqueue 
serialization in a single-threaded workqueue, and I still think I'm on 
the right track for our current use of workqueues.

My initial understanding was based on what I had read in Linux Device 
Drivers (3rd ed), which dates back to 2.6.10.  Workqueues have since 
been updated to use a thread pool (http://lwn.net/Articles/355700/), 
but our hdev workqueue is still serialized.  From that LWN article: 
"Code which requires that workqueue tasks be executed in the order in 
which they are submitted can use a WQ_UNBOUND workqueue with 
max_active set to one."  Look at process_one_work() in workqueue.c, 
the submitted work is run in the middle ("f(work);"), and the next 
work is not started until cwq_dec_nr_in_flight() is called at the end 
-- after f(work) returns.

So, using schedule_timeout() doesn't help, unless we switch away 
from the single threaded workqueue.  I'll reply to David's email on 
that point.

> Do you mean "block for a long time" the time we wait for the command
> result or the timeout?

Both.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum