[PATCH] kernel: timer: looping issue, need reset variable 'found'

[PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

According to __internal_add_timer(), in _next_timer_interrupt(), when
'tv1.vec' find one, but need 'cascade bucket(s)', we still need find
each slot of 'tv*.vec'.

So need reset variable 'found', so can fully scan ''do {...} while()''
for 'tv*.vec'.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/timer.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index aa8b964..0fce618 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1197,7 +1197,7 @@ static unsigned long __next_timer_interrupt(struct tvec_base *base)
 {
 	unsigned long timer_jiffies = base->timer_jiffies;
 	unsigned long expires = timer_jiffies + NEXT_TIMER_MAX_DELTA;
-	int index, slot, array, found = 0;
+	int index, slot, array;
 	struct timer_list *nte;
 	struct tvec *varray[4];
 
@@ -1208,7 +1208,6 @@ static unsigned long __next_timer_interrupt(struct tvec_base *base)
 			if (tbase_get_deferrable(nte->base))
 				continue;
 
-			found = 1;
 			expires = nte->expires;
 			/* Look at the cascade bucket(s)? */
 			if (!index || slot < index)
@@ -1232,6 +1231,7 @@ cascade:
 
 	for (array = 0; array < 4; array++) {
 		struct tvec *varp = varray[array];
+		bool found = false;
 
 		index = slot = timer_jiffies & TVN_MASK;
 		do {
@@ -1239,7 +1239,7 @@ cascade:
 				if (tbase_get_deferrable(nte->base))
 					continue;
 
-				found = 1;
+				found = true;
 				if (time_before(nte->expires, expires))
 					expires = nte->expires;
 			}
-- 
1.7.7.6

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Thomas Gleixner]

On Sun, 9 Jun 2013, Chen Gang wrote:

> 
> According to __internal_add_timer(), in _next_timer_interrupt(), when
> 'tv1.vec' find one, but need 'cascade bucket(s)', we still need find
> each slot of 'tv*.vec'.

No, we do not. We only need to scan the first cascade array after the
enqueued timer. If there is nothing in tv2 which might come before the
found timer, then any timer in tv3 will be later than the one we found
in the primary wheel.

> So need reset variable 'found', so can fully scan ''do {...} while()''
> for 'tv*.vec'.

And thereby lose the information, that we already found a timer in the
scan of the primary array.

Thanks,

	tglx

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

Sorry for replying late during these days, firstly.


On 06/10/2013 10:12 PM, Thomas Gleixner wrote:
> On Sun, 9 Jun 2013, Chen Gang wrote:
> 
>> > 
>> > According to __internal_add_timer(), in _next_timer_interrupt(), when
>> > 'tv1.vec' find one, but need 'cascade bucket(s)', we still need find
>> > each slot of 'tv*.vec'.
> No, we do not. We only need to scan the first cascade array after the
> enqueued timer. If there is nothing in tv2 which might come before the
> found timer, then any timer in tv3 will be later than the one we found
> in the primary wheel.
> 

OK, thanks, I should read details to confirm it (at least now, I still
not quite be sure about it).


>> > So need reset variable 'found', so can fully scan ''do {...} while()''
>> > for 'tv*.vec'.
> And thereby lose the information, that we already found a timer in the
> scan of the primary array.

Hmm..., I should read details.


And excuse me, I also have to do another things during these days, so
maybe reply late.


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

Sorry again for replying late, and now I can focus on this patch, I will
see the related details again.


Thanks.

On 06/13/2013 11:39 AM, Chen Gang wrote:
> 
> Sorry for replying late during these days, firstly.
> 
> 
> On 06/10/2013 10:12 PM, Thomas Gleixner wrote:
>> On Sun, 9 Jun 2013, Chen Gang wrote:
>>
>>>>
>>>> According to __internal_add_timer(), in _next_timer_interrupt(), when
>>>> 'tv1.vec' find one, but need 'cascade bucket(s)', we still need find
>>>> each slot of 'tv*.vec'.
>> No, we do not. We only need to scan the first cascade array after the
>> enqueued timer. If there is nothing in tv2 which might come before the
>> found timer, then any timer in tv3 will be later than the one we found
>> in the primary wheel.
>>
> 
> OK, thanks, I should read details to confirm it (at least now, I still
> not quite be sure about it).
> 
> 
>>>> So need reset variable 'found', so can fully scan ''do {...} while()''
>>>> for 'tv*.vec'.
>> And thereby lose the information, that we already found a timer in the
>> scan of the primary array.
> 
> Hmm..., I should read details.
> 
> 
> And excuse me, I also have to do another things during these days, so
> maybe reply late.
> 
> 
> Thanks.
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

On 06/10/2013 10:12 PM, Thomas Gleixner wrote:
> On Sun, 9 Jun 2013, Chen Gang wrote:
> 
>>
>> According to __internal_add_timer(), in _next_timer_interrupt(), when
>> 'tv1.vec' find one, but need 'cascade bucket(s)', we still need find
>> each slot of 'tv*.vec'.
> 
> No, we do not. We only need to scan the first cascade array after the
> enqueued timer. If there is nothing in tv2 which might come before the
> found timer, then any timer in tv3 will be later than the one we found
> in the primary wheel.
> 

If we assume "If there is nothing in tv2 which might come before the
found timer, then any timer in tv3 will ..." is correct.

When we found a timer in 'tv1', we will not search all timers in 'tv2'
(we only search first looping of tv2 for the specific 'slot').

Is it still OK ?


>> So need reset variable 'found', so can fully scan ''do {...} while()''
>> for 'tv*.vec'.
> 
> And thereby lose the information, that we already found a timer in the
> scan of the primary array.
> 

When we found a timer, 'expires' would be set.  So resetting 'found' is
still correct, but may let performance lower (if original implement is
correct too)

I think we can treat original implementation as for speed optimization,
so our discussion is "whether this speed optimization has effect with
correctness".


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Thomas Gleixner]

On Tue, 18 Jun 2013, Chen Gang wrote:
> On 06/10/2013 10:12 PM, Thomas Gleixner wrote:
> I think we can treat original implementation as for speed optimization,
> so our discussion is "whether this speed optimization has effect with
> correctness".

Then I recommend that you to sit down and analyze the correctness of
the code.

Come back when you have a proof of the code being wrong. And by proof
I mean factual proof not just handwaving theories.

Thanks,

	tglx

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

On 06/20/2013 03:47 PM, Thomas Gleixner wrote:
> On Tue, 18 Jun 2013, Chen Gang wrote:
>> > On 06/10/2013 10:12 PM, Thomas Gleixner wrote:
>> > I think we can treat original implementation as for speed optimization,
>> > so our discussion is "whether this speed optimization has effect with
>> > correctness".
> Then I recommend that you to sit down and analyze the correctness of
> the code.

That is only your recommend, not mean I have duty to.

> 
> Come back when you have a proof of the code being wrong. And by proof
> I mean factual proof not just handwaving theories.


At least before say so, please reply:

-------------------------------------------------------------------------------
If we assume "If there is nothing in tv2 which might come before the
found timer, then any timer in tv3 will ..." is correct.

When we found a timer in 'tv1', we will not search all timers in 'tv2'
(we only search first looping of tv2 for the specific 'slot').

Is it still OK ?
-------------------------------------------------------------------------------


If you do not want to discuss with others, better quite politely, not
need judging or checking others, it is useless for the cooperation with
each other, is it right ?  ;-)


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/20/2013 03:47 PM, Thomas Gleixner wrote:
> -------------------------------------------------------------------------------
> If we assume "If there is nothing in tv2 which might come before the
> found timer, then any timer in tv3 will ..." is correct.
> 
> When we found a timer in 'tv1', we will not search all timers in 'tv2'
> (we only search first looping of tv2 for the specific 'slot').

