Re: [PATCH 5/9] cpu/SMT: Create topology_smt_thread_allowed()

[Thomas Gleixner]

On Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
> A subsequent patch will enable partial SMT states, ie. when not all SMT
> threads are brought online.

Nitpick. I stumbled over this 'subsequent patch' theme a couple of times
now because it's very similar to the 'This patch does' phrase.

Just explain what you want to achieve at the end.

>  #else
>  #define topology_max_packages()			(1)
>  static inline int
> @@ -159,6 +160,7 @@ static inline int topology_max_smt_threads(void) { return 1; }
>  static inline bool topology_is_primary_thread(unsigned int cpu) { return true; }
>  static inline bool topology_smt_supported(void) { return false; }
>  static inline bool topology_smt_threads_supported(unsigned int threads) { return false; }
> +static inline bool topology_smt_thread_allowed(unsigned int cpu) { return false; }

Not all these functions need a !SMP stub. Think about the context in
which they are called. There is probably precedence for pointless ones,
but that does not make an argument to add more.

> +/**
> + * topology_smt_thread_allowed - When enabling SMT check whether this particular
> + *				 CPU thread is allowed to be brought online.
> + * @cpu:	CPU to check
> + */
> +bool topology_smt_thread_allowed(unsigned int cpu)
> +{
> +	/*
> +	 * No extra logic s required here to support different thread values
> +	 * because threads will always == 1 or smp_num_siblings because of
> +	 * topology_smt_threads_supported().
> +	 */
> +	return true;
> +}
> +

As x86 only supoorts the on/off model there is no need for this function
if you pick up the CONFIG_SMT_NUM_THREADS_DYNAMIC idea.

You still need something like that for your PPC use case, but that
reduces the overall impact, right?

Thanks,

        tglx