* [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
@ 2015-08-19 17:18 Prarit Bhargava
2015-08-21 6:51 ` Ingo Molnar
0 siblings, 1 reply; 10+ messages in thread
From: Prarit Bhargava @ 2015-08-19 17:18 UTC (permalink / raw)
To: linux-kernel
Cc: Prarit Bhargava, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
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 <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 | 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 */
--
1.7.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-19 17:18 [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match Prarit Bhargava
@ 2015-08-21 6:51 ` Ingo Molnar
2015-08-21 8:08 ` H. Peter Anvin
2015-08-21 11:50 ` Prarit Bhargava
0 siblings, 2 replies; 10+ messages in thread
From: Ingo Molnar @ 2015-08-21 6:51 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Linus Torvalds, Andrew Morton, Peter Zijlstra
* Prarit Bhargava <prarit@redhat.com> 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 <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 | 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?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 6:51 ` Ingo Molnar
@ 2015-08-21 8:08 ` H. Peter Anvin
2015-08-21 11:53 ` Prarit Bhargava
` (2 more replies)
2015-08-21 11:50 ` Prarit Bhargava
1 sibling, 3 replies; 10+ messages in thread
From: H. Peter Anvin @ 2015-08-21 8:08 UTC (permalink / raw)
To: Ingo Molnar, Prarit Bhargava
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, x86, Linus Torvalds,
Andrew Morton, Peter Zijlstra
Wrong fix, though. Instead we should change it to use the set instruction, which would also make it easier to use the CC_SET/CC_OUT proposed macros to use assembly out in the future.
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.
On August 20, 2015 11:51:03 PM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Prarit Bhargava <prarit@redhat.com> 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 <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 | 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?
>
>Thanks,
>
> Ingo
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 6:51 ` Ingo Molnar
2015-08-21 8:08 ` H. Peter Anvin
@ 2015-08-21 11:50 ` Prarit Bhargava
2015-08-22 9:14 ` Ingo Molnar
1 sibling, 1 reply; 10+ messages in thread
From: Prarit Bhargava @ 2015-08-21 11:50 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Linus Torvalds, Andrew Morton, Peter Zijlstra
On 08/21/2015 02:51 AM, Ingo Molnar wrote:
>
> * Prarit Bhargava <prarit@redhat.com> 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 <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 | 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.
P.
[1] The bug looks like it has to do with the system's firmware, not cpu hotplug.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 8:08 ` H. Peter Anvin
@ 2015-08-21 11:53 ` Prarit Bhargava
2015-08-24 18:22 ` [PATCH v2] " Prarit Bhargava
2015-10-08 11:48 ` [PATCH] " Prarit Bhargava
2 siblings, 0 replies; 10+ messages in thread
From: Prarit Bhargava @ 2015-08-21 11:53 UTC (permalink / raw)
To: H. Peter Anvin, Ingo Molnar
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, x86, Linus Torvalds,
Andrew Morton, Peter Zijlstra
On 08/21/2015 04:08 AM, H. Peter Anvin wrote:
> Wrong fix, though. Instead we should change it to use the set instruction, which would also make it easier to use the CC_SET/CC_OUT proposed macros to use assembly out in the future.
>
> 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.
>
hpa -- can you send a pointer to that discussion?
Thanks,
P.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 11:50 ` Prarit Bhargava
@ 2015-08-22 9:14 ` Ingo Molnar
0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2015-08-22 9:14 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Linus Torvalds, Andrew Morton, Peter Zijlstra
* Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 08/21/2015 02:51 AM, Ingo Molnar wrote:
> >
> > * Prarit Bhargava <prarit@redhat.com> 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 <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 | 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 8:08 ` H. Peter Anvin
2015-08-21 11:53 ` Prarit Bhargava
@ 2015-08-24 18:22 ` Prarit Bhargava
2015-10-07 23:27 ` Prarit Bhargava
2015-10-08 11:48 ` [PATCH] " Prarit Bhargava
2 siblings, 1 reply; 10+ messages in thread
From: Prarit Bhargava @ 2015-08-24 18:22 UTC (permalink / raw)
To: linux-kernel
Cc: Prarit Bhargava, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
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));
--
1.7.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-24 18:22 ` [PATCH v2] " Prarit Bhargava
@ 2015-10-07 23:27 ` Prarit Bhargava
2015-10-08 8:51 ` Ingo Molnar
0 siblings, 1 reply; 10+ messages in thread
From: Prarit Bhargava @ 2015-10-07 23:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
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));
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-10-07 23:27 ` Prarit Bhargava
@ 2015-10-08 8:51 ` Ingo Molnar
0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2015-10-08 8:51 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match
2015-08-21 8:08 ` H. Peter Anvin
2015-08-21 11:53 ` Prarit Bhargava
2015-08-24 18:22 ` [PATCH v2] " Prarit Bhargava
@ 2015-10-08 11:48 ` Prarit Bhargava
2 siblings, 0 replies; 10+ messages in thread
From: Prarit Bhargava @ 2015-10-08 11:48 UTC (permalink / raw)
To: H. Peter Anvin, Ingo Molnar
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, x86, Linus Torvalds,
Andrew Morton, Peter Zijlstra
On 08/21/2015 04:08 AM, H. Peter Anvin wrote:
> Wrong fix, though. Instead we should change it to use the set instruction, which would also make it easier to use the CC_SET/CC_OUT proposed macros to use assembly out in the future.
>
> 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.
>
hpa, I didn't realize your comment was suggesting a change. I've done a google
search on "gcc bool optimization" (and various incantations of that) and didn't
find anything that lead me in the direction of making a change. Could you
elaborate on what the issue is?
Thanks,
P.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-10-08 11:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-19 17:18 [PATCH] x86, bitops, variable_test_bit should return 1 not -1 on a match Prarit Bhargava
2015-08-21 6:51 ` Ingo Molnar
2015-08-21 8:08 ` H. Peter Anvin
2015-08-21 11:53 ` Prarit Bhargava
2015-08-24 18:22 ` [PATCH v2] " Prarit Bhargava
2015-10-07 23:27 ` Prarit Bhargava
2015-10-08 8:51 ` Ingo Molnar
2015-10-08 11:48 ` [PATCH] " Prarit Bhargava
2015-08-21 11:50 ` Prarit Bhargava
2015-08-22 9:14 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).