From mboxrd@z Thu Jan  1 00:00:00 1970
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752829AbeAQMGk (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Wed, 17 Jan 2018 07:06:40 -0500
Received: from Galois.linutronix.de ([146.0.238.70]:45813 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750931AbeAQMGj (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 17 Jan 2018 07:06:39 -0500
Date: Wed, 17 Jan 2018 13:06:37 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Lingutla Chandrasekhar <clingutla@codeaurora.org>
cc: linux-kernel@vger.kernel.org, neeraju@codeaurora.org,
        linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] kernel: time: forward timer base before migrating
 timers
In-Reply-To: <20180117115257.8351-1-clingutla@codeaurora.org>
Message-ID: <alpine.DEB.2.20.1801171303310.1777@nanos>
References: <20180117115257.8351-1-clingutla@codeaurora.org>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

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 <neeraju@codeaurora.org>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

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(&new_base->lock);
>  		raw_spin_lock_nested(&old_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