When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
initialize 4 cpus. Compute the chip per cores depending on the number of chips
and smt threads.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/pnv.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 9724719..3fbaafb 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -642,7 +642,7 @@ static void ppc_powernv_init(MachineState *machine)
     MemoryRegion *ram;
     char *fw_filename;
     long fw_size;
-    int i;
+    int i, cores_per_chip;
     char *chip_typename;
     PCIBus *pbus;
     bool has_gfx = false;
@@ -710,6 +710,22 @@ static void ppc_powernv_init(MachineState *machine)
     }
 
     pnv->chips = g_new0(PnvChip *, pnv->num_chips);
+
+    /* If user has specified number of cores, use it. Otherwise, compute it. */
+    if (smp_cores != 1) {
+        cores_per_chip = smp_cores;
+    } else {
+        cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
+    }
+
+    if (smp_cpus != (smp_threads * pnv->num_chips * cores_per_chip)) {
+        error_report("cpu topology not balanced: "
+                     "chips (%u) * cores (%u) * threads (%u) != "
+                     "number of cpus (%u)",
+                     pnv->num_chips, cores_per_chip, smp_threads, smp_cpus);
+        exit(1);
+    }
+
     for (i = 0; i < pnv->num_chips; i++) {
         char chip_name[32];
         Object *chip = object_new(chip_typename);
@@ -728,7 +744,7 @@ static void ppc_powernv_init(MachineState *machine)
         object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal);
         object_property_set_int(chip, PNV_CHIP_HWID(i), "chip-id",
                                 &error_fatal);
-        object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
+        object_property_set_int(chip, cores_per_chip, "nr-cores", &error_fatal);
         object_property_set_int(chip, 1, "num-phbs", &error_fatal);
         object_property_set_bool(chip, true, "realized", &error_fatal);
     }
-- 
2.9.3

On 09/06/2017 10:27 AM, Nikunj A Dadhania wrote:
> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
> initialize 4 cpus. Compute the chip per cores depending on the number of chips
> and smt threads.

I think we could also use the '-numa' options to define the cpus
per chip but this patch defines a good default layout and fixes 
an issue.  

> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

I pushed the patch under the latest powernv tree :

	https://github.com/legoater/qemu.git tags/powernv-2.10

Thanks,

C.

> ---
>  hw/ppc/pnv.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9724719..3fbaafb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -642,7 +642,7 @@ static void ppc_powernv_init(MachineState *machine)
>      MemoryRegion *ram;
>      char *fw_filename;
>      long fw_size;
> -    int i;
> +    int i, cores_per_chip;
>      char *chip_typename;
>      PCIBus *pbus;
>      bool has_gfx = false;
> @@ -710,6 +710,22 @@ static void ppc_powernv_init(MachineState *machine)
>      }
>  
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> +
> +    /* If user has specified number of cores, use it. Otherwise, compute it. */
> +    if (smp_cores != 1) {
> +        cores_per_chip = smp_cores;
> +    } else {
> +        cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> +    }
> +
> +    if (smp_cpus != (smp_threads * pnv->num_chips * cores_per_chip)) {
> +        error_report("cpu topology not balanced: "
> +                     "chips (%u) * cores (%u) * threads (%u) != "
> +                     "number of cpus (%u)",
> +                     pnv->num_chips, cores_per_chip, smp_threads, smp_cpus);
> +        exit(1);
> +    }
> +
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
>          Object *chip = object_new(chip_typename);
> @@ -728,7 +744,7 @@ static void ppc_powernv_init(MachineState *machine)
>          object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal);
>          object_property_set_int(chip, PNV_CHIP_HWID(i), "chip-id",
>                                  &error_fatal);
> -        object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> +        object_property_set_int(chip, cores_per_chip, "nr-cores", &error_fatal);
>          object_property_set_int(chip, 1, "num-phbs", &error_fatal);
>          object_property_set_bool(chip, true, "realized", &error_fatal);
>      }
> 

On Wed, Sep 06, 2017 at 01:57:48PM +0530, Nikunj A Dadhania wrote:
> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
> initialize 4 cpus. Compute the chip per cores depending on the number of chips
> and smt threads.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

I don't understand why simply treating smp_cores as cores per chip is wrong.

> ---
>  hw/ppc/pnv.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9724719..3fbaafb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -642,7 +642,7 @@ static void ppc_powernv_init(MachineState *machine)
>      MemoryRegion *ram;
>      char *fw_filename;
>      long fw_size;
> -    int i;
> +    int i, cores_per_chip;
>      char *chip_typename;
>      PCIBus *pbus;
>      bool has_gfx = false;
> @@ -710,6 +710,22 @@ static void ppc_powernv_init(MachineState *machine)
>      }
>  
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> +
> +    /* If user has specified number of cores, use it. Otherwise, compute it. */
> +    if (smp_cores != 1) {
> +        cores_per_chip = smp_cores;
> +    } else {
> +        cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> +    }
> +
> +    if (smp_cpus != (smp_threads * pnv->num_chips * cores_per_chip)) {
> +        error_report("cpu topology not balanced: "
> +                     "chips (%u) * cores (%u) * threads (%u) != "
> +                     "number of cpus (%u)",
> +                     pnv->num_chips, cores_per_chip, smp_threads, smp_cpus);
> +        exit(1);
> +    }
> +
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
>          Object *chip = object_new(chip_typename);
> @@ -728,7 +744,7 @@ static void ppc_powernv_init(MachineState *machine)
>          object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal);
>          object_property_set_int(chip, PNV_CHIP_HWID(i), "chip-id",
>                                  &error_fatal);
> -        object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> +        object_property_set_int(chip, cores_per_chip, "nr-cores", &error_fatal);
>          object_property_set_int(chip, 1, "num-phbs", &error_fatal);
>          object_property_set_bool(chip, true, "realized", &error_fatal);
>      }

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 06, 2017 at 01:57:48PM +0530, Nikunj A Dadhania wrote:
>> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
>> initialize 4 cpus. Compute the chip per cores depending on the number of chips
>> and smt threads.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>
> I don't understand why simply treating smp_cores as cores per chip is wrong.

We do not have SMT support and when "-smp 4" is passed, smp_cores is
always set to 1. So only once core with one thread finally show up in
the guest. Moreover, I see spapr too doing similar thing in
spapr_init_cpus() with boot_cores_nr.

Regards
Nikunj

On Mon, Sep 11, 2017 at 10:40:10AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Wed, Sep 06, 2017 at 01:57:48PM +0530, Nikunj A Dadhania wrote:
> >> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
> >> initialize 4 cpus. Compute the chip per cores depending on the number of chips
> >> and smt threads.
> >> 
> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >
> > I don't understand why simply treating smp_cores as cores per chip is wrong.
> 
> We do not have SMT support and when "-smp 4" is passed, smp_cores is
> always set to 1. So only once core with one thread finally show up in
> the guest. Moreover, I see spapr too doing similar thing in
> spapr_init_cpus() with boot_cores_nr.

I'm ok with adding an error if smp_threads > 1, since that won't
work.  Breaking the identity that smp_cores is the number of cores per
socket (which should correspond to one chip) is not ok, though.

I think you're misinterpreting the boot_cores_nr stuff - that's just
about translating the number of initially online vcpus into a number
of initially online cores.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Mon, Sep 11, 2017 at 10:40:10AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Wed, Sep 06, 2017 at 01:57:48PM +0530, Nikunj A Dadhania wrote:
>> >> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
>> >> initialize 4 cpus. Compute the chip per cores depending on the number of chips
>> >> and smt threads.
>> >> 
>> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> >
>> > I don't understand why simply treating smp_cores as cores per chip is wrong.
>> 
>> We do not have SMT support and when "-smp 4" is passed, smp_cores is
>> always set to 1. So only once core with one thread finally show up in
>> the guest. Moreover, I see spapr too doing similar thing in
>> spapr_init_cpus() with boot_cores_nr.
>
> I'm ok with adding an error if smp_threads > 1, since that won't
> work.  Breaking the identity that smp_cores is the number of cores per
> socket (which should correspond to one chip) is not ok, though.
>
> I think you're misinterpreting the boot_cores_nr stuff - that's just
> about translating the number of initially online vcpus into a number
> of initially online cores.

I thought, I am doing the same here for PowerNV, number of online cores
is equal to initial online vcpus / threads per core

   int boot_cores_nr = smp_cpus / smp_threads;

Only difference that I see in PowerNV is that we have multiple chips
(max 2, at the moment)

        cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);

And in case user has provided sane smp_cores, we use it.

Regards,
Nikunj

On Thu, Sep 14, 2017 at 10:42:52AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Mon, Sep 11, 2017 at 10:40:10AM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Wed, Sep 06, 2017 at 01:57:48PM +0530, Nikunj A Dadhania wrote:
> >> >> When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
> >> >> initialize 4 cpus. Compute the chip per cores depending on the number of chips
> >> >> and smt threads.
> >> >> 
> >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> >
> >> > I don't understand why simply treating smp_cores as cores per chip is wrong.
> >> 
> >> We do not have SMT support and when "-smp 4" is passed, smp_cores is
> >> always set to 1. So only once core with one thread finally show up in
> >> the guest. Moreover, I see spapr too doing similar thing in
> >> spapr_init_cpus() with boot_cores_nr.
> >
> > I'm ok with adding an error if smp_threads > 1, since that won't
> > work.  Breaking the identity that smp_cores is the number of cores per
> > socket (which should correspond to one chip) is not ok, though.
> >
> > I think you're misinterpreting the boot_cores_nr stuff - that's just
> > about translating the number of initially online vcpus into a number
> > of initially online cores.
> 
> I thought, I am doing the same here for PowerNV, number of online cores
> is equal to initial online vcpus / threads per core
> 
>    int boot_cores_nr = smp_cpus / smp_threads;
> 
> Only difference that I see in PowerNV is that we have multiple chips
> (max 2, at the moment)
> 
>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);

This doesn't make sense to me.  Cores per chip should *always* equal
smp_cores, you shouldn't need another calculation for it.

> And in case user has provided sane smp_cores, we use it.

If smp_cores isn't sane, you should simply reject it, not try to fix
it.  That's just asking for confusion.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

>> 
>> I thought, I am doing the same here for PowerNV, number of online cores
>> is equal to initial online vcpus / threads per core
>> 
>>    int boot_cores_nr = smp_cpus / smp_threads;
>> 
>> Only difference that I see in PowerNV is that we have multiple chips
>> (max 2, at the moment)
>> 
>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>
> This doesn't make sense to me.  Cores per chip should *always* equal
> smp_cores, you shouldn't need another calculation for it.
>
>> And in case user has provided sane smp_cores, we use it.
>
> If smp_cores isn't sane, you should simply reject it, not try to fix
> it.  That's just asking for confusion.

