block: optimizations in blk_rq_timed_out_timer()

Now the rq->deadline can't be zero if the request is in the timeout_list, so there is no need to have next_set. There is no need to access a request's deadline field if blk_rq_timed_out is called on it.

Signed-off-by Malahal Naineni <[email protected]>

diff -r a6ae42397ede block/blk-timeout.c

block/blk-timeout.c +1 -1

@@ -118,7 +118,7 @@
 void blk_rq_timed_out_timer(unsigned long data)
 {
 	struct request_queue *q = (struct request_queue *) data;
-	unsigned long flags, uninitialized_var(next), next_set = 0;
+	unsigned long flags, next = 0;
 	struct request *rq, *tmp;
 
 	spin_lock_irqsave(q->queue_lock, flags);

block/blk-timeout.c +1 -7

 		}
-		if (!next_set) {
-			next = rq->deadline;
-			next_set = 1;
-		} else if (time_after(next, rq->deadline))
-			next = rq->deadline;
 	}
 
-	if (next_set && !list_empty(&q->timeout_list))
+	if (next)
 		mod_timer(&q->timeout, round_jiffies(next));
 
 	spin_unlock_irqrestore(q->queue_lock, flags);
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · On Tue, 28 Oct 2008 11:22:43 -0700 [email protected] wrote:

Now the rq->deadline can't be zero if the request is in the timeout_list, so there is no need to have next_set. There is no need to access a request's deadline field if blk_rq_timed_out is called on it.

Signed-off-by Malahal Naineni <[email protected]>

diff -r a6ae42397ede block/blk-timeout.c

block/blk-timeout.c +1 -1

@@ -118,7 +118,7 @@
 void blk_rq_timed_out_timer(unsigned long data)
 {
 	struct request_queue *q = (struct request_queue *) data;
-	unsigned long flags, uninitialized_var(next), next_set = 0;
+	unsigned long flags, next = 0;
 	struct request *rq, *tmp;
 

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 	spin_lock_irqsave(q->queue_lock, flags);

@@ -133,15 +133,13 @@
 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		} else {
+			if (!next || time_after(next, rq->deadline))
+				next = rq->deadline;

Hmm, blk_rq_timed_out(rq) could put the rq back to q->timeout_list. We need to take care of the rq->deadline like this?

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		}
 
+		if (!next || time_after(next, rq->deadline))
+			next = rq->deadline;

But if blk_rq_timed_out calls __blk_complete_request, we don't want to touch the rq here, I guess.

Quoted 20 lines · FUJITA Tomonori [[email protected]] wrote:

Quoted 17 lines · On Tue, 28 Oct 2008 11:22:43 -0700 [email protected] wrote:

Now the rq->deadline can't be zero if the request is in the timeout_list, so there is no need to have next_set. There is no need to access a request's deadline field if blk_rq_timed_out is called on it.

Signed-off-by Malahal Naineni <[email protected]>

diff -r a6ae42397ede block/blk-timeout.c

block/blk-timeout.c +1 -1

@@ -118,7 +118,7 @@
 void blk_rq_timed_out_timer(unsigned long data)
 {
 	struct request_queue *q = (struct request_queue *) data;
-	unsigned long flags, uninitialized_var(next), next_set = 0;
+	unsigned long flags, next = 0;
 	struct request *rq, *tmp;
 

Quoted 11 lines

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 	spin_lock_irqsave(q->queue_lock, flags);

@@ -133,15 +133,13 @@
 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		} else {
+			if (!next || time_after(next, rq->deadline))
+				next = rq->deadline;

Hmm, blk_rq_timed_out(rq) could put the rq back to q->timeout_list. We need to take care of the rq->deadline like this?

If it is put back on the list, it would be placed at the end of the list so we will get to work on it again in the list traversal. There is no need to access it now.

Quoted 11 lines

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		}
 
+		if (!next || time_after(next, rq->deadline))
+			next = rq->deadline;

But if blk_rq_timed_out calls __blk_complete_request, we don't want to touch the rq here, I guess.

Yes, we should not access it although I don't think the request can be freed while the existing code is accessing the deadline field. What likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Quoted 4 lines

Yes, we should not access it although I don't think the request can be freed while the existing code is accessing the deadline field. What likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

That would not call the timer immediately, that will not call it until jiffies reaches that value again. So you're looking at a minimum of 49 days :-)

Quoted 20 lines · FUJITA Tomonori [[email protected]] wrote:

Quoted 17 lines · On Tue, 28 Oct 2008 11:22:43 -0700 [email protected] wrote:

Now the rq->deadline can't be zero if the request is in the timeout_list, so there is no need to have next_set. There is no need to access a request's deadline field if blk_rq_timed_out is called on it.

Signed-off-by Malahal Naineni <[email protected]>

diff -r a6ae42397ede block/blk-timeout.c

block/blk-timeout.c +1 -1

