Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

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

On 7/19/21 5:52 PM, Daniel P. Berrangé wrote:
> On Mon, Jul 19, 2021 at 05:43:29PM +0200, Cornelia Huck wrote:
>> (restored cc:s)
>>
>> On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
>>>> I increasingly worry that we're making a mistake by going down the
>>>> route of having custom smp_parse implementations per target, as this
>>>> is showing signs of inconsistent behaviour and error reportings. I
>>>> think the differences / restrictions have granularity at a different
>>>> level that is being tested in many cases too.
>>>>
>>>> Whether threads != 1 is valid will likely vary depending on what
>>>> CPU model is chosen, rather than what architecture is chosen.
>>>> The same is true for dies != 1. We're not really checking this
>>>> closely even in x86 - for example I can request nonsense such
>>>> as a 25 year old i486 CPU model with hyperthreading and multiple
>>>> dies
>>>>
>>>>     qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2
>>
>> Now that's what I'd call an upgrade :)
>>
>>>>
>>>> In this patch, there is no error reporting if the user specifies
>>>> dies != 1 or threads != 1 - it just silently ignores the request
>>>> which is not good.
>>>
>>> yes, I should change this
>>>
>>>>
>>>> Some machine types may have constraints on CPU sockets.
>>>>
>>>> This can of course all be handled by custom smp_parse impls, but
>>>> this is ultimately going to lead to alot of duplicated and
>>>> inconsistent logic I fear.
>>>>
>>>> I wonder if we would be better off having machine class callback
>>>> that can report topology constraints for the current configuration,
>>>> along lines ofsmp_constraints(MachineState *ms,
>>>>
>>>>        smp_constraints(MachineState *ms,
>>>>                        int *max_sockets,
>>>>                        int *max_dies,
>>>>                        int *max_cores,
>>>>                        int *max_threads)
>>>
>>> I find the idee good, but what about making it really machine agnostic
>>> by removing names and using a generic
>>>
>>> 	smp_constraints(MachineState *ms,
>>> 			int *nb_levels,
>>> 			int *levels[]
>>> 			);
>>>
>>> Level can be replaced by another name like container.
>>> The machine could also provide the level/container names according to
>>> its internal documentation.
>>
>> In theory, this could give us more flexibility; however, wouldn't
>> that still mean that the core needs to have some knowledge of the
>> individual levels? We also have the command line parsing to consider,
>> and that one uses concrete names (which may or may not make sense,
>> depending on what machine you are trying to configure), and we'd still
>> have to map these to 'levels'.
> 
> Yeah, we need to deal with names in several places, so I don't think
> abstracting it in one place is desirable, as it introduces the need
> to convert between the two and potentially obscures the semantics.
> 

Converting with names looks possible to me, every architecture can 
export a topology_name array or structure indicating names and other 
informations like the maximum possible count of entries at this level.

We have now the SMPConfiguration, couldn't we use it for this?

Regards,
Pierre

> 
> Regards,
> Daniel
> 

-- 
Pierre Morel
IBM Lab Boeblingen