* [PATCH] cciss: Optimize scan_thread
@ 2015-04-22 18:43 Davidlohr Bueso
2015-04-23 16:51 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Davidlohr Bueso @ 2015-04-22 18:43 UTC (permalink / raw)
To: Jens Axboe
Cc: Mike Miller, Don Brace, iss_storagedev, storagedev, linux-kernel, dave
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 <dbueso@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Mike Miller <mike.miller@hp.com>
Cc: Don Brace <don.brace@pmcs.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] cciss: Optimize scan_thread
2015-04-22 18:43 [PATCH] cciss: Optimize scan_thread Davidlohr Bueso
@ 2015-04-23 16:51 ` Jens Axboe
2015-04-23 17:06 ` Davidlohr Bueso
0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2015-04-23 16:51 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Mike Miller, Don Brace, iss_storagedev, storagedev, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cciss: Optimize scan_thread
2015-04-23 16:51 ` Jens Axboe
@ 2015-04-23 17:06 ` Davidlohr Bueso
2015-04-23 17:11 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Davidlohr Bueso @ 2015-04-23 17:06 UTC (permalink / raw)
To: Jens Axboe
Cc: Mike Miller, Don Brace, iss_storagedev, storagedev, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cciss: Optimize scan_thread
2015-04-23 17:06 ` Davidlohr Bueso
@ 2015-04-23 17:11 ` Jens Axboe
0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2015-04-23 17:11 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Mike Miller, Don Brace, iss_storagedev, storagedev, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-04-23 17:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-22 18:43 [PATCH] cciss: Optimize scan_thread Davidlohr Bueso
2015-04-23 16:51 ` Jens Axboe
2015-04-23 17:06 ` Davidlohr Bueso
2015-04-23 17:11 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).