* Prarit Bhargava <pra...@redhat.com> wrote:
> Commit bbb65d2d365e ("x86: use cpuid vector 0xb when available for
> detecting cpu topology") changed the value of smp_num_siblings from the
> active number of threads in a core to the maximum number threads in a
> core. e.g.) On Intel Haswell and newer systems smp_num_siblings is
> two even if SMT is disabled.
>
> topology_max_smt_threads() already returns the active number of threads.
> Introduce topology_hw_smt_threads() which returns the maximum number of
> threads. These are used to fix and replace references to smp_num_siblings.
It's unclear to the reader of this changelog what the patch does:
- is it a cleanup?
- does it introduce some new facility to be used in a later patch?
- ... or does it fix a real bug?
> diff --git a/arch/x86/include/asm/perf_event_p4.h
> b/arch/x86/include/asm/perf_event_p4.h
> index 94de1a05aeba..11afdadce9c2 100644
> --- a/arch/x86/include/asm/perf_event_p4.h
> +++ b/arch/x86/include/asm/perf_event_p4.h
> @@ -181,7 +181,7 @@ static inline u64 p4_clear_ht_bit(u64 config)
> static inline int p4_ht_active(void)
> {
> #ifdef CONFIG_SMP
> - return smp_num_siblings > 1;
> + return topology_max_smt_threads() > 1;
> #endif
> return 0;
> }
> @@ -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 (topology_max_smt_threads() == 2)
> return cpu !=
> cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
This appears to change functionality - so I guess it fixes a real bug?
> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> index c1d2a9892352..b5ff1c784eef 100644
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -116,16 +116,16 @@ extern unsigned int __max_logical_packages;
> #define topology_max_packages() (__max_logical_packages)
>
> extern int __max_smt_threads;
> -
> -static inline int topology_max_smt_threads(void)
> -{
> - return __max_smt_threads;
> -}
> +#define topology_max_smt_threads() (__max_smt_threads)
> +extern int __hw_smt_threads;
> +#define topology_hw_smt_threads() (__hw_smt_threads)
I general it's better to use C inline functions than CPP defines.
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -332,16 +332,14 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
> cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>
> node_id = ecx & 0xff;
> - smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
> + __hw_smt_threads = ((ebx >> 8) & 0xff) + 1;
>
> if (c->x86 == 0x15)
> c->cu_id = ebx & 0xff;
>
> if (c->x86 >= 0x17) {
> c->cpu_core_id = ebx & 0xff;
> -
> - if (smp_num_siblings > 1)
> - c->x86_max_cores /= smp_num_siblings;
> + c->x86_max_cores /= topology_hw_smt_threads();
> }
> @@ -1228,6 +1228,10 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> /* Init Machine Check Exception if available. */
> mcheck_cpu_init(c);
>
> + /* Must be called before select_idle_routine */
> + if (c != &boot_cpu_data)
> + set_cpu_sibling_map(raw_smp_processor_id());
> +
> select_idle_routine(c);
This appears to be an unexplained change.
> --- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
> @@ -420,7 +420,8 @@ static u32 get_nbc_for_node(int node_id)
> struct cpuinfo_x86 *c = &boot_cpu_data;
> u32 cores_per_node;
>
> - cores_per_node = (c->x86_max_cores * smp_num_siblings) /
> amd_get_nodes_per_socket();
> + cores_per_node = (c->x86_max_cores * topology_hw_smt_threads()) /
> + amd_get_nodes_per_socket();
Please don't break lines that are just slightly over col80.
So all of this looks pretty complex, but is poorly explained. Please split it
up
into a series of well-explained patches where each patch does one specific
thing.
The cleanup and renaming patches should come first, the bug fixing patch(es)
should come after them.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
