On Mon, May 22, 2017 at 03:13:25PM +0200, Greg Kurz wrote:
> On Mon, 22 May 2017 10:59:50 +0200
> Greg Kurz <gr...@kaod.org> wrote:
>
> > On Mon, 22 May 2017 12:04:13 +1000
> > David Gibson <da...@gibson.dropbear.id.au> wrote:
> >
> > > On Fri, May 19, 2017 at 12:32:20PM +0200, Greg Kurz wrote:
> > > > For historical reasons, we compute CPU device-tree ids with a
> > > > non-trivial
> > > > logic. This patch consolidate the logic in a single helper to be used
> > > > in various places where it is currently open-coded.
> > > >
> > > > It is okay to get rid of DIV_ROUND_UP() because we're sure that the
> > > > number
> > > > of threads per core in the guest cannot exceed the number of threads per
> > > > core in the host.
> > >
> > > However, your new logic still gives different answers in some cases.
> > > In particular when max_cpus is not a multiple of smp_threads. Which
> > > is generally a bad idea, but allowed for older machine types for
> > > compatibility. e.g. smp_threads=4, max_cpus=6 smt=8
> > >
>
> FWIW, this topology was never supported for pseries >= 2.7 since commit
> 94a94e4c4919 ("spapr: convert boot CPUs into CPU core devices", QEMU 2.7):
>
> qemu-system-ppc64: max_cpus (6) must be multiple of threads (4)
>
> The same happens goes with smp_cpus.
Yes, pre-2.7 is what I meant by older machine types.
> If we care for compat with pre-2.7 machine types (ie, no support for CPU
> hotplug),
We do.. RHEL 7.3 is still 2.6 based, for one thing.
> this topology isn't valid anymore since QEMU 2.9, with these
> commits:
>
> 0c86d0fd92aa ("pseries: Always use core objects for CPU construction") which
> causes the following error if we only set max_cpus:
>
> qemu-system-ppc64: This machine version does not support CPU hotplug
That patch has explicit provision for allowing the last core to have a
not-full complement of threads.
> 8149e2992f78 ("pseries: Enforce homogeneous threads-per-core") which
> causes the following error if we set smp_cpus (or smp_cpus == max_cpus):
>
> qemu-system-ppc64: invalid nr-threads 2, must be 4
This one does indeed do what you say - but that's a bug :(. It means
migration from older versions may be broken.
> So in the end, we already enforce max_cpus and smp_cpus to be multiple
> of smp_threads for all machine types. In this case...
>
> > > Old logic:
> > > DIV_ROUND_UP(6 * 8, 4)
> > > = ⌈48 / 4⌉ = 12
> > >
> > > New logic gives: ⌊6 / 4⌋ * 8 + (6 % 4)
> > > = 1 * 8 + 2
> > > = 10
> > >
> >
> > I now realize this two formulas are hardly reconcilable... this
> > probably means that this patch shouldn't try to consolidate the
> > logic in hw/ppc/spapr.c with the one in target/ppc/translate_init.c.
>
> ... both formulas are equivalent, unless I'm missing something. Or
> do we really want to re-allow the funky topologies for older
> machines ?
Want to? Not really. Have to for compatibility? Yes, absolutely.
I've just sent a patch to address this.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
