Two rather small optimizations found while going through driver
code:
1) Use the cheaper alternative to set_current_state() as we are
sure the task will block right afterward.
2) Checks for list_empty without the scan_mutex. The list_empty
function is very much designed to work without locks, obviously
as long as the head (scan_q) is reliable. In this case if another
thread is doing add_to_scan_list(), we still buckle in the outer
loop, so it will be caught upon the next iteration -- and if
kthread_should_stop() hits, it does not matter _anyway_ as we'd
still need to abort the function regardless of the status of
the scan_q.
Compile tested only.
Signed-off-by: Davidlohr Bueso <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Mike Miller <[email protected]>
Cc: Don Brace <[email protected]>
---
drivers/block/cciss.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index ff20f19..7dd3750 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3727,18 +3727,13 @@ static int scan_thread(void *data)
struct ctlr_info *h;
while (1) {
- set_current_state(TASK_INTERRUPTIBLE);
+ __set_current_state(TASK_INTERRUPTIBLE);
schedule();
if (kthread_should_stop())
break;
- while (1) {
+ while (!list_empty(&scan_q)) {
mutex_lock(&scan_mutex);
- if (list_empty(&scan_q)) {
- mutex_unlock(&scan_mutex);
- break;
- }
-
h = list_entry(scan_q.next,
struct ctlr_info,
scan_list);
--
2.1.4
On 04/22/2015 12:43 PM, Davidlohr Bueso wrote:
> Two rather small optimizations found while going through driver
> code:
>
> 1) Use the cheaper alternative to set_current_state() as we are
> sure the task will block right afterward.
>
> 2) Checks for list_empty without the scan_mutex. The list_empty
> function is very much designed to work without locks, obviously
> as long as the head (scan_q) is reliable. In this case if another
> thread is doing add_to_scan_list(), we still buckle in the outer
> loop, so it will be caught upon the next iteration -- and if
> kthread_should_stop() hits, it does not matter _anyway_ as we'd
> still need to abort the function regardless of the status of
> the scan_q.
Not that it's wrong, but this is mostly some unneeded optimizations.
It's not like this is in a hot path.
--
Jens Axboe
On Thu, 2015-04-23 at 10:51 -0600, Jens Axboe wrote:
> Not that it's wrong, but this is mostly some unneeded optimizations.
> It's not like this is in a hot path.
Most definitely small optimizations, I just happened to run into
auditing schedule calls. Still no harm in applying.
Thanks,
Davidlohr
On 04/23/2015 11:06 AM, Davidlohr Bueso wrote:
> On Thu, 2015-04-23 at 10:51 -0600, Jens Axboe wrote:
>> Not that it's wrong, but this is mostly some unneeded optimizations.
>> It's not like this is in a hot path.
>
> Most definitely small optimizations, I just happened to run into
> auditing schedule calls. Still no harm in applying.
Are you sure? What happens if scan_thread() races with pci removal? The
list was non-empty, enter the loop. PCI removal removes the device from
the scan list. Now we grab the lock in scan_thread(), and
unconditionally attempt to remove an entry from scan_q. Maybe something
protects us from this, maybe it doesn't.
So IOW, I don't like your lock optimization, it's a bad idea. The
__set_current_state() change is definitely fine, though.
--
Jens Axboe