From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753226AbbKJOBS (ORCPT ); Tue, 10 Nov 2015 09:01:18 -0500 Received: from mga02.intel.com ([134.134.136.20]:30999 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752100AbbKJOBR (ORCPT ); Tue, 10 Nov 2015 09:01:17 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,270,1444719600"; d="scan'208";a="597561366" Date: Tue, 10 Nov 2015 06:01:16 -0800 From: Jacob Pan To: Peter Zijlstra Cc: LKML , Rafael Wysocki , Len Brown , Andi Kleen , Thomas Gleixner , Paul Turner , Tim Chen , Dietmar Eggemann , Eduardo Valentin , Punit Agrawal , Srinivas Pandruvada , jacob.jun.pan@linux.intel.com Subject: Re: [RFC PATCH v2 3/3] sched: introduce synchronized idle injection Message-ID: <20151110060116.26cd5ff8@yairi> In-Reply-To: <20151110132324.GC17308@twins.programming.kicks-ass.net> References: <1447114883-23851-1-git-send-email-jacob.jun.pan@linux.intel.com> <1447114883-23851-4-git-send-email-jacob.jun.pan@linux.intel.com> <20151110132324.GC17308@twins.programming.kicks-ass.net> Organization: OTC X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.23; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 10 Nov 2015 14:23:24 +0100 Peter Zijlstra wrote: > > +static enum hrtimer_restart idle_inject_timer_fn(struct hrtimer > > *hrtimer) +{ > > + struct hrtimer *hrt = this_cpu_ptr(&idle_inject_timer); > > + int cpu = smp_processor_id(); > > + ktime_t now, delta, period; > > + bool status; > > + > > + now = hrtimer_cb_get_time(hrt); > > You're not interested in the current time. > > > + > > + status = raw_cpu_read(idle_injected); > > + if (status) { > > + /* > > + * We were injecting idle in the last phase, let's > > forward the > > + * timer to the next period > > + * > > + * status: 1 0 1 > > 0 > > + * ____ ____________________ > > _______ > > + * |________| |_________| > > + * > > + * |duration| interval | > > + * > > + * ^ we are here > > + * forward to here: ^ > > + */ > > + delta = ktime_sub(now, inject_start_time); > > + period = ktime_add(ms_to_ktime(duration), > > + ms_to_ktime(inject_interval)); > > + delta = ktime_roundup(delta, period); > > + hrtimer_set_expires(hrt, ktime_add(delta, > > inject_start_time)); > > This doesn't make any sense. Who cares what the current time is. > > > + } else { > > + /* > > + * We were not injecting idle in the last phase, > > let's forward > > + * timer after forced idle duration > > + * ____ ____________________ > > _______ > > + * |________| |_________| > > + * > > + * |duration| interval | > > + * > > + * ^ we are here > > + * ^ forward timer to here > > + */ > > + hrtimer_set_expires(hrt, > > ktime_add(ms_to_ktime(duration), now)); > > Same here, we don't care about the current time. The timer was at the > previous start of injection, just forward it a whole period to find > the next injection slot. > > > + } > > It looks like what you want is: > > hrtimer_forward(hrt, period); > > unconditionally. In the ideal world yes. But my thinking was that timers may not be so accurate to deliver interrupts, over the time the timeout error may accumulate so that eventually timers will be out of sync. That is why at the beginning of the period I realign the timer based on a common start time so that timers will never drift off. I use current time and the common start time to locate the next absolute timeout not the relative timeout. Overall, my idea was to make it more robust and handles runtime parameters (percentage, duration) smoothly. I will fix the rest comments in the patch. Thank you, Jacob