linux-kernel - Re: [PATCH 5/9] cpu/SMT: Create topology_smt_thread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875y7v7xp2.ffs@tglx>
Date:   Sun, 11 Jun 2023 00:35:21 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Michael Ellerman <mpe@...erman.id.au>, linux-kernel@...r.kernel.org
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-arch@...r.kernel.org,
        ldufour@...ux.ibm.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        mingo@...hat.com, x86@...nel.org
Subject: Re: [PATCH 5/9] cpu/SMT: Create topology_smt_thread_allowed()

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