This is the case where the user does not provide a topology(which is a
valid scenario), not sure we should reject it. So qemu defaults
smp_cores/smt_threads to 1. I think it makes sense to over-ride.

Regards
Nikunj

On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> >> 
> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> is equal to initial online vcpus / threads per core
> >> 
> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> 
> >> Only difference that I see in PowerNV is that we have multiple chips
> >> (max 2, at the moment)
> >> 
> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >
> > This doesn't make sense to me.  Cores per chip should *always* equal
> > smp_cores, you shouldn't need another calculation for it.
> >
> >> And in case user has provided sane smp_cores, we use it.
> >
> > If smp_cores isn't sane, you should simply reject it, not try to fix
> > it.  That's just asking for confusion.
> 
> This is the case where the user does not provide a topology(which is a
> valid scenario), not sure we should reject it. So qemu defaults
> smp_cores/smt_threads to 1. I think it makes sense to over-ride.

If you can find a way to override it by altering smp_cores when it's
not explicitly specified, then ok.

But overriding smp_cores with a different variable that's the "real"
number of cores is not acceptable.  If that means the user has to
specify cores explicitly, so be it.  Slight awkwardness in command
line is preferable to breaking the assumption that smp_cores == (# of
cores per next level up cpu object) which is used all over the place.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> >> 
>> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> is equal to initial online vcpus / threads per core
>> >> 
>> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> 
>> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> (max 2, at the moment)
>> >> 
>> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >
>> > This doesn't make sense to me.  Cores per chip should *always* equal
>> > smp_cores, you shouldn't need another calculation for it.
>> >
>> >> And in case user has provided sane smp_cores, we use it.
>> >
>> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> > it.  That's just asking for confusion.
>> 
>> This is the case where the user does not provide a topology(which is a
>> valid scenario), not sure we should reject it. So qemu defaults
>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>
> If you can find a way to override it by altering smp_cores when it's
> not explicitly specified, then ok.

Should I change the global smp_cores here as well ?

> But overriding smp_cores with a different variable that's the "real"
> number of cores is not acceptable. If that means the user has to
> specify cores explicitly, so be it.

Right, we would error out in case there is mismatch.

