[PATCH] block: optimizations in blk_rq_timed_out_timer()

[PATCH] block: optimizations in blk_rq_timed_out_timer()

[malahal]

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 <malahal@us.ibm.com>

diff -r a6ae42397ede block/blk-timeout.c
--- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
+++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
@@ -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);
@@ -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;
 		}
-		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);

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[FUJITA Tomonori]

On Tue, 28 Oct 2008 11:22:43 -0700
malahal@us.ibm.com 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 <malahal@us.ibm.com>
> 
> diff -r a6ae42397ede block/blk-timeout.c
> --- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
> +++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
> @@ -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);
> @@ -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 (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.


>  		}
> -		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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[malahal]

FUJITA Tomonori [fujita.tomonori@lab.ntt.co.jp] wrote:
> On Tue, 28 Oct 2008 11:22:43 -0700
> malahal@us.ibm.com 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 <malahal@us.ibm.com>
> > 
> > diff -r a6ae42397ede block/blk-timeout.c
> > --- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
> > +++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
> > @@ -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);
> > @@ -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.

> 
> >  			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...

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[Jens Axboe]

On Tue, Oct 28 2008, malahal@us.ibm.com wrote:
> FUJITA Tomonori [fujita.tomonori@lab.ntt.co.jp] wrote:
> > On Tue, 28 Oct 2008 11:22:43 -0700
> > malahal@us.ibm.com 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 <malahal@us.ibm.com>
> > > 
> > > diff -r a6ae42397ede block/blk-timeout.c
> > > --- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
> > > +++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
> > > @@ -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);
> > > @@ -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.
> 
> > 
> > >  			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...

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 :-)

-- 
Jens Axboe

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[malahal]

Jens Axboe [jens.axboe@oracle.com] wrote:
> > 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 :-)

I remember the timer code treating "past" expiry times as special. If
not, then we have a real bug! It is also hard to guarantee in
blk_rq_timed_out_timer() that we always call mod_timer with future
expiry time!

--Malahal.

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[FUJITA Tomonori]

On Tue, 28 Oct 2008 23:06:25 -0700
malahal@us.ibm.com wrote:

> FUJITA Tomonori [fujita.tomonori@lab.ntt.co.jp] wrote:
> > On Tue, 28 Oct 2008 11:22:43 -0700
> > malahal@us.ibm.com 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 <malahal@us.ibm.com>
> > > 
> > > diff -r a6ae42397ede block/blk-timeout.c
> > > --- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
> > > +++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
> > > @@ -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);
> > > @@ -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.

Ah, I see.


> > >  			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.

I don't think so too.

> 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.

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[Jens Axboe]

On Thu, Oct 30 2008, FUJITA Tomonori wrote:
> On Tue, 28 Oct 2008 23:06:25 -0700
> malahal@us.ibm.com wrote:
> 
> > FUJITA Tomonori [fujita.tomonori@lab.ntt.co.jp] wrote:
> > > On Tue, 28 Oct 2008 11:22:43 -0700
> > > malahal@us.ibm.com 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 <malahal@us.ibm.com>
> > > > 
> > > > diff -r a6ae42397ede block/blk-timeout.c
> > > > --- a/block/blk-timeout.c	Thu Oct 23 11:48:45 2008 -0700
> > > > +++ b/block/blk-timeout.c	Fri Oct 24 17:08:24 2008 -0700
> > > > @@ -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);
> > > > @@ -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.
> 
> Ah, I see.
> 
> 
> > > >  			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.
> 
> I don't think so too.
> 
> > 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?

> The patch looks good to me.

No further gried from me either, I've applied it. It does need a comment
on why 'next' cannot be 0 validly, because ->deadline is always rounded
one up if that happens.

-- 
Jens Axboe

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[FUJITA Tomonori]

On Thu, 30 Oct 2008 08:49:07 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> > > 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?

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?

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[Jens Axboe]

On Thu, Oct 30 2008, FUJITA Tomonori wrote:
> On Thu, 30 Oct 2008 08:49:07 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > > > 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?
> 
> 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?

-- 
Jens Axboe

Re: [PATCH] block: optimizations in blk_rq_timed_out_timer()

[FUJITA Tomonori]

On Thu, 30 Oct 2008 09:55:20 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Oct 30 2008, FUJITA Tomonori wrote:
> > On Thu, 30 Oct 2008 08:49:07 +0100
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > > > 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?
> > 
> > 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?

After looking at internal_add_timer(), I guess, if the difference is
less than the largest of signed long, it is considered to be the
past. But I can say that it's not a good idea to ask me about the
timer implementation details. ;)