From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756555Ab0ENNwy (ORCPT ); Fri, 14 May 2010 09:52:54 -0400 Received: from fg-out-1718.google.com ([72.14.220.154]:48110 "EHLO fg-out-1718.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751277Ab0ENNww (ORCPT ); Fri, 14 May 2010 09:52:52 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=b9G29nBUqQwV08I4cQIULTY4kUlcWEXkCbn8TSRq5W5pWBeLcbg08wyS5F6kl/aaKM EjqhM2nNjVQinE50rKQpp2PmHBzYEPbCHQwem2mSQiIlby8mpTb75nCY8f3cHRRHJiL6 yH3QccQ8SYkGlCGLsHCcvlFiRlTDmeWfp1Ado= Date: Fri, 14 May 2010 17:52:40 +0400 From: Cyrill Gorcunov To: Ingo Molnar Cc: Lin Ming , Jaswinder Singh Rajput , Linux Kernel Mailing List , Peter Zijlstra , Frederic Weisbecker Subject: Re: Performance Events hangs with Intel P4 system Message-ID: <20100514135240.GA4952@lenovo> References: <1273834571.3530.82.camel@minggr.sh.intel.com> <20100514115655.GA18069@elte.hu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100514115655.GA18069@elte.hu> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 14, 2010 at 01:56:55PM +0200, Ingo Molnar wrote: > > * Lin Ming wrote: > > > p4_event_bind::cntr is "unsigned char". > > But p4_next_cntr has return type of "int". > > So the explicit conversion is needed to get the correct result. > > > @@ -780,7 +780,7 @@ static int p4_pmu_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign > > if (unlikely(escr_idx == -1)) > > goto done; > > > > - if (hwc->idx != -1 && !p4_should_swap_ts(hwc->config, cpu)) { > > + if (hwc->idx != (unsigned char)-1 && !p4_should_swap_ts(hwc->config, cpu)) { > > That cast is _extremely_ ugly. Please fix the signedness of the types instead. > > Ingo > Ingo, what about this one? Jaswinder could you give it a shot (untested)? -- Cyrill --- [PATCH -tip/master] x86,perf: P4 PMU - fix counters allocation logic and sign issue Jaswinder reported GP: | | Message from syslogd@ht at May 14 09:39:32 ... | kernel:[ 314.908612] EIP: [] | x86_perf_event_set_period+0x19d/0x1b2 SS:ESP 0068:edac3d70 | Ming has narrowed it down to comparation issue between signed/unsigned values. As result event index reaches value 255 which in turn leads to GP fault. Also it was found that p4_next_cntr has a broken logic and should return counter index if only it was not yet borrowed for another event. Reported-by: Jaswinder Singh Rajput Reported-by: Lin Ming Bisected-by: Lin Ming CC: Peter Zijlstra CC: Ingo Molnar CC: Frederic Weisbecker Signed-off-by: Cyrill Gorcunov --- arch/x86/kernel/cpu/perf_event_p4.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c ===================================================================== --- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c +++ linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c @@ -18,7 +18,7 @@ struct p4_event_bind { unsigned int opcode; /* Event code and ESCR selector */ unsigned int escr_msr[2]; /* ESCR MSR for this event */ - unsigned char cntr[2][P4_CNTR_LIMIT]; /* counter index (offset), -1 on abscence */ + char cntr[2][P4_CNTR_LIMIT]; /* counter index (offset), -1 on abscence */ }; struct p4_cache_event_bind { @@ -747,11 +747,11 @@ static int p4_get_escr_idx(unsigned int static int p4_next_cntr(int thread, unsigned long *used_mask, struct p4_event_bind *bind) { - int i = 0, j; + int i, j; for (i = 0; i < P4_CNTR_LIMIT; i++) { - j = bind->cntr[thread][i++]; - if (j == -1 || !test_bit(j, used_mask)) + j = (int)bind->cntr[thread][i]; + if (j != -1 && !test_bit(j, used_mask)) return j; }