> Slight awkwardness in command line is preferable to breaking the
> assumption that smp_cores == (# of cores per next level up cpu object)
> which is used all over the place.

Regards
Nikunj

On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> >> 
> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> is equal to initial online vcpus / threads per core
> >> >> 
> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> 
> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> (max 2, at the moment)
> >> >> 
> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >
> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> > smp_cores, you shouldn't need another calculation for it.
> >> >
> >> >> And in case user has provided sane smp_cores, we use it.
> >> >
> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> > it.  That's just asking for confusion.
> >> 
> >> This is the case where the user does not provide a topology(which is a
> >> valid scenario), not sure we should reject it. So qemu defaults
> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >
> > If you can find a way to override it by altering smp_cores when it's
> > not explicitly specified, then ok.
> 
> Should I change the global smp_cores here as well ?

I'm pretty uneasy with that option.  It would take a fair bit of
checking to ensure that changing smp_cores is safe here.  An easier to
verify option would be to make the generic logic which splits up an
unspecified -smp N into cores and sockets more flexible, possibly
based on machine options for max values.

That might still be more trouble than its worth.

> > But overriding smp_cores with a different variable that's the "real"
> > number of cores is not acceptable. If that means the user has to
> > specify cores explicitly, so be it.
> 
> Right, we would error out in case there is mismatch.
> 
> > Slight awkwardness in command line is preferable to breaking the
> > assumption that smp_cores == (# of cores per next level up cpu object)
> > which is used all over the place.
> 
> Regards
> Nikunj
> 

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> 
>> >> >> 
>> >> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> >> is equal to initial online vcpus / threads per core
>> >> >> 
>> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> >> 
>> >> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> >> (max 2, at the moment)
>> >> >> 
>> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >> >
>> >> > This doesn't make sense to me.  Cores per chip should *always* equal
>> >> > smp_cores, you shouldn't need another calculation for it.
>> >> >
>> >> >> And in case user has provided sane smp_cores, we use it.
>> >> >
>> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> >> > it.  That's just asking for confusion.
>> >> 
>> >> This is the case where the user does not provide a topology(which is a
>> >> valid scenario), not sure we should reject it. So qemu defaults
>> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>> >
>> > If you can find a way to override it by altering smp_cores when it's
>> > not explicitly specified, then ok.
>> 
>> Should I change the global smp_cores here as well ?
>
> I'm pretty uneasy with that option.

Me too.

> It would take a fair bit of checking to ensure that changing smp_cores
> is safe here. An easier to verify option would be to make the generic
> logic which splits up an unspecified -smp N into cores and sockets
> more flexible, possibly based on machine options for max values.
>
> That might still be more trouble than its worth.

I think the current approach is the simplest and less intrusive, as we
are handling a case where user has not bothered to provide a detailed
topology, the best we can do is create single threaded cores equal to
number of cores.

Regards
Nikunj

On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> 
> >> >> >> 
> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> >> is equal to initial online vcpus / threads per core
> >> >> >> 
> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> >> 
> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> >> (max 2, at the moment)
> >> >> >> 
> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >> >
> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> >> > smp_cores, you shouldn't need another calculation for it.
> >> >> >
> >> >> >> And in case user has provided sane smp_cores, we use it.
> >> >> >
> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> >> > it.  That's just asking for confusion.
> >> >> 
> >> >> This is the case where the user does not provide a topology(which is a
> >> >> valid scenario), not sure we should reject it. So qemu defaults
> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >> >
> >> > If you can find a way to override it by altering smp_cores when it's
> >> > not explicitly specified, then ok.
> >> 
> >> Should I change the global smp_cores here as well ?
> >
> > I'm pretty uneasy with that option.
> 
> Me too.
> 
> > It would take a fair bit of checking to ensure that changing smp_cores
> > is safe here. An easier to verify option would be to make the generic
> > logic which splits up an unspecified -smp N into cores and sockets
> > more flexible, possibly based on machine options for max values.
> >
> > That might still be more trouble than its worth.
> 
> I think the current approach is the simplest and less intrusive, as we
> are handling a case where user has not bothered to provide a detailed
> topology, the best we can do is create single threaded cores equal to
> number of cores.

No, sorry.  Having smp_cores not correspond to the number of cores per
chip in all cases is just not ok.  Add an error message if the
topology isn't workable for powernv by all means.  But users having to
use a longer command line is better than breaking basic assumptions
about what numbers reflect what topology.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> 
>> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> 
>> >> >> >> 
>> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> >> >> is equal to initial online vcpus / threads per core
>> >> >> >> 
>> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> >> >> 
>> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> >> >> (max 2, at the moment)
>> >> >> >> 
>> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >> >> >
>> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
>> >> >> > smp_cores, you shouldn't need another calculation for it.
>> >> >> >
>> >> >> >> And in case user has provided sane smp_cores, we use it.
>> >> >> >
>> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> >> >> > it.  That's just asking for confusion.
>> >> >> 
>> >> >> This is the case where the user does not provide a topology(which is a
>> >> >> valid scenario), not sure we should reject it. So qemu defaults
>> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>> >> >
>> >> > If you can find a way to override it by altering smp_cores when it's
>> >> > not explicitly specified, then ok.
>> >> 
>> >> Should I change the global smp_cores here as well ?
>> >
>> > I'm pretty uneasy with that option.
>> 
>> Me too.
>> 
>> > It would take a fair bit of checking to ensure that changing smp_cores
>> > is safe here. An easier to verify option would be to make the generic
>> > logic which splits up an unspecified -smp N into cores and sockets
>> > more flexible, possibly based on machine options for max values.
>> >
>> > That might still be more trouble than its worth.
>> 
>> I think the current approach is the simplest and less intrusive, as we
>> are handling a case where user has not bothered to provide a detailed
>> topology, the best we can do is create single threaded cores equal to
>> number of cores.
>
> No, sorry.  Having smp_cores not correspond to the number of cores per
> chip in all cases is just not ok.  Add an error message if the
> topology isn't workable for powernv by all means.  But users having to
> use a longer command line is better than breaking basic assumptions
> about what numbers reflect what topology.

Sorry to ask again, as I am still not convinced, we do similar
adjustment in spapr where the user did not provide the number of cores,
but qemu assumes them as single threaded cores and created
cores(boot_cores_nr) that were not same as smp_cores ?

Regards
Nikunj

On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> 
> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> 
> >> >> >> >> 
> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> >> >> is equal to initial online vcpus / threads per core
> >> >> >> >> 
> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> >> >> 
> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> >> >> (max 2, at the moment)
> >> >> >> >> 
> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >> >> >
> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> >> >> > smp_cores, you shouldn't need another calculation for it.
> >> >> >> >
> >> >> >> >> And in case user has provided sane smp_cores, we use it.
> >> >> >> >
> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> >> >> > it.  That's just asking for confusion.
> >> >> >> 
> >> >> >> This is the case where the user does not provide a topology(which is a
> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >> >> >
> >> >> > If you can find a way to override it by altering smp_cores when it's
> >> >> > not explicitly specified, then ok.
> >> >> 
> >> >> Should I change the global smp_cores here as well ?
> >> >
> >> > I'm pretty uneasy with that option.
> >> 
> >> Me too.
> >> 
> >> > It would take a fair bit of checking to ensure that changing smp_cores
> >> > is safe here. An easier to verify option would be to make the generic
> >> > logic which splits up an unspecified -smp N into cores and sockets
> >> > more flexible, possibly based on machine options for max values.
> >> >
> >> > That might still be more trouble than its worth.
> >> 
> >> I think the current approach is the simplest and less intrusive, as we
> >> are handling a case where user has not bothered to provide a detailed
> >> topology, the best we can do is create single threaded cores equal to
> >> number of cores.
> >
> > No, sorry.  Having smp_cores not correspond to the number of cores per
> > chip in all cases is just not ok.  Add an error message if the
> > topology isn't workable for powernv by all means.  But users having to
> > use a longer command line is better than breaking basic assumptions
> > about what numbers reflect what topology.
> 
> Sorry to ask again, as I am still not convinced, we do similar
> adjustment in spapr where the user did not provide the number of cores,
> but qemu assumes them as single threaded cores and created
> cores(boot_cores_nr) that were not same as smp_cores ?

What?  boot_cores_nr has absolutely nothing to do with adjusting the
topology, and it certainly doesn't assume they're single threaded.

boot_cores_nr is simply the number of cores (each of smp_threads
threads) which are online initially.  In an sPAPR system there are
max_cpus total potential threads.  Those are divided into cores each
with smp_threads threads (so max_cpus / smp_threads total cores), and
those cores are divided into sockets each with smp_cores sockets (so
max_cpus / smp_threads / smp_cores total sockets).  Of all those
potential threads smp_cpus are initially online (the rest can be
hotplugged later), so there are smp_cpus / smp_threads cores initially
online.

We need that calculation because we can only hotplug cpus on spapr at
core granularity, not thread granularity (x86 can do that).  If
smp_cpus is not a multiple of smp_threads we give an error (except for
old machine types, where we have some hacks for backwards compat).

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> 
>> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> 
>> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> 
>> >> >> >> >> 
>> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> >> >> >> is equal to initial online vcpus / threads per core
>> >> >> >> >> 
>> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> >> >> >> 
>> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> >> >> >> (max 2, at the moment)
>> >> >> >> >> 
>> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >> >> >> >
>> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
>> >> >> >> > smp_cores, you shouldn't need another calculation for it.
>> >> >> >> >
>> >> >> >> >> And in case user has provided sane smp_cores, we use it.
>> >> >> >> >
>> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> >> >> >> > it.  That's just asking for confusion.
>> >> >> >> 
>> >> >> >> This is the case where the user does not provide a topology(which is a
>> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
>> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>> >> >> >
>> >> >> > If you can find a way to override it by altering smp_cores when it's
>> >> >> > not explicitly specified, then ok.
>> >> >> 
>> >> >> Should I change the global smp_cores here as well ?
>> >> >
>> >> > I'm pretty uneasy with that option.
>> >> 
>> >> Me too.
>> >> 
>> >> > It would take a fair bit of checking to ensure that changing smp_cores
>> >> > is safe here. An easier to verify option would be to make the generic
>> >> > logic which splits up an unspecified -smp N into cores and sockets
>> >> > more flexible, possibly based on machine options for max values.
>> >> >
>> >> > That might still be more trouble than its worth.
>> >> 
>> >> I think the current approach is the simplest and less intrusive, as we
>> >> are handling a case where user has not bothered to provide a detailed
>> >> topology, the best we can do is create single threaded cores equal to
>> >> number of cores.
>> >
>> > No, sorry.  Having smp_cores not correspond to the number of cores per
>> > chip in all cases is just not ok.  Add an error message if the
>> > topology isn't workable for powernv by all means.  But users having to
>> > use a longer command line is better than breaking basic assumptions
>> > about what numbers reflect what topology.
>> 
>> Sorry to ask again, as I am still not convinced, we do similar
>> adjustment in spapr where the user did not provide the number of cores,
>> but qemu assumes them as single threaded cores and created
>> cores(boot_cores_nr) that were not same as smp_cores ?
>
> What?  boot_cores_nr has absolutely nothing to do with adjusting the
> topology, and it certainly doesn't assume they're single threaded.

When we start a TCG guest and user provides following commandline, e.g.
"-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
with 4 cores, each having 1 thread.

Regards
Nikunj

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

>> >> 
>>> >> I think the current approach is the simplest and less intrusive, as we
>>> >> are handling a case where user has not bothered to provide a detailed
>>> >> topology, the best we can do is create single threaded cores equal to
>>> >> number of cores.
>>> >
>>> > No, sorry.  Having smp_cores not correspond to the number of cores per
>>> > chip in all cases is just not ok.  Add an error message if the
>>> > topology isn't workable for powernv by all means.  But users having to
>>> > use a longer command line is better than breaking basic assumptions
>>> > about what numbers reflect what topology.
>>> 
>>> Sorry to ask again, as I am still not convinced, we do similar
>>> adjustment in spapr where the user did not provide the number of cores,
>>> but qemu assumes them as single threaded cores and created
>>> cores(boot_cores_nr) that were not same as smp_cores ?
>>
>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
>> topology, and it certainly doesn't assume they're single threaded.
>
> When we start a TCG guest and user provides following commandline,
> e.g. "-smp 4", smt_threads is set to 1 by default in vl.c.

I meant smp_threads here.

> So the guest boots with 4 cores, each having 1 thread.
>
> Regards
> Nikunj

On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> 
> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> 
> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> 
> >> >> >> >> >> 
> >> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> >> >> >> is equal to initial online vcpus / threads per core
> >> >> >> >> >> 
> >> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> >> >> >> 
> >> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> >> >> >> (max 2, at the moment)
> >> >> >> >> >> 
> >> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >> >> >> >
> >> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> >> >> >> > smp_cores, you shouldn't need another calculation for it.
> >> >> >> >> >
> >> >> >> >> >> And in case user has provided sane smp_cores, we use it.
> >> >> >> >> >
> >> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> >> >> >> > it.  That's just asking for confusion.
> >> >> >> >> 
> >> >> >> >> This is the case where the user does not provide a topology(which is a
> >> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
> >> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >> >> >> >
> >> >> >> > If you can find a way to override it by altering smp_cores when it's
> >> >> >> > not explicitly specified, then ok.
> >> >> >> 
> >> >> >> Should I change the global smp_cores here as well ?
> >> >> >
> >> >> > I'm pretty uneasy with that option.
> >> >> 
> >> >> Me too.
> >> >> 
> >> >> > It would take a fair bit of checking to ensure that changing smp_cores
> >> >> > is safe here. An easier to verify option would be to make the generic
> >> >> > logic which splits up an unspecified -smp N into cores and sockets
> >> >> > more flexible, possibly based on machine options for max values.
> >> >> >
> >> >> > That might still be more trouble than its worth.
> >> >> 
> >> >> I think the current approach is the simplest and less intrusive, as we
> >> >> are handling a case where user has not bothered to provide a detailed
> >> >> topology, the best we can do is create single threaded cores equal to
> >> >> number of cores.
> >> >
> >> > No, sorry.  Having smp_cores not correspond to the number of cores per
> >> > chip in all cases is just not ok.  Add an error message if the
> >> > topology isn't workable for powernv by all means.  But users having to
> >> > use a longer command line is better than breaking basic assumptions
> >> > about what numbers reflect what topology.
> >> 
> >> Sorry to ask again, as I am still not convinced, we do similar
> >> adjustment in spapr where the user did not provide the number of cores,
> >> but qemu assumes them as single threaded cores and created
> >> cores(boot_cores_nr) that were not same as smp_cores ?
> >
> > What?  boot_cores_nr has absolutely nothing to do with adjusting the
> > topology, and it certainly doesn't assume they're single threaded.
> 
> When we start a TCG guest and user provides following commandline, e.g.
> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> with 4 cores, each having 1 thread.

Ok.. and what's the problem with that behaviour on powernv?

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> 
>> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> 
>> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> 
>> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> >> 
>> >> >> >> >> >> 
>> >> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> >> >> >> >> is equal to initial online vcpus / threads per core
>> >> >> >> >> >> 
>> >> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> >> >> >> >> 
>> >> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> >> >> >> >> (max 2, at the moment)
>> >> >> >> >> >> 
>> >> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >> >> >> >> >
>> >> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
>> >> >> >> >> > smp_cores, you shouldn't need another calculation for it.
>> >> >> >> >> >
>> >> >> >> >> >> And in case user has provided sane smp_cores, we use it.
>> >> >> >> >> >
>> >> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> >> >> >> >> > it.  That's just asking for confusion.
>> >> >> >> >> 
>> >> >> >> >> This is the case where the user does not provide a topology(which is a
>> >> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
>> >> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>> >> >> >> >
>> >> >> >> > If you can find a way to override it by altering smp_cores when it's
>> >> >> >> > not explicitly specified, then ok.
>> >> >> >> 
>> >> >> >> Should I change the global smp_cores here as well ?
>> >> >> >
>> >> >> > I'm pretty uneasy with that option.
>> >> >> 
>> >> >> Me too.
>> >> >> 
>> >> >> > It would take a fair bit of checking to ensure that changing smp_cores
>> >> >> > is safe here. An easier to verify option would be to make the generic
>> >> >> > logic which splits up an unspecified -smp N into cores and sockets
>> >> >> > more flexible, possibly based on machine options for max values.
>> >> >> >
>> >> >> > That might still be more trouble than its worth.
>> >> >> 
>> >> >> I think the current approach is the simplest and less intrusive, as we
>> >> >> are handling a case where user has not bothered to provide a detailed
>> >> >> topology, the best we can do is create single threaded cores equal to
>> >> >> number of cores.
>> >> >
>> >> > No, sorry.  Having smp_cores not correspond to the number of cores per
>> >> > chip in all cases is just not ok.  Add an error message if the
>> >> > topology isn't workable for powernv by all means.  But users having to
>> >> > use a longer command line is better than breaking basic assumptions
>> >> > about what numbers reflect what topology.
>> >> 
>> >> Sorry to ask again, as I am still not convinced, we do similar
>> >> adjustment in spapr where the user did not provide the number of cores,
>> >> but qemu assumes them as single threaded cores and created
>> >> cores(boot_cores_nr) that were not same as smp_cores ?
>> >
>> > What?  boot_cores_nr has absolutely nothing to do with adjusting the
>> > topology, and it certainly doesn't assume they're single threaded.
>> 
>> When we start a TCG guest and user provides following commandline, e.g.
>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>> with 4 cores, each having 1 thread.
>
> Ok.. and what's the problem with that behaviour on powernv?

As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
default value of 1 in vl.c. In powernv, we were setting nr-cores like
this:

        object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);

Even when there were multiple cpus (-smp 4), when the guest boots up, we
just get one core (i.e. smp_cores was 1) with single thread(smp_threads
was 1), which is wrong as per the command-line that was provided.

Regards
Nikunj

On 09/20/2017 09:18 AM, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>
>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>
>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>
>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>
>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
>>>>>>>>>>>>>
>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
>>>>>>>>>>>>> (max 2, at the moment)
>>>>>>>>>>>>>
>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>>>>>>>>>>>>
>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
>>>>>>>>>>>>
>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.
>>>>>>>>>>>>
>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
>>>>>>>>>>>> it.  That's just asking for confusion.
>>>>>>>>>>>
>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>>>>>>>>>>
>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
>>>>>>>>>> not explicitly specified, then ok.
>>>>>>>>>
>>>>>>>>> Should I change the global smp_cores here as well ?
>>>>>>>>
>>>>>>>> I'm pretty uneasy with that option.
>>>>>>>
>>>>>>> Me too.
>>>>>>>
>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
>>>>>>>> is safe here. An easier to verify option would be to make the generic
>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
>>>>>>>> more flexible, possibly based on machine options for max values.
>>>>>>>>
>>>>>>>> That might still be more trouble than its worth.
>>>>>>>
>>>>>>> I think the current approach is the simplest and less intrusive, as we
>>>>>>> are handling a case where user has not bothered to provide a detailed
>>>>>>> topology, the best we can do is create single threaded cores equal to
>>>>>>> number of cores.
>>>>>>
>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
>>>>>> chip in all cases is just not ok.  Add an error message if the
>>>>>> topology isn't workable for powernv by all means.  But users having to
>>>>>> use a longer command line is better than breaking basic assumptions
>>>>>> about what numbers reflect what topology.
>>>>>
>>>>> Sorry to ask again, as I am still not convinced, we do similar
>>>>> adjustment in spapr where the user did not provide the number of cores,
>>>>> but qemu assumes them as single threaded cores and created
>>>>> cores(boot_cores_nr) that were not same as smp_cores ?
>>>>
>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
>>>> topology, and it certainly doesn't assume they're single threaded.
>>>
>>> When we start a TCG guest and user provides following commandline, e.g.
>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>>> with 4 cores, each having 1 thread.
>>
>> Ok.. and what's the problem with that behaviour on powernv?
> 
> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> this:
> 
>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> 
> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> was 1), which is wrong as per the command-line that was provided.

I have never noticed as always use the cores= option but if you use
the following on a powernv machine:

 -smp 4				1 cpu
 -smp cpus=4,threads=1		1 cpu
 -smp cores=4,threads=1		4 cpus
 -smp cpus=4,cores=4,threads=1	4 cpus
 -smp cpus=1,cores=4,threads=1	fails
 -smp cpus=4,cores=1,threads=1	1 cpu

Should we be using 'smp_cpus' instead of 'smp_cores' then ? Honestly,
I feel a bit lost with all default behaviors.

C. 

On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> 
> >> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> 
> >> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> 
> >> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> >> 
> >> >> >> >> >> >> 
> >> >> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> >> >> >> >> is equal to initial online vcpus / threads per core
> >> >> >> >> >> >> 
> >> >> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> >> >> >> >> 
> >> >> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> >> >> >> >> (max 2, at the moment)
> >> >> >> >> >> >> 
> >> >> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >> >> >> >> >
> >> >> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> >> >> >> >> > smp_cores, you shouldn't need another calculation for it.
> >> >> >> >> >> >
> >> >> >> >> >> >> And in case user has provided sane smp_cores, we use it.
> >> >> >> >> >> >
> >> >> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> >> >> >> >> > it.  That's just asking for confusion.
> >> >> >> >> >> 
> >> >> >> >> >> This is the case where the user does not provide a topology(which is a
> >> >> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
> >> >> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >> >> >> >> >
> >> >> >> >> > If you can find a way to override it by altering smp_cores when it's
> >> >> >> >> > not explicitly specified, then ok.
> >> >> >> >> 
> >> >> >> >> Should I change the global smp_cores here as well ?
> >> >> >> >
> >> >> >> > I'm pretty uneasy with that option.
> >> >> >> 
> >> >> >> Me too.
> >> >> >> 
> >> >> >> > It would take a fair bit of checking to ensure that changing smp_cores
> >> >> >> > is safe here. An easier to verify option would be to make the generic
> >> >> >> > logic which splits up an unspecified -smp N into cores and sockets
> >> >> >> > more flexible, possibly based on machine options for max values.
> >> >> >> >
> >> >> >> > That might still be more trouble than its worth.
> >> >> >> 
> >> >> >> I think the current approach is the simplest and less intrusive, as we
> >> >> >> are handling a case where user has not bothered to provide a detailed
> >> >> >> topology, the best we can do is create single threaded cores equal to
> >> >> >> number of cores.
> >> >> >
> >> >> > No, sorry.  Having smp_cores not correspond to the number of cores per
> >> >> > chip in all cases is just not ok.  Add an error message if the
> >> >> > topology isn't workable for powernv by all means.  But users having to
> >> >> > use a longer command line is better than breaking basic assumptions
> >> >> > about what numbers reflect what topology.
> >> >> 
> >> >> Sorry to ask again, as I am still not convinced, we do similar
> >> >> adjustment in spapr where the user did not provide the number of cores,
> >> >> but qemu assumes them as single threaded cores and created
> >> >> cores(boot_cores_nr) that were not same as smp_cores ?
> >> >
> >> > What?  boot_cores_nr has absolutely nothing to do with adjusting the
> >> > topology, and it certainly doesn't assume they're single threaded.
> >> 
> >> When we start a TCG guest and user provides following commandline, e.g.
> >> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> >> with 4 cores, each having 1 thread.
> >
> > Ok.. and what's the problem with that behaviour on powernv?
> 
> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> this:
> 
>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> 
> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> was 1), which is wrong as per the command-line that was provided.

Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
thread.  If you can't supply 4 sockets you should error, but you
shouldn't go and change the number of cores per socket.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> 
>> >> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> 
>> >> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> 
>> >> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> >> 
>> >> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>> >> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
>> >> >> >> >> >> 
>> >> >> >> >> >> >> 
>> >> >> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
>> >> >> >> >> >> >> is equal to initial online vcpus / threads per core
>> >> >> >> >> >> >> 
>> >> >> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
>> >> >> >> >> >> >> 
>> >> >> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
>> >> >> >> >> >> >> (max 2, at the moment)
>> >> >> >> >> >> >> 
>> >> >> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>> >> >> >> >> >> >
>> >> >> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
>> >> >> >> >> >> > smp_cores, you shouldn't need another calculation for it.
>> >> >> >> >> >> >
>> >> >> >> >> >> >> And in case user has provided sane smp_cores, we use it.
>> >> >> >> >> >> >
>> >> >> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
>> >> >> >> >> >> > it.  That's just asking for confusion.
>> >> >> >> >> >> 
>> >> >> >> >> >> This is the case where the user does not provide a topology(which is a
>> >> >> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
>> >> >> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>> >> >> >> >> >
>> >> >> >> >> > If you can find a way to override it by altering smp_cores when it's
>> >> >> >> >> > not explicitly specified, then ok.
>> >> >> >> >> 
>> >> >> >> >> Should I change the global smp_cores here as well ?
>> >> >> >> >
>> >> >> >> > I'm pretty uneasy with that option.
>> >> >> >> 
>> >> >> >> Me too.
>> >> >> >> 
>> >> >> >> > It would take a fair bit of checking to ensure that changing smp_cores
>> >> >> >> > is safe here. An easier to verify option would be to make the generic
>> >> >> >> > logic which splits up an unspecified -smp N into cores and sockets
>> >> >> >> > more flexible, possibly based on machine options for max values.
>> >> >> >> >
>> >> >> >> > That might still be more trouble than its worth.
>> >> >> >> 
>> >> >> >> I think the current approach is the simplest and less intrusive, as we
>> >> >> >> are handling a case where user has not bothered to provide a detailed
>> >> >> >> topology, the best we can do is create single threaded cores equal to
>> >> >> >> number of cores.
>> >> >> >
>> >> >> > No, sorry.  Having smp_cores not correspond to the number of cores per
>> >> >> > chip in all cases is just not ok.  Add an error message if the
>> >> >> > topology isn't workable for powernv by all means.  But users having to
>> >> >> > use a longer command line is better than breaking basic assumptions
>> >> >> > about what numbers reflect what topology.
>> >> >> 
>> >> >> Sorry to ask again, as I am still not convinced, we do similar
>> >> >> adjustment in spapr where the user did not provide the number of cores,
>> >> >> but qemu assumes them as single threaded cores and created
>> >> >> cores(boot_cores_nr) that were not same as smp_cores ?
>> >> >
>> >> > What?  boot_cores_nr has absolutely nothing to do with adjusting the
>> >> > topology, and it certainly doesn't assume they're single threaded.
>> >> 
>> >> When we start a TCG guest and user provides following commandline, e.g.
>> >> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>> >> with 4 cores, each having 1 thread.
>> >
>> > Ok.. and what's the problem with that behaviour on powernv?
>> 
>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>> this:
>> 
>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>> 
>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>> was 1), which is wrong as per the command-line that was provided.
>
> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> thread.  If you can't supply 4 sockets you should error, but you
> shouldn't go and change the number of cores per socket.

OK, that makes sense now. And I do see that smp_cpus is 4 in the above
case. Now looking more into it, i see that powernv has something called
"num_chips", isnt this same as sockets ? Do we need num_chips separately?

Regards,
Nikunj

On Thu, Sep 21, 2017 at 09:24:46AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> 
> >> >> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> 
> >> >> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> 
> >> >> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> >> 
> >> >> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> >> >> >> >> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> >> >> >> >> >> 
> >> >> >> >> >> >> >> 
> >> >> >> >> >> >> >> I thought, I am doing the same here for PowerNV, number of online cores
> >> >> >> >> >> >> >> is equal to initial online vcpus / threads per core
> >> >> >> >> >> >> >> 
> >> >> >> >> >> >> >>    int boot_cores_nr = smp_cpus / smp_threads;
> >> >> >> >> >> >> >> 
> >> >> >> >> >> >> >> Only difference that I see in PowerNV is that we have multiple chips
> >> >> >> >> >> >> >> (max 2, at the moment)
> >> >> >> >> >> >> >> 
> >> >> >> >> >> >> >>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > This doesn't make sense to me.  Cores per chip should *always* equal
> >> >> >> >> >> >> > smp_cores, you shouldn't need another calculation for it.
> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> And in case user has provided sane smp_cores, we use it.
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > If smp_cores isn't sane, you should simply reject it, not try to fix
> >> >> >> >> >> >> > it.  That's just asking for confusion.
> >> >> >> >> >> >> 
> >> >> >> >> >> >> This is the case where the user does not provide a topology(which is a
> >> >> >> >> >> >> valid scenario), not sure we should reject it. So qemu defaults
> >> >> >> >> >> >> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >> >> >> >> >> >
> >> >> >> >> >> > If you can find a way to override it by altering smp_cores when it's
> >> >> >> >> >> > not explicitly specified, then ok.
> >> >> >> >> >> 
> >> >> >> >> >> Should I change the global smp_cores here as well ?
> >> >> >> >> >
> >> >> >> >> > I'm pretty uneasy with that option.
> >> >> >> >> 
> >> >> >> >> Me too.
> >> >> >> >> 
> >> >> >> >> > It would take a fair bit of checking to ensure that changing smp_cores
> >> >> >> >> > is safe here. An easier to verify option would be to make the generic
> >> >> >> >> > logic which splits up an unspecified -smp N into cores and sockets
> >> >> >> >> > more flexible, possibly based on machine options for max values.
> >> >> >> >> >
> >> >> >> >> > That might still be more trouble than its worth.
> >> >> >> >> 
> >> >> >> >> I think the current approach is the simplest and less intrusive, as we
> >> >> >> >> are handling a case where user has not bothered to provide a detailed
> >> >> >> >> topology, the best we can do is create single threaded cores equal to
> >> >> >> >> number of cores.
> >> >> >> >
> >> >> >> > No, sorry.  Having smp_cores not correspond to the number of cores per
> >> >> >> > chip in all cases is just not ok.  Add an error message if the
> >> >> >> > topology isn't workable for powernv by all means.  But users having to
> >> >> >> > use a longer command line is better than breaking basic assumptions
> >> >> >> > about what numbers reflect what topology.
> >> >> >> 
> >> >> >> Sorry to ask again, as I am still not convinced, we do similar
> >> >> >> adjustment in spapr where the user did not provide the number of cores,
> >> >> >> but qemu assumes them as single threaded cores and created
> >> >> >> cores(boot_cores_nr) that were not same as smp_cores ?
> >> >> >
> >> >> > What?  boot_cores_nr has absolutely nothing to do with adjusting the
> >> >> > topology, and it certainly doesn't assume they're single threaded.
> >> >> 
> >> >> When we start a TCG guest and user provides following commandline, e.g.
> >> >> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> >> >> with 4 cores, each having 1 thread.
> >> >
> >> > Ok.. and what's the problem with that behaviour on powernv?
> >> 
> >> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >> this:
> >> 
> >>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >> 
> >> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >> was 1), which is wrong as per the command-line that was provided.
> >
> > Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> > thread.  If you can't supply 4 sockets you should error, but you
> > shouldn't go and change the number of cores per socket.
> 
> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> case. Now looking more into it, i see that powernv has something called
> "num_chips", isnt this same as sockets ? Do we need num_chips separately?

Ah, yes, I see.  It's probably still reasonable to keep num_chips as
an internal variable, rather than using (smp_cpus / smp_cores /
smp_threads) everywhere.  But we shouldn't have it as a direct
user-settable property, instead setting it from the -smp command line
option.

-- 
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

David Gibson <david@gibson.dropbear.id.au> writes:

>> >> 
>> >> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>> >> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>> >> this:
>> >> 
>> >>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>> >> 
>> >> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>> >> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>> >> was 1), which is wrong as per the command-line that was provided.
>> >
>> > Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>> > thread.  If you can't supply 4 sockets you should error, but you
>> > shouldn't go and change the number of cores per socket.
>> 
>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>> case. Now looking more into it, i see that powernv has something called
>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>
> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
> an internal variable, rather than using (smp_cpus / smp_cores /
> smp_threads) everywhere.  But we shouldn't have it as a direct
> user-settable property, instead setting it from the -smp command line
> option.

Something like the below works till num_chips=2, after that guest does
not boot up. This might be some limitation within the OS, Cedric might
have some clue. Otherwise, I see that multiple chips are created with
single core having single thread.

    ppc/pnv: Use num_chips for multiple sockets
    
    When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
    initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make sure
    that we initialize multiple chips for this.
    
    Remove the user-settable property num_chips from machine command line option
    
    Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 9724719..fa501f9 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -709,7 +709,17 @@ static void ppc_powernv_init(MachineState *machine)
         exit(1);
     }
 
+    pnv->num_chips = smp_cpus / (smp_cores * smp_threads);
     pnv->chips = g_new0(PnvChip *, pnv->num_chips);
+
+    if (smp_cpus != (smp_threads * pnv->num_chips * smp_cores)) {
+        error_report("cpu topology not balanced: "
+                     "chips (%u) * cores (%u) * threads (%u) != "
+                     "number of cpus (%u)",
+                     pnv->num_chips, smp_cores, smp_threads, smp_cpus);
+        exit(1);
+    }
+
     for (i = 0; i < pnv->num_chips; i++) {
         char chip_name[32];
         Object *chip = object_new(chip_typename);
@@ -1255,53 +1265,12 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
     }
 }
 
