Re: [PATCH v11 3/4] arm64: topology: Support SMT control on ACPI based system

[Pierre Gondois]

On 3/4/25 11:02, Sudeep Holla wrote:
> On Tue, Mar 04, 2025 at 09:25:02AM +0100, Pierre Gondois wrote:
> > On 3/3/25 15:40, Yicong Yang wrote:
> > > On 2025/3/3 19:16, Sudeep Holla wrote:
> > > > On Mon, Mar 03, 2025 at 10:56:12AM +0100, Pierre Gondois wrote:
> > > > > On 2/28/25 20:06, Sudeep Holla wrote:
> > > > > > > > Ditto as previous patch, can get rid if it is default 1.
> > > > > > >
> > > > > > > On non-SMT platforms, not calling cpu_smt_set_num_threads() leaves
> > > > > > > cpu_smt_num_threads uninitialized to UINT_MAX:
> > > > > > >
> > > > > > > smt/active:0
> > > > > > > smt/control:-1
> > > > > > >
> > > > > > > If cpu_smt_set_num_threads() is called:
> > > > > > > active:0
> > > > > > > control:notsupported
> > > > > > >
> > > > > > > So it might be slightly better to still initialize max_smt_thread_num.
> > > > > >
> > > > > > Sure, what I meant is to have max_smt_thread_num set to 1 by default is
> > > > > > that is what needed anyways and the above code does that now.
> > > > > >
> > > > > > Why not start with initialised to 1 instead ?
> > > > > > Of course some current logic needs to change around testing it for zero.
> > > > >
> > > > > I think there would still be a way to check against the default value.
> > > > > If we have:
> > > > > unsigned int max_smt_thread_num = 1;
> > > > >
> > > > > then on a platform with 2 threads, the detection condition would trigger:
> > > > > xa_for_each(&hetero_cpu, hetero_id, entry) {
> > > > >       if (entry->thread_num != max_smt_thread_num && max_smt_thread_num)     <---- (entry->thread_num=2) and (max_smt_thread_num=1)
> > > > >           pr_warn_once("Heterogeneous SMT topology is partly
> > > > >                         supported by SMT control\n");
> > > > >
> > > > > so we would need an additional variable:
> > > > > bool is_initialized = false;
> > > >
> > > > Sure, we could do that or skip the check if max_smt_thread_num == 1 ?
> > > >
> > > > I mean
> > > > 	if (entry->thread_num != max_smt_thread_num && max_smt_thread_num != 1)
> >
> > I think it will be problematic if we parse:
> > - first a CPU with 1 thread
> > - then a CPU with 2 threads
> >
> > in that case we should detect the 'Heterogeneous SMT topology',
> > but we cannot because we don't know whether max_smt_thread_num=1
> > because 1 is the default value or we found a CPU with one thread.
>
> Right, but as per Dietmar's and my previous response, it may be a valid
> case. See latest response from Dietmar which is explicitly requesting
> support for this. It may need some special handling if we decide to support
> that.

Ah ok, right indeed.
For heterogeneous SMT platforms, the 'smt/control' file is able to accept
on/off/forceoff strings. But providing the max #count of threads as an integer would
be wrong if the CPU doesn't have this #count of threads.

Initially the idea was to just warn that support might be needed for heterogeneous
SMT platforms, and let whoever would have such platform solve this case, but just
disabling the integer interface in this case would solve the issue generically.