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

[Daniel P . Berrangé]

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.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|