Re: [PATCH] kernel: time: forward timer base before migrating timers

[Chandra Sekhar Lingutla]

Hi,
Sorry for the basic mistakes, posted new patch V1.

On 1/17/2018 5:39 PM, Thomas Gleixner wrote:

On Wed, 17 Jan 2018, Thomas Gleixner wrote:

On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

And please fix the subject line:

 kernel: time:

is not the proper subsystem prefix.

git log should give you a hint.

Aside of that the text after the prefix starts with an uppercase letter.

Thanks,

tglx

Re: [PATCH] kernel: time: forward timer base before migrating timers

[Chandra Sekhar Lingutla]

Hi,
Sorry for the basic mistakes, posted new patch V1.

On 1/17/2018 5:39 PM, Thomas Gleixner wrote:

On Wed, 17 Jan 2018, Thomas Gleixner wrote:

On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

And please fix the subject line:

 kernel: time:

is not the proper subsystem prefix.

git log should give you a hint.

Aside of that the text after the prefix starts with an uppercase letter.

Thanks,

tglx

Re: [PATCH] kernel: time: forward timer base before migrating timers

[Thomas Gleixner]

On Wed, 17 Jan 2018, Thomas Gleixner wrote:
> On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

And please fix the subject line:

kernel: time:

is not the proper subsystem prefix.

git log should give you a hint.

Aside of that the text after the prefix starts with an uppercase letter.

Thanks,

tglx

Re: [PATCH] kernel: time: forward timer base before migrating timers

[Thomas Gleixner]

On Wed, 17 Jan 2018, Thomas Gleixner wrote:
> On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

And please fix the subject line:

kernel: time:

is not the proper subsystem prefix.

git log should give you a hint.

Aside of that the text after the prefix starts with an uppercase letter.

Thanks,

tglx

Re: [PATCH] kernel: time: forward timer base before migrating timers

[Thomas Gleixner]

On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

> In case when timers are migrated to a CPU, after it exits
> idle, but before timer base is forwarded, either from
> run_timer_softirq()/mod_timer()/add_timer_on(), it's
> possible that migrated timers are queued, based on older
> clock value. This can cause delays in handling those timers.
> 
> For example, consider below sequence of events:
> 
> - CPU0 timer1 expires = 59969 and base->clk = 59131. So,
>   timer is queued at level 2, with next expiry for this timer
>   = 60032 (due to granularity addition).
> - CPU1 enters idle @60007, with next timer expiry @60020.
> - CPU1 exits idle.
> - CPU0 is hotplugged at 60009, and timers are migrated to
>   CPU1, with new base->clk = 60007. timer1 is queued,
>   based on 60007 at level 0, for immediate handling (in
>   next timer softirq handling).
> - CPU1's base->clk is forwarded to 60009, so, in next sched
>   timer interrupt, timer1 is not handled.
> 
> The issue happens as timer wheel collects expired timers
> starting from the current clk's index onwards, but migrated
> timers, if enqueued, based on older clk value can result
> in their index being less than clk's current index.
> This can only happen if new base->clk is ahead of
> timer->expires, resulting in timer being queued at
> new base->clk's current index.
> 
> Change-Id: Idbe737b346f00e6e7241b93181bbbd80871f1400

I asked that a gazillion of times now.

Please do not include your internal change id tags. They are completely
useless for anybody else. Can you please make sure that anyone inside
codeaurora knows?

> Signed-off-by: Neeraj Upadhyay 
> Signed-off-by: Lingutla Chandrasekhar 

This SOB chain is wrong 

> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 89a9e1b4264a..ae94aa97b5a9 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1886,6 +1886,11 @@ int timers_dead_cpu(unsigned int cpu)
>   raw_spin_lock_irq(_base->lock);
>   raw_spin_lock_nested(_base->lock, SINGLE_DEPTH_NESTING);
>  
> + /* Before migrating timers, update new base clk to avoid
> +  * queueing timers based on older clock value.
> +  */

Multiline comments are

/*
 * First line
 * Second line
 */

Please do not use that retarded comment style above.

Thanks,

tglx

Re: [PATCH] kernel: time: forward timer base before migrating timers

[Thomas Gleixner]

On Wed, 17 Jan 2018, Lingutla Chandrasekhar wrote:

> In case when timers are migrated to a CPU, after it exits
> idle, but before timer base is forwarded, either from
> run_timer_softirq()/mod_timer()/add_timer_on(), it's
> possible that migrated timers are queued, based on older
> clock value. This can cause delays in handling those timers.
> 
> For example, consider below sequence of events:
> 
> - CPU0 timer1 expires = 59969 and base->clk = 59131. So,
>   timer is queued at level 2, with next expiry for this timer
>   = 60032 (due to granularity addition).
> - CPU1 enters idle @60007, with next timer expiry @60020.
> - CPU1 exits idle.
> - CPU0 is hotplugged at 60009, and timers are migrated to
>   CPU1, with new base->clk = 60007. timer1 is queued,
>   based on 60007 at level 0, for immediate handling (in
>   next timer softirq handling).
> - CPU1's base->clk is forwarded to 60009, so, in next sched
>   timer interrupt, timer1 is not handled.
> 
> The issue happens as timer wheel collects expired timers
> starting from the current clk's index onwards, but migrated
> timers, if enqueued, based on older clk value can result
> in their index being less than clk's current index.
> This can only happen if new base->clk is ahead of
> timer->expires, resulting in timer being queued at
> new base->clk's current index.
> 
> Change-Id: Idbe737b346f00e6e7241b93181bbbd80871f1400