Yes, because that's how the timer wheel works. And I'm not going to
explain you every little detail of it.
 
> Is it still OK ?

Yes, it is. 

> If you do not want to discuss with others, better quite politely, not
> need judging or checking others, it is useless for the cooperation with
> each other, is it right ?  ;-)

I discussed all your patches which fall into my area of responsibility
with you and I explained to you very politely why your patches are
incorrect.

When I noticed, that you do not even understand how the timer wheel
works in detail, which is necessary to understand why the code in
__next_timer_interrupt() is correct, I asked you politely:

> > Then I recommend that you to sit down and analyze the correctness of
> > the code.

And you answered:

> That is only your recommend, not mean I have duty to.

Right, it's only a recommendation. Though without proof of a failure,
I'm not going to discuss that further and I'm not going to apply a
patch.

Thanks,

	tglx

Re: [PATCH] kernel: timer: looping issue, need reset variable 'found'

[Chen Gang]

On 06/20/2013 05:13 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
>> > On 06/20/2013 03:47 PM, Thomas Gleixner wrote:
>> > -------------------------------------------------------------------------------
>> > If we assume "If there is nothing in tv2 which might come before the
>> > found timer, then any timer in tv3 will ..." is correct.
>> > 
>> > When we found a timer in 'tv1', we will not search all timers in 'tv2'
>> > (we only search first looping of tv2 for the specific 'slot').
> Yes, because that's how the timer wheel works. And I'm not going to
> explain you every little detail of it.
>  

OK, thanks.

>> > Is it still OK ?
> Yes, it is. 
> 

OK, thanks.

>> > If you do not want to discuss with others, better quite politely, not
>> > need judging or checking others, it is useless for the cooperation with
>> > each other, is it right ?  ;-)
> I discussed all your patches which fall into my area of responsibility
> with you and I explained to you very politely why your patches are
> incorrect.
> 

Firstly, thank you for your explanations in details in your area of
responsibility.

At least this means you are warm-hearted, and can spend your time
resources on it.


> When I noticed, that you do not even understand how the timer wheel
> works in detail, which is necessary to understand why the code in
> __next_timer_interrupt() is correct, I asked you politely:
> 

I really do not know about the timer wheel works in detail.

I never said I am familiar with kernel, in fact, I often said I am not
quite familiar with kernel (you can find it in my many other replying
mails).

Is it necessary to give some related comments for it (the 'found' is
really in an informal using way) ?

We'd better let our code much readable for 'many readers' (which may not
know about more details, e.g. timer wheel).

Do my reply politely ?  ;-)


>>> > > Then I recommend that you to sit down and analyze the correctness of
>>> > > the code.
> And you answered:
> 
>> > That is only your recommend, not mean I have duty to.
> Right, it's only a recommendation. Though without proof of a failure,
> I'm not going to discuss that further and I'm not going to apply a
> patch.

OK.

For cooperation, every members are joined with their own willing, not by
force, but really need duty.

This time, it does not need duty, it is really a recommend.


Thanks.
-- 
Chen Gang

Asianux Corporation