Re: [PATCH 1/2] pseries/smp: export the smt level in the SYS FS.

[Michal Suchánek]

Hello,

On Fri, Apr 14, 2023 at 10:11:24PM +1000, Michael Ellerman wrote:
> Laurent Dufour <ldufour@xxxxxxxxxxxxx> writes:
> > On 13/04/2023 15:37:59, Michael Ellerman wrote:
> >> Laurent Dufour <ldufour@xxxxxxxxxxxxx> writes:
> >>> There is no SMT level recorded in the kernel neither in user space.
> >>> Indeed there is no real constraint about that and mixed SMT levels are
> >>> allowed and system is working fine this way.
> >>>
> >>> However when new CPU are added, the kernel is onlining all the threads
> >>> which is leading to mixed SMT levels and confuse end user a bit.
> >>>
> >>> To prevent this exports a SMT level from the kernel so user space
> >>> application like the energy daemon, could read it to adjust their settings.
> >>> There is no action unless recording the value when a SMT value is written
> >>> into the new sysfs entry. User space applications like ppc64_cpu should
> >>> update the sysfs when changing the SMT level to keep the system consistent.
> >>>
> >>> Suggested-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxx>
> >>> ---
> >>>  arch/powerpc/platforms/pseries/pseries.h |  3 ++
> >>>  arch/powerpc/platforms/pseries/smp.c     | 39 ++++++++++++++++++++++++
> >>>  2 files changed, 42 insertions(+)
> >>
> >> There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt
> >>
> >> I think we should be enabling that on powerpc and then adapting it to
> >> our needs, rather than adding a pseries specific file.
> >
> > Thanks Michael, I was not aware of this sysfs interface.
> >
> >> Currently the generic code is only aware of SMT on/off, so it would need
> >> to be taught about SMT4 and 8 at least.
> >
> > Do you think we should limit our support to SMT4 and SMT8 only?
> 
> Possibly? Currently the SMT state is represented by an enum:
> 
> enum cpuhp_smt_control {
> 	CPU_SMT_ENABLED,
> 	CPU_SMT_DISABLED,
> 	CPU_SMT_FORCE_DISABLED,
> 	CPU_SMT_NOT_SUPPORTED,
> 	CPU_SMT_NOT_IMPLEMENTED,
> };
> 
> Adding two states for SMT4 and SMT8 seeems like it might be acceptable.
> 
> On the other hand if we want to support artbitrary SMT values from 3 to
> 8 then it might be better to store that value separately from the state
> enum.
> 
> TBH I'm not sure whether we want to support values other than 1/2/4/8
> via this interface.
> 
> A user who wants some odd numbered SMT value can always configure that
> manually using the existing tools.
> 
> But maybe it's less confusing if this interface supports all values?
> Even if they're unlikely to get much usage.

It looks like ppc64_cpu simply enables first n threads of the CPU where
n is the smt value without any interleaving hoping that the architecture
does the right thing. Under this implementation smt=3 is well-defined.

For the dual cluster P9 CPUs that have two clusters of four this might
work out well for some workloads, and others might want that
interleaving. With that the odd smt values are not well-definedd
anymore.

Nonetheless, if the kernel does support some smt=n parameter whatever
the semantic this should be also supported by the runtime knob.

If it's too difficult to get right there is always that option to not
enable any thread by default, and let the userspace to implement
arbitrarily complex schemes :)

Thanks

Michal