-static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
-{
-    visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp);
-}
-
-static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
-{
-    PnvMachineState *pnv = POWERNV_MACHINE(obj);
-    uint32_t num_chips;
-    Error *local_err = NULL;
-
-    visit_type_uint32(v, name, &num_chips, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    /*
-     * TODO: should we decide on how many chips we can create based
-     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
-     */
-    if (!is_power_of_2(num_chips) || num_chips > 4) {
-        error_setg(errp, "invalid number of chips: '%d'", num_chips);
-        return;
-    }
-
-    pnv->num_chips = num_chips;
-}
-
 static void powernv_machine_initfn(Object *obj)
 {
     PnvMachineState *pnv = POWERNV_MACHINE(obj);
     pnv->num_chips = 1;
 }
 
-static void powernv_machine_class_props_init(ObjectClass *oc)
-{
-    object_class_property_add(oc, "num-chips", "uint32",
-                              pnv_get_num_chips, pnv_set_num_chips,
-                              NULL, NULL, NULL);
-    object_class_property_set_description(oc, "num-chips",
-                              "Specifies the number of processor chips",
-                              NULL);
-}
-
 static void powernv_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1321,8 +1290,6 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
     xic->ics_get = pnv_ics_get;
     xic->ics_resend = pnv_ics_resend;
     ispc->print_info = pnv_pic_print_info;
-
-    powernv_machine_class_props_init(oc);
 }
 
 static const TypeInfo powernv_machine_info = {

On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>>>>>
>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>> this:
>>>>>
>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>>>
>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>
>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>> shouldn't go and change the number of cores per socket.
>>>
>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>> case. Now looking more into it, i see that powernv has something called
>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>
>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
>> an internal variable, rather than using (smp_cpus / smp_cores /
>> smp_threads) everywhere.  But we shouldn't have it as a direct
>> user-settable property, instead setting it from the -smp command line
>> option.
> 
> Something like the below works till num_chips=2, after that guest does
> not boot up. This might be some limitation within the OS, Cedric might
> have some clue.

Some controllers might need some more tweaking, PSI, LPC, to elect a 
master one. Anyhow I don't think we want to deduce the number of chips
from the number of cpus. These two figures are very different.

C.

> Otherwise, I see that multiple chips are created with single core having 
> single thread.
>
>     ppc/pnv: Use num_chips for multiple sockets
>     
>     When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
>     initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make sure
>     that we initialize multiple chips for this.
>     
>     Remove the user-settable property num_chips from machine command line option
>     
>     Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9724719..fa501f9 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -709,7 +709,17 @@ static void ppc_powernv_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    pnv->num_chips = smp_cpus / (smp_cores * smp_threads);
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> +
> +    if (smp_cpus != (smp_threads * pnv->num_chips * smp_cores)) {
> +        error_report("cpu topology not balanced: "
> +                     "chips (%u) * cores (%u) * threads (%u) != "
> +                     "number of cpus (%u)",
> +                     pnv->num_chips, smp_cores, smp_threads, smp_cpus);
> +        exit(1);
> +    }
> +
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
>          Object *chip = object_new(chip_typename);
> @@ -1255,53 +1265,12 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      }
>  }
>  
> -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp);
> -}
> -
> -static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> -    uint32_t num_chips;
> -    Error *local_err = NULL;
> -
> -    visit_type_uint32(v, name, &num_chips, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> -    /*
> -     * TODO: should we decide on how many chips we can create based
> -     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
> -     */
> -    if (!is_power_of_2(num_chips) || num_chips > 4) {
> -        error_setg(errp, "invalid number of chips: '%d'", num_chips);
> -        return;
> -    }
> -
> -    pnv->num_chips = num_chips;
> -}
> -
>  static void powernv_machine_initfn(Object *obj)
>  {
>      PnvMachineState *pnv = POWERNV_MACHINE(obj);
>      pnv->num_chips = 1;
>  }
>  
> -static void powernv_machine_class_props_init(ObjectClass *oc)
> -{
> -    object_class_property_add(oc, "num-chips", "uint32",
> -                              pnv_get_num_chips, pnv_set_num_chips,
> -                              NULL, NULL, NULL);
> -    object_class_property_set_description(oc, "num-chips",
> -                              "Specifies the number of processor chips",
> -                              NULL);
> -}
> -
>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1321,8 +1290,6 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
>      xic->ics_get = pnv_ics_get;
>      xic->ics_resend = pnv_ics_resend;
>      ispc->print_info = pnv_pic_print_info;
> -
> -    powernv_machine_class_props_init(oc);
>  }
>  
>  static const TypeInfo powernv_machine_info = {
> 

On Fri, Sep 22, 2017 at 08:07:06AM +0200, Cédric Le Goater wrote:
> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> > 
> >>>>>
> >>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >>>>> this:
> >>>>>
> >>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >>>>>
> >>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >>>>> was 1), which is wrong as per the command-line that was provided.
> >>>>
> >>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> >>>> thread.  If you can't supply 4 sockets you should error, but you
> >>>> shouldn't go and change the number of cores per socket.
> >>>
> >>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> >>> case. Now looking more into it, i see that powernv has something called
> >>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
> >>
> >> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
> >> an internal variable, rather than using (smp_cpus / smp_cores /
> >> smp_threads) everywhere.  But we shouldn't have it as a direct
> >> user-settable property, instead setting it from the -smp command line
> >> option.
> > 
> > Something like the below works till num_chips=2, after that guest does
> > not boot up. This might be some limitation within the OS, Cedric might
> > have some clue.
> 
> Some controllers might need some more tweaking, PSI, LPC, to elect a 
> master one.

