"ht" flag indicates only ability to detect siblings not HT presence itself.
Inspired by:
http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
It could be that there are some user-space programs depending on "ht"
flag so the patch is marked as RFC..
arch/x86/include/asm/cpufeature.h | 4 ++--
arch/x86/kernel/cpu/capflags.c | 2 +-
arch/x86/kernel/cpu/common.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
Index: b/arch/x86/include/asm/cpufeature.h
===================================================================
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -42,7 +42,7 @@
#define X86_FEATURE_XMM (0*32+25) /* "sse" */
#define X86_FEATURE_XMM2 (0*32+26) /* "sse2" */
#define X86_FEATURE_SELFSNOOP (0*32+27) /* "ss" CPU self snoop */
-#define X86_FEATURE_HT (0*32+28) /* Hyper-Threading */
+#define X86_FEATURE_HT_DETECTION (0*32+28) /* Hyper-Threading detection */
#define X86_FEATURE_ACC (0*32+29) /* "tm" Automatic clock control */
#define X86_FEATURE_IA64 (0*32+30) /* IA-64 processor */
#define X86_FEATURE_PBE (0*32+31) /* Pending Break Enable */
@@ -220,7 +220,7 @@ extern const char * const x86_power_flag
#define cpu_has_xmm2 boot_cpu_has(X86_FEATURE_XMM2)
#define cpu_has_xmm3 boot_cpu_has(X86_FEATURE_XMM3)
#define cpu_has_aes boot_cpu_has(X86_FEATURE_AES)
-#define cpu_has_ht boot_cpu_has(X86_FEATURE_HT)
+#define cpu_has_ht_detection boot_cpu_has(X86_FEATURE_HT_DETECTION)
#define cpu_has_mp boot_cpu_has(X86_FEATURE_MP)
#define cpu_has_nx boot_cpu_has(X86_FEATURE_NX)
#define cpu_has_k6_mtrr boot_cpu_has(X86_FEATURE_K6_MTRR)
Index: b/arch/x86/kernel/cpu/capflags.c
===================================================================
--- a/arch/x86/kernel/cpu/capflags.c
+++ b/arch/x86/kernel/cpu/capflags.c
@@ -27,7 +27,7 @@ const char * const x86_cap_flags[NCAPINT
[X86_FEATURE_XMM] = "sse",
[X86_FEATURE_XMM2] = "sse2",
[X86_FEATURE_SELFSNOOP] = "ss",
- [X86_FEATURE_HT] = "ht",
+ [X86_FEATURE_HT_DETECTION] = "ht_detection",
[X86_FEATURE_ACC] = "tm",
[X86_FEATURE_IA64] = "ia64",
[X86_FEATURE_PBE] = "pbe",
Index: b/arch/x86/kernel/cpu/common.c
===================================================================
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -433,7 +433,7 @@ void __cpuinit detect_ht(struct cpuinfo_
u32 eax, ebx, ecx, edx;
int index_msb, core_bits;
- if (!cpu_has(c, X86_FEATURE_HT))
+ if (!cpu_has(c, X86_FEATURE_HT_DETECTION))
return;
if (cpu_has(c, X86_FEATURE_CMP_LEGACY))
On 11/11/2009 02:34 PM, Bartlomiej Zolnierkiewicz wrote:
>
> "ht" flag indicates only ability to detect siblings not HT presence itself.
>
> Inspired by:
> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> ---
> It could be that there are some user-space programs depending on "ht"
> flag so the patch is marked as RFC..
If a cpu is capable of ht but doesn't have any siblings, do we actually
care?
If many people consider the "ht" flag to indicate that hyperthreading is
actually available, what about instead changing the meaning of the "ht"
flag to indicate that there actually are siblings?
Chris
On 11/11/2009 03:34 PM, Bartlomiej Zolnierkiewicz wrote:
>
> "ht" flag indicates only ability to detect siblings not HT presence itself.
>
> Inspired by:
> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
>
> Signed-off-by: Bartlomiej Zolnierkiewicz<[email protected]>
> ---
> It could be that there are some user-space programs depending on "ht"
> flag so the patch is marked as RFC..
I think that is the only problem with this patch -- it is an ABI change.
See as it's been this way for years, a comment is probably the best
you can do...
Jeff
* Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> "ht" flag indicates only ability to detect siblings not HT presence
> itself.
>
> Inspired by:
> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
i think instead of changing 'ht' to 'ht_detect', it would be even more
intuitive to only expose the 'ht' flag if the ht-detect capability is
there _and_ if the number of siblings is 2 or more.
That way we dont change it - we just 'hide' the 'ht' string on the
category of systems that can enumerate HT via the CPUID but dont
actually have HyperThreading.
Hm?
Ingo
On 11/11/2009 10:59 PM, Ingo Molnar wrote:
>
> * Bartlomiej Zolnierkiewicz <[email protected]> wrote:
>
>> "ht" flag indicates only ability to detect siblings not HT presence
>> itself.
>>
>> Inspired by:
>> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
>
> i think instead of changing 'ht' to 'ht_detect', it would be even more
> intuitive to only expose the 'ht' flag if the ht-detect capability is
> there _and_ if the number of siblings is 2 or more.
>
> That way we dont change it - we just 'hide' the 'ht' string on the
> category of systems that can enumerate HT via the CPUID but dont
> actually have HyperThreading.
>
/proc/cpuinfo is a user-space visible ABI. Changing it is bad chicken.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
* H. Peter Anvin <[email protected]> wrote:
> On 11/11/2009 10:59 PM, Ingo Molnar wrote:
> >
> > * Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> >
> >> "ht" flag indicates only ability to detect siblings not HT presence
> >> itself.
> >>
> >> Inspired by:
> >> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
> >
> > i think instead of changing 'ht' to 'ht_detect', it would be even
> > more intuitive to only expose the 'ht' flag if the ht-detect
> > capability is there _and_ if the number of siblings is 2 or more.
> >
> > That way we dont change it - we just 'hide' the 'ht' string on the
> > category of systems that can enumerate HT via the CPUID but dont
> > actually have HyperThreading.
>
> /proc/cpuinfo is a user-space visible ABI. Changing it is bad
> chicken.
Almost ... something is only an ABI if an actual application relies on
the 'ht' flag being there on non-hyperthreading CPUs. We dont know
whether there's any, but the likelyhood looks pretty low. Could park it
a branch for some time to see whether anything trips up.
Ingo
On 11/12/2009 12:13 AM, Ingo Molnar wrote:
>>
>> /proc/cpuinfo is a user-space visible ABI. Changing it is bad
>> chicken.
>
> Almost ... something is only an ABI if an actual application relies on
> the 'ht' flag being there on non-hyperthreading CPUs. We dont know
> whether there's any, but the likelyhood looks pretty low. Could park it
> a branch for some time to see whether anything trips up.
>
No, please. We have had people who have "helpfully" tried to change
/proc/cpuinfo in the past, and what we find is that application
developers don't tell us -- they grumble among themselves and put out a
new release saying "fixes breakage introduced by Linux ..."
It's an ABI. Keep it stable, please.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
Chris Friesen <[email protected]> wrote:
>>
>> "ht" flag indicates only ability to detect siblings not HT presence itself.
>>
>> Inspired by:
>> http://www.codemonkey.org.uk/2009/11/10/common-hyperthreading-misconception/
>>
>> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
>> ---
>> It could be that there are some user-space programs depending on "ht"
>> flag so the patch is marked as RFC..
>
> If a cpu is capable of ht but doesn't have any siblings, do we actually
> care?
>
I have a Core2 Duo U7600 with siblings set to 2 and the 'ht' flag and I
know the CPU in my lapdog does not actually support hyperthreading.
> If many people consider the "ht" flag to indicate that hyperthreading is
> actually available, what about instead changing the meaning of the "ht"
> flag to indicate that there actually are siblings?
>
Could be determined surely by "grep siblings /proc/cpuinfo"?
Cheers
--
Alexander Clouter
.sigmonster says: Old musicians never die, they just decompose.
* H. Peter Anvin <[email protected]> wrote:
> On 11/12/2009 12:13 AM, Ingo Molnar wrote:
> >>
> >> /proc/cpuinfo is a user-space visible ABI. Changing it is bad
> >> chicken.
> >
> > Almost ... something is only an ABI if an actual application relies on
> > the 'ht' flag being there on non-hyperthreading CPUs. We dont know
> > whether there's any, but the likelyhood looks pretty low. Could park it
> > a branch for some time to see whether anything trips up.
> >
>
> No, please. We have had people who have "helpfully" tried to change
> /proc/cpuinfo in the past, and what we find is that application
> developers don't tell us -- they grumble among themselves and put out
> a new release saying "fixes breakage introduced by Linux ..."
>
> It's an ABI. Keep it stable, please.
That's generally true, but i'm not suggesting that: i'm suggesting to
_clear_ the HT flag from the cpufeatures if there's only one sibling.
It's meaningless in that case and as the link quoted by the original
patch shows many people are confused by that.
I have such a box so i can test it. (but i dont expect any problems)
Ingo
On Thu, Nov 12, 2009 at 06:59:08PM +0100, Ingo Molnar wrote:
> > It's an ABI. Keep it stable, please.
>
> That's generally true, but i'm not suggesting that: i'm suggesting to
> _clear_ the HT flag from the cpufeatures if there's only one sibling.
> It's meaningless in that case and as the link quoted by the original
> patch shows many people are confused by that.
>
> I have such a box so i can test it. (but i dont expect any problems)
I agree that it's an ABI change, but any software depending on its current
state has to implement a fallback for the case where 'ht' isn't present anyway
unless there's some program that only runs on ht capable hardware, which
sounds just crazy.
The only potential for breakage that I can see is that code that is tuned
to be run in the HT case will stop running in cases where it shouldn't.
Which sounds like a positive thing to me.
Dave
--
http://www.codemonkey.org.uk
On 11/12/2009 10:37 AM, Dave Jones wrote:
> On Thu, Nov 12, 2009 at 06:59:08PM +0100, Ingo Molnar wrote:
> > > It's an ABI. Keep it stable, please.
> >
> > That's generally true, but i'm not suggesting that: i'm suggesting to
> > _clear_ the HT flag from the cpufeatures if there's only one sibling.
> > It's meaningless in that case and as the link quoted by the original
> > patch shows many people are confused by that.
> >
> > I have such a box so i can test it. (but i dont expect any problems)
>
> I agree that it's an ABI change, but any software depending on its current
> state has to implement a fallback for the case where 'ht' isn't present anyway
> unless there's some program that only runs on ht capable hardware, which
> sounds just crazy.
>
> The only potential for breakage that I can see is that code that is tuned
> to be run in the HT case will stop running in cases where it shouldn't.
> Which sounds like a positive thing to me.
The most likely breakage would be some stupid licensing scheme.
The other aspect is that we as much as possible have tried to stay to
the hardware-documented names of these strings. Inventing new strings
is generally a bad idea.
-hpa
On 11/12/2009 09:59 AM, Ingo Molnar wrote:
>>
>> No, please. We have had people who have "helpfully" tried to change
>> /proc/cpuinfo in the past, and what we find is that application
>> developers don't tell us -- they grumble among themselves and put out
>> a new release saying "fixes breakage introduced by Linux ..."
>>
>> It's an ABI. Keep it stable, please.
>
> That's generally true, but i'm not suggesting that: i'm suggesting to
> _clear_ the HT flag from the cpufeatures if there's only one sibling.
> It's meaningless in that case and as the link quoted by the original
> patch shows many people are confused by that.
>
> I have such a box so i can test it. (but i dont expect any problems)
>
Ah, okay, that makes more sense. Someone else in the tread wanted to
rename the flag.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
* H. Peter Anvin <[email protected]> wrote:
> On 11/12/2009 10:37 AM, Dave Jones wrote:
> > On Thu, Nov 12, 2009 at 06:59:08PM +0100, Ingo Molnar wrote:
> > > > It's an ABI. Keep it stable, please.
> > >
> > > That's generally true, but i'm not suggesting that: i'm suggesting to
> > > _clear_ the HT flag from the cpufeatures if there's only one sibling.
> > > It's meaningless in that case and as the link quoted by the original
> > > patch shows many people are confused by that.
> > >
> > > I have such a box so i can test it. (but i dont expect any problems)
> >
> > I agree that it's an ABI change, but any software depending on its current
> > state has to implement a fallback for the case where 'ht' isn't present anyway
> > unless there's some program that only runs on ht capable hardware, which
> > sounds just crazy.
> >
> > The only potential for breakage that I can see is that code that is tuned
> > to be run in the HT case will stop running in cases where it shouldn't.
> > Which sounds like a positive thing to me.
>
> The most likely breakage would be some stupid licensing scheme.
>
> The other aspect is that we as much as possible have tried to stay to
> the hardware-documented names of these strings. Inventing new strings
> is generally a bad idea.
Agreed - and we rejected such patches a couple of times in the past and
for good reasons. Some /proc details are rarely used by apps (so they
are no real ABIs) but cpuinfo is frequently parsed.
Clearing the ht flag on non-hyperthreading CPUs would be a limited
quirk/fix in essence applicable to a relatively narrow range of CPUs -
and easily undone, should it cause any problems. So if Bart wants to
take a stab at that it would be a nice solution to the problem at hand
...
Ingo
On Friday 13 November 2009 08:42:48 Ingo Molnar wrote:
>
> * H. Peter Anvin <[email protected]> wrote:
>
> > On 11/12/2009 10:37 AM, Dave Jones wrote:
> > > On Thu, Nov 12, 2009 at 06:59:08PM +0100, Ingo Molnar wrote:
> > > > > It's an ABI. Keep it stable, please.
> > > >
> > > > That's generally true, but i'm not suggesting that: i'm suggesting to
> > > > _clear_ the HT flag from the cpufeatures if there's only one sibling.
> > > > It's meaningless in that case and as the link quoted by the original
> > > > patch shows many people are confused by that.
> > > >
> > > > I have such a box so i can test it. (but i dont expect any problems)
> > >
> > > I agree that it's an ABI change, but any software depending on its current
> > > state has to implement a fallback for the case where 'ht' isn't present anyway
> > > unless there's some program that only runs on ht capable hardware, which
> > > sounds just crazy.
> > >
> > > The only potential for breakage that I can see is that code that is tuned
> > > to be run in the HT case will stop running in cases where it shouldn't.
> > > Which sounds like a positive thing to me.
> >
> > The most likely breakage would be some stupid licensing scheme.
> >
> > The other aspect is that we as much as possible have tried to stay to
> > the hardware-documented names of these strings. Inventing new strings
> > is generally a bad idea.
>
> Agreed - and we rejected such patches a couple of times in the past and
> for good reasons. Some /proc details are rarely used by apps (so they
> are no real ABIs) but cpuinfo is frequently parsed.
>
> Clearing the ht flag on non-hyperthreading CPUs would be a limited
> quirk/fix in essence applicable to a relatively narrow range of CPUs -
> and easily undone, should it cause any problems. So if Bart wants to
> take a stab at that it would be a nice solution to the problem at hand
> ...
I'm rather busy with other/real stuff so if anybody wants to beat me
to making the proper quirk just feel free to do it.
--
Bartlomiej Zolnierkiewicz