Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241855 > unrolled thread

Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not  -1 on a match Prarit Bhargava <prarit@redhat.com> - 2015-10-08 01:30 +0200
    Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1  on a match Ingo Molnar <mingo@kernel.org> - 2015-10-08 11:00 +0200

#1241855 — Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match

re-ping on this.  Just making sure this wasn't dropped on the floor.

P.

On 08/24/2015 02:22 PM, 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.
> 
> It looks like the code never does, for example, (test_bit() == 1) so this
> change should not have any impact.
> 
> [v2]: hpa: Use setc, (Set if Carry, "Sets the byte in the operand to 1 if
> the Carry Flag is set, otherwise sets the operand to 0.") instead of !!
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  arch/x86/include/asm/bitops.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index cfe3b95..c0bff87 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -313,10 +313,10 @@ static __always_inline int constant_test_bit(long nr, const volatile unsigned lo
>  
>  static inline int variable_test_bit(long nr, volatile const unsigned long *addr)
>  {
> -	int oldbit;
> +	u8 oldbit;
>  
>  	asm volatile("bt %2,%1\n\t"
> -		     "sbb %0,%0"
> +		     "setc %0"
>  		     : "=r" (oldbit)
>  		     : "m" (*(unsigned long *)addr), "Ir" (nr));
>  
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242092 — Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match

* Prarit Bhargava <prarit@redhat.com> wrote:

> re-ping on this.  Just making sure this wasn't dropped on the floor.

So I didn't apply it back when I saw your patch because I didn't see where you 
addressed/analyzed the second paragraph of hpa's review:

  "The downside with set is that it only sets a single byte, the upside is that it 
   always outputs 0 or 1, and apparently if the output variable is your bool gcc 
   can use that for optimization."

That's a valid concern and should be addressed.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web