From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753470AbbHVJOV (ORCPT ); Sat, 22 Aug 2015 05:14:21 -0400 Received: from mail-wi0-f180.google.com ([209.85.212.180]:34290 "EHLO mail-wi0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753240AbbHVJOT (ORCPT ); Sat, 22 Aug 2015 05:14:19 -0400 Date: Sat, 22 Aug 2015 11:14:14 +0200 From: Ingo Molnar To: Prarit Bhargava Cc: linux-kernel@vger.kernel.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Linus Torvalds , Andrew Morton , Peter Zijlstra Subject: Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match Message-ID: <20150822091414.GA18050@gmail.com> References: <1440004734-24290-1-git-send-email-prarit@redhat.com> <20150821065103.GA4541@gmail.com> <55D7106D.3040904@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55D7106D.3040904@redhat.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 * Prarit Bhargava wrote: > > > On 08/21/2015 02:51 AM, Ingo Molnar wrote: > > > > * Prarit Bhargava wrote: > > > >> This issue was noticed while debugging a CPU hotplug issue. On x86 > >> with (NR_CPUS > 1) the cpu_online() define is cpumask_test_cpu(). > >> cpumask_test_cpu() should return 1 if the cpu is set in cpumask and > >> 0 otherwise. > >> > >> However, cpumask_test_cpu() returns -1 if the cpu in the cpumask is > >> set and 0 otherwise. This happens because cpumask_test_cpu() calls > >> test_bit() which is a define that will call variable_test_bit(). > >> > >> variable_test_bit() calls the assembler instruction sbb (Subtract > >> with Borrow, " Subtracts the source from the destination, and subtracts 1 > >> extra if the Carry Flag is set. Results are returned in "dest".) > >> > >> A bit match results in -1 being returned from variable_test_bit() if a > >> match occurs, not 1 as the function is supposed to. This can be easily > >> resolved by adding a "!!" to force 0 or 1 as a return. > >> > >> It looks like the code never does, for example, (test_bit() == 1) so this > >> change should not have any impact. > >> > >> Cc: Thomas Gleixner > >> Cc: Ingo Molnar > >> Cc: "H. Peter Anvin" > >> Cc: x86@kernel.org > >> Cc: linux-kernel@vger.kernel.org > >> Signed-off-by: Prarit Bhargava > >> --- > >> arch/x86/include/asm/bitops.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h > >> index cfe3b95..a87a5fb 100644 > >> --- a/arch/x86/include/asm/bitops.h > >> +++ b/arch/x86/include/asm/bitops.h > >> @@ -320,7 +320,7 @@ static inline int variable_test_bit(long nr, volatile const unsigned long *addr) > >> : "=r" (oldbit) > >> : "m" (*(unsigned long *)addr), "Ir" (nr)); > >> > >> - return oldbit; > >> + return !!oldbit; > >> } > >> > >> #if 0 /* Fool kernel-doc since it doesn't do macros yet */ > > > > Ok, I think this is a good fix to improve the robustness of this primitive, unless > > someone objects. > > > > I tried to find the CPU hotplug code that broke with cpu_online() returning -1 but > > failed - all current mainline usage sites seem to be testing for nonzero in one > > way or another. Could you please point it out? > > I'm sorry Ingo, I think my description may have confused you. I was debugging a > cpu hotplug issue[1] and did > > printk("cpu %d cpu online status %d\n", cpu, cpu_online(cpu)); > > as a debug printk. This printed out > > cpu 3 cpu online status -1 > > which was really confusing. That lead me down the rabbit hole of looking at the > sbb assembler instruction in variable_test_bit() to figure out why I was seeing -1. Ok, fair enough! Still worth fixing IMHO. Thanks, Ingo