Uh.. why?

> Anyhow I don't think we want to deduce the number of chips
> from the number of cpus. These two figures are very different.

How so?  It's not totally perfect, but making a single chip correspond
to a "socket" in qemu's (somewhat x86 centric) terminology is still a
pretty good match.

-- 
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

On 09/22/2017 12:12 PM, David Gibson wrote:
> On Fri, Sep 22, 2017 at 08:07:06AM +0200, Cédric Le Goater wrote:
>> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>
>>>>>>>
>>>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>>>> this:
>>>>>>>
>>>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>>>>>
>>>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>>>
>>>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>>>> shouldn't go and change the number of cores per socket.
>>>>>
>>>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>>>> case. Now looking more into it, i see that powernv has something called
>>>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>>>
>>>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
>>>> an internal variable, rather than using (smp_cpus / smp_cores /
>>>> smp_threads) everywhere.  But we shouldn't have it as a direct
>>>> user-settable property, instead setting it from the -smp command line
>>>> option.
>>>
>>> Something like the below works till num_chips=2, after that guest does
>>> not boot up. This might be some limitation within the OS, Cedric might
>>> have some clue.
>>
>> Some controllers might need some more tweaking, PSI, LPC, to elect a 
>> master one.
> 
> Uh.. why?

that's not true. I managed to boot a pnv machine with 4 chips/sockets 
each having 4 cpus using a 4.4.9-openpower2 skiroot kernel, from an 
openpower firmare 1.10 I think. Recent openpower kernel must be using 
some new features/instructions that we don't manage well in QEMU.