I asked that a gazillion of times now.

Please do not include your internal change id tags. They are completely
useless for anybody else. Can you please make sure that anyone inside
codeaurora knows?

> Signed-off-by: Neeraj Upadhyay 
> Signed-off-by: Lingutla Chandrasekhar 

This SOB chain is wrong 

> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 89a9e1b4264a..ae94aa97b5a9 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1886,6 +1886,11 @@ int timers_dead_cpu(unsigned int cpu)
>   raw_spin_lock_irq(_base->lock);
>   raw_spin_lock_nested(_base->lock, SINGLE_DEPTH_NESTING);
>  
> + /* Before migrating timers, update new base clk to avoid
> +  * queueing timers based on older clock value.
> +  */

Multiline comments are

/*
 * First line
 * Second line
 */

Please do not use that retarded comment style above.

Thanks,

tglx

[PATCH] kernel: time: forward timer base before migrating timers

[Lingutla Chandrasekhar]

In case when timers are migrated to a CPU, after it exits
idle, but before timer base is forwarded, either from
run_timer_softirq()/mod_timer()/add_timer_on(), it's
possible that migrated timers are queued, based on older
clock value. This can cause delays in handling those timers.

For example, consider below sequence of events:

- CPU0 timer1 expires = 59969 and base->clk = 59131. So,
  timer is queued at level 2, with next expiry for this timer
  = 60032 (due to granularity addition).
- CPU1 enters idle @60007, with next timer expiry @60020.
- CPU1 exits idle.
- CPU0 is hotplugged at 60009, and timers are migrated to
  CPU1, with new base->clk = 60007. timer1 is queued,
  based on 60007 at level 0, for immediate handling (in
  next timer softirq handling).
- CPU1's base->clk is forwarded to 60009, so, in next sched
  timer interrupt, timer1 is not handled.

The issue happens as timer wheel collects expired timers
starting from the current clk's index onwards, but migrated
timers, if enqueued, based on older clk value can result
in their index being less than clk's current index.
This can only happen if new base->clk is ahead of
timer->expires, resulting in timer being queued at
new base->clk's current index.

Change-Id: Idbe737b346f00e6e7241b93181bbbd80871f1400
Signed-off-by: Neeraj Upadhyay 
Signed-off-by: Lingutla Chandrasekhar 

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 89a9e1b4264a..ae94aa97b5a9 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1886,6 +1886,11 @@ int timers_dead_cpu(unsigned int cpu)
raw_spin_lock_irq(_base->lock);
raw_spin_lock_nested(_base->lock, SINGLE_DEPTH_NESTING);
 
+   /* Before migrating timers, update new base clk to avoid
+* queueing timers based on older clock value.
+*/
+   forward_timer_base(new_base);
+
BUG_ON(old_base->running_timer);
 
for (i = 0; i < WHEEL_SIZE; i++)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum,
 a Linux Foundation Collaborative Project.

[PATCH] kernel: time: forward timer base before migrating timers

[Lingutla Chandrasekhar]

In case when timers are migrated to a CPU, after it exits
idle, but before timer base is forwarded, either from
run_timer_softirq()/mod_timer()/add_timer_on(), it's
possible that migrated timers are queued, based on older
clock value. This can cause delays in handling those timers.

For example, consider below sequence of events:

- CPU0 timer1 expires = 59969 and base->clk = 59131. So,
  timer is queued at level 2, with next expiry for this timer
  = 60032 (due to granularity addition).
- CPU1 enters idle @60007, with next timer expiry @60020.
- CPU1 exits idle.
- CPU0 is hotplugged at 60009, and timers are migrated to
  CPU1, with new base->clk = 60007. timer1 is queued,
  based on 60007 at level 0, for immediate handling (in
  next timer softirq handling).
- CPU1's base->clk is forwarded to 60009, so, in next sched
  timer interrupt, timer1 is not handled.

The issue happens as timer wheel collects expired timers
starting from the current clk's index onwards, but migrated
timers, if enqueued, based on older clk value can result
in their index being less than clk's current index.
This can only happen if new base->clk is ahead of
timer->expires, resulting in timer being queued at
new base->clk's current index.

Change-Id: Idbe737b346f00e6e7241b93181bbbd80871f1400
Signed-off-by: Neeraj Upadhyay 
Signed-off-by: Lingutla Chandrasekhar 

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 89a9e1b4264a..ae94aa97b5a9 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1886,6 +1886,11 @@ int timers_dead_cpu(unsigned int cpu)
raw_spin_lock_irq(_base->lock);
raw_spin_lock_nested(_base->lock, SINGLE_DEPTH_NESTING);
 
+   /* Before migrating timers, update new base clk to avoid
+* queueing timers based on older clock value.
+*/
+   forward_timer_base(new_base);
+
BUG_ON(old_base->running_timer);
 
for (i = 0; i < WHEEL_SIZE; i++)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum,
 a Linux Foundation Collaborative Project.