@@ -118,7 +118,7 @@
 void blk_rq_timed_out_timer(unsigned long data)
 {
 	struct request_queue *q = (struct request_queue *) data;
-	unsigned long flags, uninitialized_var(next), next_set = 0;
+	unsigned long flags, next = 0;
 	struct request *rq, *tmp;
 

Quoted 11 lines

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 	spin_lock_irqsave(q->queue_lock, flags);

@@ -133,15 +133,13 @@
 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		} else {
+			if (!next || time_after(next, rq->deadline))
+				next = rq->deadline;

Hmm, blk_rq_timed_out(rq) could put the rq back to q->timeout_list. We need to take care of the rq->deadline like this?

If it is put back on the list, it would be placed at the end of the list so we will get to work on it again in the list traversal. There is no need to access it now.

Quoted 5 lines

Quoted 5 lines

block/blk-timeout.c +1 -0

 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		}
 

Quoted 5 lines

block/blk-timeout.c line 138

Quoted 2 lines

block/blk-timeout.c +2 -0

+		if (!next || time_after(next, rq->deadline))
+			next = rq->deadline;

But if blk_rq_timed_out calls __blk_complete_request, we don't want to touch the rq here, I guess.

Yes, we should not access it although I don't think the request can be freed while the existing code is accessing the deadline field.

What
likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Quoted 33 lines · On Tue, 28 Oct 2008 23:06:25 -0700 [email protected] wrote:

Quoted 20 lines · FUJITA Tomonori [[email protected]] wrote:

Quoted 17 lines · On Tue, 28 Oct 2008 11:22:43 -0700 [email protected] wrote:

Now the rq->deadline can't be zero if the request is in the timeout_list, so there is no need to have next_set. There is no need to access a request's deadline field if blk_rq_timed_out is called on it.

Signed-off-by Malahal Naineni <[email protected]>

diff -r a6ae42397ede block/blk-timeout.c

block/blk-timeout.c +1 -1

@@ -118,7 +118,7 @@
 void blk_rq_timed_out_timer(unsigned long data)
 {
 	struct request_queue *q = (struct request_queue *) data;
-	unsigned long flags, uninitialized_var(next), next_set = 0;
+	unsigned long flags, next = 0;
 	struct request *rq, *tmp;
 

Quoted 11 lines

block/blk-timeout.c line 138

Quoted 8 lines

block/blk-timeout.c +3 -0

 	spin_lock_irqsave(q->queue_lock, flags);

@@ -133,15 +133,13 @@
 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		} else {
+			if (!next || time_after(next, rq->deadline))
+				next = rq->deadline;

Hmm, blk_rq_timed_out(rq) could put the rq back to q->timeout_list. We need to take care of the rq->deadline like this?

Quoted 3 lines

If it is put back on the list, it would be placed at the end of the list so we will get to work on it again in the list traversal. There is no need to access it now.

Ah, I see.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

block/blk-timeout.c +1 -0

 			if (blk_mark_rq_complete(rq))
 				continue;
 			blk_rq_timed_out(rq);
+		}
 

Quoted 8 lines

Quoted 5 lines

block/blk-timeout.c line 138

Quoted 2 lines

block/blk-timeout.c +2 -0

+		if (!next || time_after(next, rq->deadline))
+			next = rq->deadline;

But if blk_rq_timed_out calls __blk_complete_request, we don't want to touch the rq here, I guess.

Yes, we should not access it although I don't think the request can be freed while the existing code is accessing the deadline field.

I don't think so too.

Quoted 3 lines

What
likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Yeah, I think that the timer is called immediately here. It's unnecessary.

The patch looks good to me.

Quoted 6 lines

Quoted 3 lines

What
likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Yeah, I think that the timer is called immediately here. It's unnecessary.

Hmm, just checked the code, and indeed it does. Have the timers always behaved like that?

Quoted 7 lines · On Thu, 30 Oct 2008 08:49:07 +0100 Jens Axboe <[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

What
likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Yeah, I think that the timer is called immediately here. It's unnecessary.

Quoted 2 lines

Hmm, just checked the code, and indeed it does. Have the timers always behaved like that?

I guess so because it's unrealistic that the caller of mod_time makes sure that expires is future time, in particular if the caller wants short timeout?

Quoted 13 lines · FUJITA Tomonori

Quoted 9 lines · On Thu, 30 Oct 2008 08:49:07 +0100 Jens Axboe <[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

What
likely can happen is that we may call mod_timer with jiffies that is older than current which would call the timer immediately...

Yeah, I think that the timer is called immediately here. It's unnecessary.

Hmm, just checked the code, and indeed it does. Have the timers always behaved like that?

Quoted 3 lines

I guess so because it's unrealistic that the caller of mod_time makes sure that expires is future time, in particular if the caller wants short timeout?

It's all a little confusing, I think. When do you stop considering it a little in the past and long into the future?