I would need to build a kernel with more output.

>> Anyhow I don't think we want to deduce the number of chips
>> from the number of cpus. These two figures are very different.
> 
> How so?  It's not totally perfect, but making a single chip correspond
> to a "socket" in qemu's (somewhat x86 centric) terminology is still a
> pretty good match.

well, it would be good to be able to define chips with different
numbers of cpus. That is something will we want to do for sure.

Thanks,

C. 
 

On Fri, Sep 22, 2017 at 12:46:58PM +0200, Cédric Le Goater wrote:
> On 09/22/2017 12:12 PM, David Gibson wrote:
> > On Fri, Sep 22, 2017 at 08:07:06AM +0200, Cédric Le Goater wrote:
> >> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> >>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>
> >>>>>>>
> >>>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >>>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >>>>>>> this:
> >>>>>>>
> >>>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >>>>>>>
> >>>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >>>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >>>>>>> was 1), which is wrong as per the command-line that was provided.
> >>>>>>
> >>>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> >>>>>> thread.  If you can't supply 4 sockets you should error, but you
> >>>>>> shouldn't go and change the number of cores per socket.
> >>>>>
> >>>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> >>>>> case. Now looking more into it, i see that powernv has something called
> >>>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
> >>>>
> >>>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
> >>>> an internal variable, rather than using (smp_cpus / smp_cores /
> >>>> smp_threads) everywhere.  But we shouldn't have it as a direct
> >>>> user-settable property, instead setting it from the -smp command line
> >>>> option.
> >>>
> >>> Something like the below works till num_chips=2, after that guest does
> >>> not boot up. This might be some limitation within the OS, Cedric might
> >>> have some clue.
> >>
> >> Some controllers might need some more tweaking, PSI, LPC, to elect a 
> >> master one.
> > 
> > Uh.. why?
> 
> that's not true. I managed to boot a pnv machine with 4 chips/sockets 
> each having 4 cpus using a 4.4.9-openpower2 skiroot kernel, from an 
> openpower firmare 1.10 I think. Recent openpower kernel must be using 
> some new features/instructions that we don't manage well in QEMU.
> 
> I would need to build a kernel with more output.
> 
> >> Anyhow I don't think we want to deduce the number of chips
> >> from the number of cpus. These two figures are very different.
> > 
> > How so?  It's not totally perfect, but making a single chip correspond
> > to a "socket" in qemu's (somewhat x86 centric) terminology is still a
> > pretty good match.
> 
> well, it would be good to be able to define chips with different
> numbers of cpus. That is something will we want to do for sure.

You mean multiple chips in a single system with non-uniform numbers of
cores?  Are there really such systems in the wild?

-- 
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

>> well, it would be good to be able to define chips with different
>> numbers of cpus. That is something will we want to do for sure.
> 
> You mean multiple chips in a single system with non-uniform numbers of
> cores?  Are there really such systems in the wild?
> 

When CPU fail for some reason, yes. They are garded by the FW and next
boot the system will have different numbers of CPUs on its chip.

C.

On Fri, Sep 22, 2017 at 01:37:39PM +0200, Cédric Le Goater wrote:
> >> well, it would be good to be able to define chips with different
> >> numbers of cpus. That is something will we want to do for sure.
> > 
> > You mean multiple chips in a single system with non-uniform numbers of
> > cores?  Are there really such systems in the wild?
> > 
> 
> When CPU fail for some reason, yes. They are garded by the FW and next
> boot the system will have different numbers of CPUs on its chip.

Ok, but you can model that with max_cpus the full potential number,
then just don't have all of them on initially.  The basic topology
remains uniform.

-- 
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

On Fri, 22 Sep 2017 21:20:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Sep 22, 2017 at 12:46:58PM +0200, Cédric Le Goater wrote:
> > On 09/22/2017 12:12 PM, David Gibson wrote:  
> > > On Fri, Sep 22, 2017 at 08:07:06AM +0200, Cédric Le Goater wrote:  
> > >> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:  
> > >>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>  
> > >>>>>>>
> > >>>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> > >>>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> > >>>>>>> this:
> > >>>>>>>
> > >>>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> > >>>>>>>
> > >>>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> > >>>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> > >>>>>>> was 1), which is wrong as per the command-line that was provided.  
> > >>>>>>
> > >>>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> > >>>>>> thread.  If you can't supply 4 sockets you should error, but you
> > >>>>>> shouldn't go and change the number of cores per socket.  
> > >>>>>
> > >>>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> > >>>>> case. Now looking more into it, i see that powernv has something called
> > >>>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?  
> > >>>>
> > >>>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
> > >>>> an internal variable, rather than using (smp_cpus / smp_cores /
> > >>>> smp_threads) everywhere.  But we shouldn't have it as a direct
> > >>>> user-settable property, instead setting it from the -smp command line
> > >>>> option.  
> > >>>
> > >>> Something like the below works till num_chips=2, after that guest does
> > >>> not boot up. This might be some limitation within the OS, Cedric might
> > >>> have some clue.  
> > >>
> > >> Some controllers might need some more tweaking, PSI, LPC, to elect a 
> > >> master one.  
> > > 
> > > Uh.. why?  
> > 
> > that's not true. I managed to boot a pnv machine with 4 chips/sockets 
> > each having 4 cpus using a 4.4.9-openpower2 skiroot kernel, from an 
> > openpower firmare 1.10 I think. Recent openpower kernel must be using 
> > some new features/instructions that we don't manage well in QEMU.
> > 
> > I would need to build a kernel with more output.
> >   
> > >> Anyhow I don't think we want to deduce the number of chips
> > >> from the number of cpus. These two figures are very different.  
> > > 
> > > How so?  It's not totally perfect, but making a single chip correspond
> > > to a "socket" in qemu's (somewhat x86 centric) terminology is still a
> > > pretty good match.  
> > 
> > well, it would be good to be able to define chips with different
> > numbers of cpus. That is something will we want to do for sure.  
> 
> You mean multiple chips in a single system with non-uniform numbers of
> cores?  Are there really such systems in the wild?
> 

Doesn't it happen when a CPU core gets deconfigured ?

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>>>>>
>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>> this:
>>>>>
>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>>>
>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>
>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>> shouldn't go and change the number of cores per socket.
>>>
>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>> case. Now looking more into it, i see that powernv has something called
>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>
>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
>> an internal variable, rather than using (smp_cpus / smp_cores /
>> smp_threads) everywhere.  But we shouldn't have it as a direct
>> user-settable property, instead setting it from the -smp command line
>> option.
> 
> Something like the below works till num_chips=2, after that guest does
> not boot up. This might be some limitation within the OS, Cedric might
> have some clue. Otherwise, I see that multiple chips are created with
> single core having single thread.
> 
>     ppc/pnv: Use num_chips for multiple sockets
>     
>     When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
>     initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make sure
>     that we initialize multiple chips for this.

-smp 4          would give a machine with 4 sockets with 1 core
-smp 4,cores=4  would give a machine with 1 socket  with 4 cores

correct ?

C.

>     Remove the user-settable property num_chips from machine command line option
>     
>     Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9724719..fa501f9 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -709,7 +709,17 @@ static void ppc_powernv_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    pnv->num_chips = smp_cpus / (smp_cores * smp_threads);
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> +
> +    if (smp_cpus != (smp_threads * pnv->num_chips * smp_cores)) {
> +        error_report("cpu topology not balanced: "
> +                     "chips (%u) * cores (%u) * threads (%u) != "
> +                     "number of cpus (%u)",
> +                     pnv->num_chips, smp_cores, smp_threads, smp_cpus);
> +        exit(1);
> +    }
> +
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
>          Object *chip = object_new(chip_typename);
> @@ -1255,53 +1265,12 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      }
>  }
>  
> -static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp);
> -}
> -
> -static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> -{
> -    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> -    uint32_t num_chips;
> -    Error *local_err = NULL;
> -
> -    visit_type_uint32(v, name, &num_chips, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> -    /*
> -     * TODO: should we decide on how many chips we can create based
> -     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
> -     */
> -    if (!is_power_of_2(num_chips) || num_chips > 4) {
> -        error_setg(errp, "invalid number of chips: '%d'", num_chips);
> -        return;
> -    }
> -
> -    pnv->num_chips = num_chips;
> -}
> -
>  static void powernv_machine_initfn(Object *obj)
>  {
>      PnvMachineState *pnv = POWERNV_MACHINE(obj);
>      pnv->num_chips = 1;
>  }
>  
> -static void powernv_machine_class_props_init(ObjectClass *oc)
> -{
> -    object_class_property_add(oc, "num-chips", "uint32",
> -                              pnv_get_num_chips, pnv_set_num_chips,
> -                              NULL, NULL, NULL);
> -    object_class_property_set_description(oc, "num-chips",
> -                              "Specifies the number of processor chips",
> -                              NULL);
> -}
> -
>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1321,8 +1290,6 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
>      xic->ics_get = pnv_ics_get;
>      xic->ics_resend = pnv_ics_resend;
>      ispc->print_info = pnv_pic_print_info;
> -
> -    powernv_machine_class_props_init(oc);
>  }
>  
>  static const TypeInfo powernv_machine_info = {
> 

Cédric Le Goater <clg@kaod.org> writes:

> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>>>>>>
>>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>>> this:
>>>>>>
>>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>>>>
>>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>>
>>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>>> shouldn't go and change the number of cores per socket.
>>>>
>>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>>> case. Now looking more into it, i see that powernv has something called
>>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>>
>>> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
>>> an internal variable, rather than using (smp_cpus / smp_cores /
>>> smp_threads) everywhere.  But we shouldn't have it as a direct
>>> user-settable property, instead setting it from the -smp command line
>>> option.
>> 
>> Something like the below works till num_chips=2, after that guest does
>> not boot up. This might be some limitation within the OS, Cedric might
>> have some clue. Otherwise, I see that multiple chips are created with
>> single core having single thread.
>> 
>>     ppc/pnv: Use num_chips for multiple sockets
>>     
>>     When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
>>     initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make sure
>>     that we initialize multiple chips for this.
>
> -smp 4          would give a machine with 4 sockets with 1 core
> -smp 4,cores=4  would give a machine with 1 socket  with 4 cores
>
> correct ?

Yes

Regards
Nikunj

On Fri, Sep 22, 2017 at 12:56:40PM +0200, Cédric Le Goater wrote:
> On 09/22/2017 08:00 AM, Nikunj A Dadhania wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> > 
> >>>>>
> >>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >>>>> this:
> >>>>>
> >>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >>>>>
> >>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >>>>> was 1), which is wrong as per the command-line that was provided.
> >>>>
> >>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> >>>> thread.  If you can't supply 4 sockets you should error, but you
> >>>> shouldn't go and change the number of cores per socket.
> >>>
> >>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> >>> case. Now looking more into it, i see that powernv has something called
> >>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
> >>
> >> Ah, yes, I see.  It's probably still reasonable to keep num_chips as
> >> an internal variable, rather than using (smp_cpus / smp_cores /
> >> smp_threads) everywhere.  But we shouldn't have it as a direct
> >> user-settable property, instead setting it from the -smp command line
> >> option.
> > 
> > Something like the below works till num_chips=2, after that guest does
> > not boot up. This might be some limitation within the OS, Cedric might
> > have some clue. Otherwise, I see that multiple chips are created with
> > single core having single thread.
> > 
> >     ppc/pnv: Use num_chips for multiple sockets
> >     
> >     When the user does not provide the cpu topology, e.g. "-smp 4", machine fails to
> >     initialize 4 cpus. QEMU assumes smp_threads and smp_cores both as 1. Make sure
> >     that we initialize multiple chips for this.
> 
> -smp 4          would give a machine with 4 sockets with 1 core
> -smp 4,cores=4  would give a machine with 1 socket  with 4 cores
> 
> correct ?

