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

[Cornelia Huck]

(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'.

>
> Regards,
> Pierre
>
>
>
>> 
>> And then have only a single smp_parse impl that takes into account
>> these constraints, to report errors / fill in missing fields / etc ?
>> 
>> Regards,
>> Daniel
>> 
>
> -- 
> Pierre Morel
> IBM Lab Boeblingen