From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752620AbdJFVGk (ORCPT ); Fri, 6 Oct 2017 17:06:40 -0400 Received: from mga05.intel.com ([192.55.52.43]:37407 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751491AbdJFVGj (ORCPT ); Fri, 6 Oct 2017 17:06:39 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,484,1500966000"; d="scan'208";a="1179494569" Date: Fri, 6 Oct 2017 14:09:13 -0700 From: Jacob Pan To: Jason Baron Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, jacob.jun.pan@linux.intel.com Subject: Re: [PATCH] intel_idle: replace conditionals with static_cpu_has(X86_FEATURE_ARAT) Message-ID: <20171006140913.33417872@jacob-builder> In-Reply-To: References: <1507310385-22388-1-git-send-email-jbaron@akamai.com> <20171006113633.0186eb78@jacob-builder> Organization: OTC X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; 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 Fri, 6 Oct 2017 14:41:07 -0400 Jason Baron wrote: > On 10/06/2017 02:36 PM, Jacob Pan wrote: > > On Fri, 6 Oct 2017 13:19:45 -0400 > > Jason Baron wrote: > > > >> If the 'arat' cpu flag is set, then the conditionals in > >> intel_idle() that guard calling tick_broadcast_enter()/exit() will > >> never be true. Use static_cpu_has(X86_FEATURE_ARAT) to create a > >> fast path to replace the conditional. > >> > >> Signed-off-by: Jason Baron > >> Cc: Jacob Pan > >> Cc: Len Brown > >> Cc: Rafael J. Wysocki > >> --- > >> drivers/idle/intel_idle.c | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > >> index 5dc7ea4..5db5e31 100644 > >> --- a/drivers/idle/intel_idle.c > >> +++ b/drivers/idle/intel_idle.c > >> @@ -913,8 +913,7 @@ static __cpuidle int intel_idle(struct > >> cpuidle_device *dev, struct cpuidle_state *state = > >> &drv->states[index]; unsigned long eax = flg2MWAIT(state->flags); > >> unsigned int cstate; > >> - > >> - cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & > >> MWAIT_CSTATE_MASK) + 1; > >> + bool uninitialized_var(tick); > >> > >> /* > >> * NB: if CPUIDLE_FLAG_TLB_FLUSHED is set, this idle > >> transition @@ -923,12 +922,19 @@ static __cpuidle int > >> intel_idle(struct cpuidle_device *dev, > >> * useful with this knowledge. > >> */ > >> > >> - if (!(lapic_timer_reliable_states & (1 << (cstate)))) > >> - tick_broadcast_enter(); > >> + if (!static_cpu_has(X86_FEATURE_ARAT)) { > >> + cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & > >> + MWAIT_CSTATE_MASK) + 1; > >> + tick = false; > >> + if (!(lapic_timer_reliable_states & (1 << > >> (cstate)))) { > >> + tick = true; > >> + tick_broadcast_enter(); > >> + } > >> + } > >> > >> mwait_idle_with_hints(eax, ecx); > >> > >> - if (!(lapic_timer_reliable_states & (1 << (cstate)))) > >> + if (!static_cpu_has(X86_FEATURE_ARAT) && tick) > >> tick_broadcast_exit(); > >> > >> return index; > > > > Seems better to have a function pointer set up at init time to > > select whether we do tick_broadcast or not (two functions). There > > is no need to check CPU feature on every entry. > > > > Hi, > > static_cpu_has() uses alternatives patching, so the cpu feature is not > tested on every entry. With the arat flag set you just have two nops > in the straight-line code path with this patch. > Thanks for explaining, i didn't know it was self modifying. Acked-by: Jacob Pan