That's right.

-- 
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

On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>
>>>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>
>>>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>
>>>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>
>>>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
>>>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
>>>>>>>>>>>>>>> (max 2, at the moment)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
>>>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
>>>>>>>>>>>>>> it.  That's just asking for confusion.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
>>>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
>>>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>>>>>>>>>>>>
>>>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
>>>>>>>>>>>> not explicitly specified, then ok.
>>>>>>>>>>>
>>>>>>>>>>> Should I change the global smp_cores here as well ?
>>>>>>>>>>
>>>>>>>>>> I'm pretty uneasy with that option.
>>>>>>>>>
>>>>>>>>> Me too.
>>>>>>>>>
>>>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
>>>>>>>>>> is safe here. An easier to verify option would be to make the generic
>>>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
>>>>>>>>>> more flexible, possibly based on machine options for max values.
>>>>>>>>>>
>>>>>>>>>> That might still be more trouble than its worth.
>>>>>>>>>
>>>>>>>>> I think the current approach is the simplest and less intrusive, as we
>>>>>>>>> are handling a case where user has not bothered to provide a detailed
>>>>>>>>> topology, the best we can do is create single threaded cores equal to
>>>>>>>>> number of cores.
>>>>>>>>
>>>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
>>>>>>>> chip in all cases is just not ok.  Add an error message if the
>>>>>>>> topology isn't workable for powernv by all means.  But users having to
>>>>>>>> use a longer command line is better than breaking basic assumptions
>>>>>>>> about what numbers reflect what topology.
>>>>>>>
>>>>>>> Sorry to ask again, as I am still not convinced, we do similar
>>>>>>> adjustment in spapr where the user did not provide the number of cores,
>>>>>>> but qemu assumes them as single threaded cores and created
>>>>>>> cores(boot_cores_nr) that were not same as smp_cores ?
>>>>>>
>>>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
>>>>>> topology, and it certainly doesn't assume they're single threaded.
>>>>>
>>>>> When we start a TCG guest and user provides following commandline, e.g.
>>>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>>>>> with 4 cores, each having 1 thread.
>>>>
>>>> Ok.. and what's the problem with that behaviour on powernv?
>>>
>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>> this:
>>>
>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>
>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>> was 1), which is wrong as per the command-line that was provided.
>>
>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>> thread.  If you can't supply 4 sockets you should error, but you
>> shouldn't go and change the number of cores per socket.
> 
> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> case. Now looking more into it, i see that powernv has something called
> "num_chips", isnt this same as sockets ? Do we need num_chips separately?

yes that would do for cpus, but how do we retrieve the number of 
sockets ? I don't see a smp_sockets. 

If we start looking at such issues, we should also take into account 
memory distribution :

      -numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]

would allow us to define a set of cpus per node, cpus should be evenly 
distributed on the nodes though, and also define memory per node, but 
some nodes could be without memory.   

C. 

On Thu, 21 Sep 2017 08:04:55 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> >   
> >> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:  
> >>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>  
> >>>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:  
> >>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>  
> >>>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:  
> >>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>  
> >>>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:  
> >>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>  
> >>>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:  
> >>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>>>  
> >>>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:  
> >>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>>>>>  
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
> >>>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
> >>>>>>>>>>>>>>> (max 2, at the moment)
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);  
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
> >>>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
> >>>>>>>>>>>>>>  
> >>>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.  
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
> >>>>>>>>>>>>>> it.  That's just asking for confusion.  
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
> >>>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
> >>>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.  
> >>>>>>>>>>>>
> >>>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
> >>>>>>>>>>>> not explicitly specified, then ok.  
> >>>>>>>>>>>
> >>>>>>>>>>> Should I change the global smp_cores here as well ?  
> >>>>>>>>>>
> >>>>>>>>>> I'm pretty uneasy with that option.  
> >>>>>>>>>
> >>>>>>>>> Me too.
> >>>>>>>>>  
> >>>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
> >>>>>>>>>> is safe here. An easier to verify option would be to make the generic
> >>>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
> >>>>>>>>>> more flexible, possibly based on machine options for max values.
> >>>>>>>>>>
> >>>>>>>>>> That might still be more trouble than its worth.  
> >>>>>>>>>
> >>>>>>>>> I think the current approach is the simplest and less intrusive, as we
> >>>>>>>>> are handling a case where user has not bothered to provide a detailed
> >>>>>>>>> topology, the best we can do is create single threaded cores equal to
> >>>>>>>>> number of cores.  
> >>>>>>>>
> >>>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
> >>>>>>>> chip in all cases is just not ok.  Add an error message if the
> >>>>>>>> topology isn't workable for powernv by all means.  But users having to
> >>>>>>>> use a longer command line is better than breaking basic assumptions
> >>>>>>>> about what numbers reflect what topology.  
> >>>>>>>
> >>>>>>> Sorry to ask again, as I am still not convinced, we do similar
> >>>>>>> adjustment in spapr where the user did not provide the number of cores,
> >>>>>>> but qemu assumes them as single threaded cores and created
> >>>>>>> cores(boot_cores_nr) that were not same as smp_cores ?  
> >>>>>>
> >>>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
> >>>>>> topology, and it certainly doesn't assume they're single threaded.  
> >>>>>
> >>>>> When we start a TCG guest and user provides following commandline, e.g.
> >>>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> >>>>> with 4 cores, each having 1 thread.  
> >>>>
> >>>> Ok.. and what's the problem with that behaviour on powernv?  
> >>>
> >>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >>> this:
> >>>
> >>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >>>
> >>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >>> was 1), which is wrong as per the command-line that was provided.  
> >>
> >> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> >> thread.  If you can't supply 4 sockets you should error, but you
> >> shouldn't go and change the number of cores per socket.  
> > 
> > OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> > case. Now looking more into it, i see that powernv has something called
> > "num_chips", isnt this same as sockets ? Do we need num_chips separately?  
> 
> yes that would do for cpus, but how do we retrieve the number of 
> sockets ? I don't see a smp_sockets. 
I'd suggest to rewrite QEMU again :)

more exactly, -smp parsing is global and sometimes doesn't suite
target device model/machine.
Idea was to make it's options machine properties to get rid of globals
and then let leaf machine redefine parsing behaviour.
here is Drew's take on it:

[Qemu-devel] [PATCH RFC 00/16] Rework SMP parameters
https://www.mail-archive.com/qemu-devel@nongnu.org/msg376961.html

considering there weren't pressing need, the series has been pushed
to the end of TODO list. Maybe you can revive it and make work for
pnv and other machines.


> If we start looking at such issues, we should also take into account 
> memory distribution :
> 
>       -numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]
it's interface based on cpu_index, which internal qemu number for
an 1 cpu execution context.

It would be better if one would use new interface with new machines
 -numa cpu,node-id=0,socket-id=0 ...

> 
> would allow us to define a set of cpus per node, cpus should be evenly 
> distributed on the nodes though, and also define memory per node, but 
> some nodes could be without memory.   
> 
> C. 
> 

