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

[Pierre Morel <pmorel@linux.ibm.com>]

On 11/7/22 19:04, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
>>
>> 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;
> 
> If topology is only supported for new machines and not the old machines
> for which you set max_threads to a compatibility value (max cpus), then
> you should just ignore the threads, cpus == cores.
> (There might not be any point in keeping a topo->cpus member in this case, I haven't checked)

Right but, I need the nr_cpus in the topology so I prefer to keep it.

However, smp.threads has nothing to do there anymore as you pointed.
I think that nr_cpus should may be named nr_cores and should be set to 
smp.cores.

Thanks,
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen