From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752390AbbJTX2M (ORCPT ); Tue, 20 Oct 2015 19:28:12 -0400 Received: from mga01.intel.com ([192.55.52.88]:1908 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750910AbbJTX2I (ORCPT ); Tue, 20 Oct 2015 19:28:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,709,1437462000"; d="scan'208";a="668379620" Date: Tue, 20 Oct 2015 16:17:55 -0700 From: Yunhong Jiang To: Thomas Gleixner Cc: arjan@linux.intel.com, linux-kernel@vger.kernel.org, rjw@rjwysocki.net, viresh.kumar@linaro.org Subject: Re: [RFC] nohz: no update idle entry time on get_cpu_idle/iowait_time_us() Message-ID: <20151020231755.GC31289@jnakajim-build> References: <1443488484-24305-1-git-send-email-yunhong.jiang@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Oct 11, 2015 at 08:38:17PM +0200, Thomas Gleixner wrote: > On Mon, 28 Sep 2015, Yunhong Jiang wrote: > > Currently the get_cpu_idle/iowait_time_us() updates the idle_entrytime. > > When it's invoked from another CPU and the target CPU has been on idle > > already, it will update the idle_entrytime to now, which is incorrect. > > > > However, the get_cpu_idle/iowait_time_us() is not guranteed to be called > > on the target CPU. For example, the get_cpu_idle_time_us() seems is > > invoked remotely on drivers/cpufreq/cpufreq_governor.c through > > get_cpu_idle_time(). > > > > There is a check that the update happens only if a valid last_update_time > > parameter passed. IMHO, this is more a hack because there is no guarantee > > that it's invoked on the target CPU when last_update_time is valid. > > Looking at the call sites, this last_update_time parameter is > silly. Why is the calling code not taking the timestamp? There is > hardly a requirement that this needs to be the same timestamp as the > one which is used to calculate idle time. That cpufreq calculations > are speculative anyway. > > So we better get rid of that parameter completely. Sure, will change the patch accordingly. > > > In fact, we don't need update the idle stats from > > get_cpu_idle/iowait_time_us(). Now the policy is, we record the > > entrytime when tick_nohz_start_idle() and update the stats > > when tick_nohz_stop_idle(). We calculate the stats on other situations. > > > > Please notice: > > 1) There is a bug currently that the tick_nohz_stop_idle() calls the > > update_ts_time_stats() and update the idle_entrytime, which is sure > > to be wrong. Removing the idle_entrytime update resolves the bug also. > > Care to explain the actual bug and what wreckage it causes? If it's a > real bug then removing "ts->idle_entrytime = now" needs to be a > separate patch. AFAICT, it's a cosmetic issue. Thanks for review and sorry for slow response. Yes, it's an exaggeration to call it a bug. I just thought that updating the idle_entrytime when exiting the idle state should be something wrong. But since the idle_entrytime is not used meaningful, it has no impact. > > > 2) There is a small widows in tick_nohz_stop_idle() between > > update_ts_time_stats() and clear ts->idle_active, that > > get_cpu_idle/iowait_time_us(), when invoked remotely, may double > > count last idle period. This window exists w/o this change and this > > change does not fix it. > > Calling any of those functions from a remote cpu is broken to begin > with, especially on 32bit machines. And that does not change with your > patch at all. Yes, as stated, this change does not fix it. Arjan also said that get_cpu_idle/iowait_time_us() should not be called from remote CPUs, but as stated, drivers/cpufreq/cpufreq_governor.c seems trying to invoke them remotely. I have no idea of the impact, Rafael or Viresh, can you give me some hints? > > What we really need here is protecting the idle stats fields with a > raw_spinlock. If no calling from remote CPU, then we don't need the spinlock protection, right? Thanks --jyh > > Thanks, > > tglx