Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

Pierre Morel
2022-10-28T09:30:45Z

On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>>
>> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-12 at 18:20 +0200, Pierre Morel wrote:
>>>> In the S390x CPU topology the core_id specifies the CPU address
>>>> and the position of the core withing the topology.
>>>>
>>>> Let's build the topology based on the core_id.
>>>> s390x/cpu topology: core_id sets s390x CPU topology
>>>>
>>>> In the S390x CPU topology the core_id specifies the CPU address
>>>> and the position of the cpu withing the topology.
>>>>
>>>> Let's build the topology based on the core_id.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>> include/hw/s390x/cpu-topology.h | 45 +++++++++++
>>>> hw/s390x/cpu-topology.c | 132 ++++++++++++++++++++++++++++++++
>>>> hw/s390x/s390-virtio-ccw.c | 21 +++++
>>>> hw/s390x/meson.build | 1 +
>>>> 4 files changed, 199 insertions(+)
>>>> create mode 100644 include/hw/s390x/cpu-topology.h
>>>> create mode 100644 hw/s390x/cpu-topology.c
>>>>
>>> [...]
>>>
>>>> +/**
>>>> + * s390_topology_realize:
>>>> + * @dev: the device state
>>>> + * @errp: the error pointer (not used)
>>>> + *
>>>> + * During realize the machine CPU topology is initialized with the
>>>> + * QEMU -smp parameters.
>>>> + * The maximum count of CPU TLE in the all Topology can not be greater
>>>> + * than the maximum CPUs.
>>>> + */
>>>> +static void s390_topology_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> + MachineState *ms = MACHINE(qdev_get_machine());
>>>> + S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>>>> +
>>>> + topo->cpus = ms->smp.cores * ms->smp.threads;
>>>
>>> Currently threads are not supported, effectively increasing the number of cpus,
>>> so this is currently correct. Once the machine version limits the threads to 1,
>>> it is also correct. However, once we support multiple threads, this becomes incorrect.
>>> I wonder if it's ok from a backward compatibility point of view to modify the smp values
>>> by doing cores *= threads, threads = 1 for old machines.
>>
>> Right, this will become incorrect with thread support.
>> What about having a dedicated function:
>>
>> topo->cpus = s390_get_cpus(ms);
>>
>> This function will use the S390CcwMachineClass->max_thread introduced
>> later to report the correct number of CPUs.
>
> I don't think max_threads is exactly what matters here, it's if
> threads are supported or not or, if max_threads == 1 it doesn't matter.
> The question is how best to do the check. You could check the machine version.
> I wonder if you could add a feature bit for the multithreading facility that is
> always false and use that.
>
> I don't know if using a function makes a difference, that is if it is obvious on
> introduction of multithreading support that the function needs to be updated.
> (If it is implemented in a way that requires updating, if you check the machine
> version it doesn't)
> In any case, the name you suggested isn't very descriptive.

I think we care about this machine and olders.
Olders do not support topology so this, Multithreading (MT) does not mater.
This machine support topology, if I follow Cedric advise, the
"max_thread" will/may be introduce before the topology.

This in fact is not an implementation for MT or does not allow the
implementation of MT it is only a way to get rid of the false
information given to the user that we accept MT.

So I think that when we introduce MT we will take care of making things
right at this place as in other places of the code.

What about we keep the original:

topo->cpus = ms->smp.cores * ms->smp.threads;

Which does not do any arm to machines without MT ?

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen