| Message ID | 20220812164144.30829-8-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | x86/topology: Improve CPUID.1F handling | expand |
* Zhang Rui <rui.zhang@intel.com> wrote: > smp_num_siblings can be larger than 2. > > Any value larger than 1 suggests HT is supported. > > Reviewed-by: Len Brown <len.brown@intel.com> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > arch/x86/include/asm/perf_event_p4.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h > index 94de1a05aeba..b14e9a20a7c0 100644 > --- a/arch/x86/include/asm/perf_event_p4.h > +++ b/arch/x86/include/asm/perf_event_p4.h > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void) > static inline int p4_ht_thread(int cpu) > { > #ifdef CONFIG_SMP > - if (smp_num_siblings == 2) > + if (smp_num_siblings > 1) > return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map)); This fix too should probably come before all the other changes. (Not that Pentium 4 code is expected to ever see such SMT thread values.) Thanks, Ingo
Hi, Ingo, On Sat, 2022-08-13 at 12:50 +0200, Ingo Molnar wrote: > > * Zhang Rui <rui.zhang@intel.com> wrote: > > > smp_num_siblings can be larger than 2. > > > > Any value larger than 1 suggests HT is supported. > > > > Reviewed-by: Len Brown <len.brown@intel.com> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > arch/x86/include/asm/perf_event_p4.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/perf_event_p4.h > > b/arch/x86/include/asm/perf_event_p4.h > > index 94de1a05aeba..b14e9a20a7c0 100644 > > --- a/arch/x86/include/asm/perf_event_p4.h > > +++ b/arch/x86/include/asm/perf_event_p4.h > > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void) > > static inline int p4_ht_thread(int cpu) > > { > > #ifdef CONFIG_SMP > > - if (smp_num_siblings == 2) > > + if (smp_num_siblings > 1) > > return cpu != > > cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map)); > > This fix too should probably come before all the other changes. > > (Not that Pentium 4 code is expected to ever see such SMT thread > values.) > Do you mean that this is a clean fix, and there is no reason for this patch to be blocked by any of the other patches in this series? thanks, rui
On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote: > smp_num_siblings can be larger than 2. Not on a P4 it can't ;-) > > Any value larger than 1 suggests HT is supported. > > Reviewed-by: Len Brown <len.brown@intel.com> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > arch/x86/include/asm/perf_event_p4.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h > index 94de1a05aeba..b14e9a20a7c0 100644 > --- a/arch/x86/include/asm/perf_event_p4.h > +++ b/arch/x86/include/asm/perf_event_p4.h > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void) > static inline int p4_ht_thread(int cpu) > { > #ifdef CONFIG_SMP > - if (smp_num_siblings == 2) > + if (smp_num_siblings > 1) > return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map)); > #endif > return 0; Unless Intel plans to respin an P4 with extra siblings on, I don't think this qualifies for the word 'fix'.
On Mon, 2022-08-15 at 11:11 +0200, Peter Zijlstra wrote: > On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote: > > smp_num_siblings can be larger than 2. > > Not on a P4 it can't ;-) Kernel code doesn't prevent this from happening, so it just depends on how SMT ID is encoded in APICID. Checking for smp_num_sibling > 1 is the right logic to detect HT support, which is followed by all other kernel code except this one. :) thanks, rui > > > Any value larger than 1 suggests HT is supported. > > > > Reviewed-by: Len Brown <len.brown@intel.com> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > arch/x86/include/asm/perf_event_p4.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/perf_event_p4.h > > b/arch/x86/include/asm/perf_event_p4.h > > index 94de1a05aeba..b14e9a20a7c0 100644 > > --- a/arch/x86/include/asm/perf_event_p4.h > > +++ b/arch/x86/include/asm/perf_event_p4.h > > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void) > > static inline int p4_ht_thread(int cpu) > > { > > #ifdef CONFIG_SMP > > - if (smp_num_siblings == 2) > > + if (smp_num_siblings > 1) > > return cpu != > > cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map)); > > #endif > > return 0; > > Unless Intel plans to respin an P4 with extra siblings on, I don't > think > this qualifies for the word 'fix'.
On Tue, Aug 16, 2022 at 10:26:19AM +0800, Zhang Rui wrote: > On Mon, 2022-08-15 at 11:11 +0200, Peter Zijlstra wrote: > > On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote: > > > smp_num_siblings can be larger than 2. > > > > Not on a P4 it can't ;-) > > Kernel code doesn't prevent this from happening, so it just depends on > how SMT ID is encoded in APICID. No P4 ever encountered had it different and since no P4 will ever be made again (I sincerely hope) we have a complete case. > Checking for smp_num_sibling > 1 is the right logic to detect HT > support, which is followed by all other kernel code except this one. :) I'm fine with making it consistent, I'm arguing with the subject calling this a fix, it is not, it's a functional no-op.
diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h index 94de1a05aeba..b14e9a20a7c0 100644 --- a/arch/x86/include/asm/perf_event_p4.h +++ b/arch/x86/include/asm/perf_event_p4.h @@ -189,7 +189,7 @@ static inline int p4_ht_active(void) static inline int p4_ht_thread(int cpu) { #ifdef CONFIG_SMP - if (smp_num_siblings == 2) + if (smp_num_siblings > 1) return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map)); #endif return 0;