Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757541AbbGGOqI (ORCPT ); Tue, 7 Jul 2015 10:46:08 -0400 Received: from mail-wg0-f53.google.com ([74.125.82.53]:34146 "EHLO mail-wg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757350AbbGGOqF (ORCPT ); Tue, 7 Jul 2015 10:46:05 -0400 Date: Tue, 7 Jul 2015 16:45:59 +0200 From: Frederic Weisbecker To: Preeti U Murthy Cc: LKML , Peter Zijlstra , Thomas Gleixner , Christoph Lameter , Ingo Molnar , Viresh Kumar , Rik van Riel Subject: Re: [PATCH 4/8] nohz: Remove idle task special case Message-ID: <20150707144557.GF20498@lerouge> References: <1434044168-23173-1-git-send-email-fweisbec@gmail.com> <1434044168-23173-5-git-send-email-fweisbec@gmail.com> <557CDC82.9080903@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <557CDC82.9080903@linux.vnet.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3804 Lines: 97 On Sun, Jun 14, 2015 at 07:14:34AM +0530, Preeti U Murthy wrote: > On 06/11/2015 11:06 PM, Frederic Weisbecker wrote: > > This is a leftover from old days to avoid conflicts with dynticks idle > > code. Now full dynticks and idle dynticks are better integrated and > > interact without known issue. > > I am sorry but I fail to understand why the check on idle task was there > in the first place in the below code paths. It would help if you could > clarify this in the changelog as well. Because dynticks-idle maintains various stats that were ignored by nohz full. Both machinery were so badly integrated to each other that if the nohz full code started or stopped the tick in idle on behalf of the dynticks idle code, the relevant stats got ignored or buggy. But now the code has been consolidated to handle that. I'll try to improve the changelog. > > > > > So lets remove it. > > > > Cc: Christoph Lameter > > Cc: Ingo Molnar > > Cc; John Stultz > > Cc: Peter Zijlstra > > Cc: Preeti U Murthy > > Cc: Rik van Riel > > Cc: Thomas Gleixner > > Cc: Viresh Kumar > > Signed-off-by: Frederic Weisbecker > > --- > > kernel/time/tick-sched.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > > index 812f7a3..324482f 100644 > > --- a/kernel/time/tick-sched.c > > +++ b/kernel/time/tick-sched.c > > @@ -208,10 +208,8 @@ void __tick_nohz_full_check(void) > > struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched); > > > > if (tick_nohz_full_cpu(smp_processor_id())) { > > - if (ts->tick_stopped && !is_idle_task(current)) { > > - if (!can_stop_full_tick()) > > can_stop_full_tick() would have bailed out if the current task was idle, > since it checks for the number of tasks being greater than 1 to restart > the tick. So why was the check is_idle_task() introduced earlier ? If the tick was stopped by dynticks idle and we received that IPI before the tick got a chance to be restarted by the dynticks-idle code, the tick could be restarted by nohz full and break some idle related statistics. So we had to introduce that condition before the code got consolidated. > > > - tick_nohz_restart_sched_tick(ts, ktime_get()); > > - } > > + if (ts->tick_stopped && !can_stop_full_tick()) > > + tick_nohz_restart_sched_tick(ts, ktime_get()); > > } > > } > > > > @@ -710,7 +708,7 @@ static void tick_nohz_full_stop_tick(struct tick_sched *ts) > > #ifdef CONFIG_NO_HZ_FULL > > int cpu = smp_processor_id(); > > > > - if (!tick_nohz_full_cpu(cpu) || is_idle_task(current)) > > + if (!tick_nohz_full_cpu(cpu)) > > If the current task was indeed idle, the check on ts->inidle would have > succeeded in tick_irq_exit() and we would not have reached this function > at all, isn't it? So here too I am unable to understand why we had it in > the first place. is_idle_task() means that we run the idle task. But ts->inidle means that we run the idle task between tick_nohz_idle_enter() and tick_nohz_idle_exit(). So there was a chance that we stopped the tick with full dynticks and then tick_nohz_idle_enter() gets called with tick disabled and break some stats in the way. Again that's all leftover junk from early code. > Regards > Preeti U Murthy > > return; > > > > if (!ts->tick_stopped && ts->nohz_mode == NOHZ_MODE_INACTIVE) > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/