Re: [PATCH v7 05/15] machine: Improve the error reporting of smp parsing

[wangyanan (Y)]

On 2021/8/23 21:17, Philippe Mathieu-Daudé wrote:
> On 8/23/21 2:27 PM, Yanan Wang wrote:
> > We have two requirements for a valid SMP configuration:
> > the product of "sockets * cores * threads" must represent all the
> > possible cpus, i.e., max_cpus, and then must include the initially
> > present cpus, i.e., smp_cpus.
> >
> > So we only need to ensure 1) "sockets * cores * threads == maxcpus"
> > at first and then ensure 2) "maxcpus >= cpus". With a reasonable
> > order of the sanity check, we can simplify the error reporting code.
> > When reporting an error message we also report the exact value of
> > each topology member to make users easily see what's going on.
> >
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
> > ---
> >   hw/core/machine.c | 22 +++++++++-------------
> >   hw/i386/pc.c      | 24 ++++++++++--------------
> >   2 files changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 85908abc77..093c0d382d 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -779,25 +779,21 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> > *config, Error **errp)
> >       maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
> >       cpus = cpus > 0 ? cpus : maxcpus;
> >   
> > -    if (sockets * cores * threads < cpus) {
> > -        error_setg(errp, "cpu topology: "
> > -                   "sockets (%u) * cores (%u) * threads (%u) < "
> > -                   "smp_cpus (%u)",
> > -                   sockets, cores, threads, cpus);
> > +    if (sockets * cores * threads != maxcpus) {
> > +        error_setg(errp, "Invalid CPU topology: "
> > +                   "product of the hierarchy must match maxcpus: "
> > +                   "sockets (%u) * cores (%u) * threads (%u) "
> > +                   "!= maxcpus (%u)",
> > +                   sockets, cores, threads, maxcpus);
> >           return;
> >       }
> Thinking about scalability, MachineClass could have a
> parse_cpu_topology() handler, and this would be the
> generic one. Principally because architectures don't
> use the same terms, and die/socket/core/thread arrangement
> is machine specific (besides being arch-spec).
> Not a problem as of today, but the way we try to handle
> this generically seems over-engineered to me.
Hi Philippe,

The reason for introducing a generic implementation and avoiding
specific ones is that we thought there is little difference in parsing
logic between the specific parsers. Most part of the parsing is the
automatic calculation of missing values and the related error reporting,
in which the only difference between parsers is the handling of specific
(no matter of arch-specific or machine-specifc) parameters.

So it may be better to keep the parsing logic unified if we can easily
realize that. And actually we can use compat stuff to handle specific
topology parameters well. See implementation in patch #10.

There have been patches on list introducing new specific members
(s390 related in [1] and ARM related in [2]), and in each of them there
is a specific parser needed. However, based on generic one we can
extend without the increasing code duplication.

There is also some discussion about generic/specific parser in [1],
which can be a reference.

[1] 
https://lore.kernel.org/qemu-devel/1626281596-31061-2-git-send-email-pmorel@linux.ibm.com/
[2] 
https://lore.kernel.org/qemu-devel/20210516103228.37792-1-wangyanan55@huawei.com/

Thanks,
Yanan
.
> [unrelated to this particular patch]
>
> .