On Thu, Sep 21, 2017 at 09:42:26AM +0200, Igor Mammedov wrote:
> On Thu, 21 Sep 2017 08:04:55 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
> > > David Gibson <david@gibson.dropbear.id.au> writes:
> > >   
> > >> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:  
> > >>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>  
> > >>>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:  
> > >>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>>>  
> > >>>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:  
> > >>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>>>>>  
> > >>>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:  
> > >>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>>>>>>>  
> > >>>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:  
> > >>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>>>>>>>>>  
> > >>>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:  
> > >>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> > >>>>>>>>>>>>>  
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
> > >>>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
> > >>>>>>>>>>>>>>> (max 2, at the moment)
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);  
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
> > >>>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
> > >>>>>>>>>>>>>>  
> > >>>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.  
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
> > >>>>>>>>>>>>>> it.  That's just asking for confusion.  
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
> > >>>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
> > >>>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.  
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
> > >>>>>>>>>>>> not explicitly specified, then ok.  
> > >>>>>>>>>>>
> > >>>>>>>>>>> Should I change the global smp_cores here as well ?  
> > >>>>>>>>>>
> > >>>>>>>>>> I'm pretty uneasy with that option.  
> > >>>>>>>>>
> > >>>>>>>>> Me too.
> > >>>>>>>>>  
> > >>>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
> > >>>>>>>>>> is safe here. An easier to verify option would be to make the generic
> > >>>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
> > >>>>>>>>>> more flexible, possibly based on machine options for max values.
> > >>>>>>>>>>
> > >>>>>>>>>> That might still be more trouble than its worth.  
> > >>>>>>>>>
> > >>>>>>>>> I think the current approach is the simplest and less intrusive, as we
> > >>>>>>>>> are handling a case where user has not bothered to provide a detailed
> > >>>>>>>>> topology, the best we can do is create single threaded cores equal to
> > >>>>>>>>> number of cores.  
> > >>>>>>>>
> > >>>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
> > >>>>>>>> chip in all cases is just not ok.  Add an error message if the
> > >>>>>>>> topology isn't workable for powernv by all means.  But users having to
> > >>>>>>>> use a longer command line is better than breaking basic assumptions
> > >>>>>>>> about what numbers reflect what topology.  
> > >>>>>>>
> > >>>>>>> Sorry to ask again, as I am still not convinced, we do similar
> > >>>>>>> adjustment in spapr where the user did not provide the number of cores,
> > >>>>>>> but qemu assumes them as single threaded cores and created
> > >>>>>>> cores(boot_cores_nr) that were not same as smp_cores ?  
> > >>>>>>
> > >>>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
> > >>>>>> topology, and it certainly doesn't assume they're single threaded.  
> > >>>>>
> > >>>>> When we start a TCG guest and user provides following commandline, e.g.
> > >>>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> > >>>>> with 4 cores, each having 1 thread.  
> > >>>>
> > >>>> Ok.. and what's the problem with that behaviour on powernv?  
> > >>>
> > >>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> > >>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> > >>> this:
> > >>>
> > >>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> > >>>
> > >>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> > >>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> > >>> was 1), which is wrong as per the command-line that was provided.  
> > >>
> > >> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> > >> thread.  If you can't supply 4 sockets you should error, but you
> > >> shouldn't go and change the number of cores per socket.  
> > > 
> > > OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> > > case. Now looking more into it, i see that powernv has something called
> > > "num_chips", isnt this same as sockets ? Do we need num_chips separately?  
> > 
> > yes that would do for cpus, but how do we retrieve the number of 
> > sockets ? I don't see a smp_sockets. 
> I'd suggest to rewrite QEMU again :)
> 
> more exactly, -smp parsing is global and sometimes doesn't suite
> target device model/machine.
> Idea was to make it's options machine properties to get rid of globals
> and then let leaf machine redefine parsing behaviour.
> here is Drew's take on it:
> 
> [Qemu-devel] [PATCH RFC 00/16] Rework SMP parameters
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg376961.html
> 
> considering there weren't pressing need, the series has been pushed
> to the end of TODO list. Maybe you can revive it and make work for
> pnv and other machines.

Right, making the core smp parsing more flexible might be good.

-- 
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

On Thu, Sep 21, 2017 at 08:04:55AM +0200, Cédric Le Goater wrote:
> On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> > 
> >> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
> >>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>
> >>>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
> >>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>
> >>>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
> >>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>
> >>>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
> >>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>
> >>>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> >>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>>>
> >>>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
> >>>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
> >>>>>>>>>>>>>>> (max 2, at the moment)
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
> >>>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
> >>>>>>>>>>>>>> it.  That's just asking for confusion.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
> >>>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
> >>>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
> >>>>>>>>>>>>
> >>>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
> >>>>>>>>>>>> not explicitly specified, then ok.
> >>>>>>>>>>>
> >>>>>>>>>>> Should I change the global smp_cores here as well ?
> >>>>>>>>>>
> >>>>>>>>>> I'm pretty uneasy with that option.
> >>>>>>>>>
> >>>>>>>>> Me too.
> >>>>>>>>>
> >>>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
> >>>>>>>>>> is safe here. An easier to verify option would be to make the generic
> >>>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
> >>>>>>>>>> more flexible, possibly based on machine options for max values.
> >>>>>>>>>>
> >>>>>>>>>> That might still be more trouble than its worth.
> >>>>>>>>>
> >>>>>>>>> I think the current approach is the simplest and less intrusive, as we
> >>>>>>>>> are handling a case where user has not bothered to provide a detailed
> >>>>>>>>> topology, the best we can do is create single threaded cores equal to
> >>>>>>>>> number of cores.
> >>>>>>>>
> >>>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
> >>>>>>>> chip in all cases is just not ok.  Add an error message if the
> >>>>>>>> topology isn't workable for powernv by all means.  But users having to
> >>>>>>>> use a longer command line is better than breaking basic assumptions
> >>>>>>>> about what numbers reflect what topology.
> >>>>>>>
> >>>>>>> Sorry to ask again, as I am still not convinced, we do similar
> >>>>>>> adjustment in spapr where the user did not provide the number of cores,
> >>>>>>> but qemu assumes them as single threaded cores and created
> >>>>>>> cores(boot_cores_nr) that were not same as smp_cores ?
> >>>>>>
> >>>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
> >>>>>> topology, and it certainly doesn't assume they're single threaded.
> >>>>>
> >>>>> When we start a TCG guest and user provides following commandline, e.g.
> >>>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
> >>>>> with 4 cores, each having 1 thread.
> >>>>
> >>>> Ok.. and what's the problem with that behaviour on powernv?
> >>>
> >>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> >>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> >>> this:
> >>>
> >>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> >>>
> >>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> >>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> >>> was 1), which is wrong as per the command-line that was provided.
> >>
> >> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> >> thread.  If you can't supply 4 sockets you should error, but you
> >> shouldn't go and change the number of cores per socket.
> > 
> > OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> > case. Now looking more into it, i see that powernv has something called
> > "num_chips", isnt this same as sockets ? Do we need num_chips separately?
> 
> yes that would do for cpus, but how do we retrieve the number of 
> sockets ? I don't see a smp_sockets. 

	# sockets = smp_cpus / smp_threads / smp_cores

Or, if you want the maximum possible number of sockets (for a fully
populated system)
	# sockets = max_cpus / smp_threads / smp_cores
	

> If we start looking at such issues, we should also take into account 
> memory distribution :
> 
>       -numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]
> 
> would allow us to define a set of cpus per node, cpus should be evenly 
> distributed on the nodes though, and also define memory per node, but 
> some nodes could be without memory.

I don't really see what that has to do with anything.  We already have
ways to assign memory or cpus to specific nodes if we want.

-- 
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

On 09/22/2017 12:08 PM, David Gibson wrote:
> On Thu, Sep 21, 2017 at 08:04:55AM +0200, Cédric Le Goater wrote:
>> On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>
>>>> On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote:
>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>
>>>>>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>
>>>>>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>
>>>>>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>>>>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online cores
>>>>>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple chips
>>>>>>>>>>>>>>>>> (max 2, at the moment)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * pnv->num_chips);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* equal
>>>>>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to fix
>>>>>>>>>>>>>>>> it.  That's just asking for confusion.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is the case where the user does not provide a topology(which is a
>>>>>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
>>>>>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
>>>>>>>>>>>>>> not explicitly specified, then ok.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Should I change the global smp_cores here as well ?
>>>>>>>>>>>>
>>>>>>>>>>>> I'm pretty uneasy with that option.
>>>>>>>>>>>
>>>>>>>>>>> Me too.
>>>>>>>>>>>
>>>>>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
>>>>>>>>>>>> is safe here. An easier to verify option would be to make the generic
>>>>>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
>>>>>>>>>>>> more flexible, possibly based on machine options for max values.
>>>>>>>>>>>>
>>>>>>>>>>>> That might still be more trouble than its worth.
>>>>>>>>>>>
>>>>>>>>>>> I think the current approach is the simplest and less intrusive, as we
>>>>>>>>>>> are handling a case where user has not bothered to provide a detailed
>>>>>>>>>>> topology, the best we can do is create single threaded cores equal to
>>>>>>>>>>> number of cores.
>>>>>>>>>>
>>>>>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
>>>>>>>>>> chip in all cases is just not ok.  Add an error message if the
>>>>>>>>>> topology isn't workable for powernv by all means.  But users having to
>>>>>>>>>> use a longer command line is better than breaking basic assumptions
>>>>>>>>>> about what numbers reflect what topology.
>>>>>>>>>
>>>>>>>>> Sorry to ask again, as I am still not convinced, we do similar
>>>>>>>>> adjustment in spapr where the user did not provide the number of cores,
>>>>>>>>> but qemu assumes them as single threaded cores and created
>>>>>>>>> cores(boot_cores_nr) that were not same as smp_cores ?
>>>>>>>>
>>>>>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
>>>>>>>> topology, and it certainly doesn't assume they're single threaded.
>>>>>>>
>>>>>>> When we start a TCG guest and user provides following commandline, e.g.
>>>>>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>>>>>>> with 4 cores, each having 1 thread.
>>>>>>
>>>>>> Ok.. and what's the problem with that behaviour on powernv?
>>>>>
>>>>> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
>>>>> default value of 1 in vl.c. In powernv, we were setting nr-cores like
>>>>> this:
>>>>>
>>>>>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
>>>>>
>>>>> Even when there were multiple cpus (-smp 4), when the guest boots up, we
>>>>> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
>>>>> was 1), which is wrong as per the command-line that was provided.
>>>>
>>>> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
>>>> thread.  If you can't supply 4 sockets you should error, but you
>>>> shouldn't go and change the number of cores per socket.
>>>
>>> OK, that makes sense now. And I do see that smp_cpus is 4 in the above
>>> case. Now looking more into it, i see that powernv has something called
>>> "num_chips", isnt this same as sockets ? Do we need num_chips separately?
>>
>> yes that would do for cpus, but how do we retrieve the number of 
>> sockets ? I don't see a smp_sockets. 
> 
> 	# sockets = smp_cpus / smp_threads / smp_cores
> 
> Or, if you want the maximum possible number of sockets (for a fully
> populated system)
> 	# sockets = max_cpus / smp_threads / smp_cores
ok. that would do for a default setting. Sorry for the noise.

>> If we start looking at such issues, we should also take into account 
>> memory distribution :
>>
>>       -numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]
>>
>> would allow us to define a set of cpus per node, cpus should be evenly 
>> distributed on the nodes though, and also define memory per node, but 
>> some nodes could be without memory.
> 
> I don't really see what that has to do with anything.  We already have
> ways to assign memory or cpus to specific nodes if we want.

We will see when the needs come if numa options fit the requirements.

C.