| Message ID | 1535553121-80352-1-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl:c: make sure that sockets are calculated correctly in '-smp X' case | expand |
On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > commit > (5cdc9b76e3 vl.c: Remove dead assignment) > removed sockets calculation when 'sockets' weren't provided on CLI > since there wasn't any users for it back then. Exiting checks > are neither reachable > } else if (sockets * cores * threads < cpus) { > or nor triggable > if (sockets * cores * threads > max_cpus) > so we weren't noticing wrong topology since then, since users > recalculate sockets adhoc on their own. > > However with deprecation check it becomes noticable, for example > -smp 2 > will start printing warning: > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > calculating sockets if they weren't specified. > > Fix it by returning back sockets calculation if it's omited on CLI. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > vl.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/vl.c b/vl.c > index 7fd700e..333d638 100644 > --- a/vl.c > +++ b/vl.c > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > /* compute missing values, prefer sockets over cores over threads */ > if (cpus == 0 || sockets == 0) { > - sockets = sockets > 0 ? sockets : 1; > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > if (cpus == 0) { > + sockets = sockets > 0 ? sockets : 1; > cpus = cores * threads * sockets; > + } else { > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); We already have a "max_cpus = qemu_opt_get_number(...)" line in this function[1]: /* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; if (cpus == 0) { cpus = cores * threads * sockets; } } else if (cores == 0) { [...] } else if (threads == 0) { [...] } else if (sockets * cores * threads < cpus) { [...] } max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ /* [2] */ if (max_cpus < cpus) { error_report("maxcpus must be equal to or greater than smp"); exit(1); } Why not move the sockets == 0 check to [2]? > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; The two patches in this thread make QEMU print a warning on a case that was never documented as invalid/deprecated: maxcpus now needs to be a multiple of cores*threads. However, the error message doesn't make it obvious: $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) I think this would make more sense: $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) > } > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > -- > 2.7.4 >
On Wed, 29 Aug 2018 14:33:01 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > > commit > > (5cdc9b76e3 vl.c: Remove dead assignment) > > removed sockets calculation when 'sockets' weren't provided on CLI > > since there wasn't any users for it back then. Exiting checks > > are neither reachable > > } else if (sockets * cores * threads < cpus) { > > or nor triggable > > if (sockets * cores * threads > max_cpus) > > so we weren't noticing wrong topology since then, since users > > recalculate sockets adhoc on their own. > > > > However with deprecation check it becomes noticable, for example > > -smp 2 > > will start printing warning: > > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > > calculating sockets if they weren't specified. > > > > Fix it by returning back sockets calculation if it's omited on CLI. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > vl.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/vl.c b/vl.c > > index 7fd700e..333d638 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > > > /* compute missing values, prefer sockets over cores over threads */ > > if (cpus == 0 || sockets == 0) { > > - sockets = sockets > 0 ? sockets : 1; > > cores = cores > 0 ? cores : 1; > > threads = threads > 0 ? threads : 1; > > if (cpus == 0) { > > + sockets = sockets > 0 ? sockets : 1; > > cpus = cores * threads * sockets; > > + } else { > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > We already have a "max_cpus = qemu_opt_get_number(...)" line in > this function[1]: > > /* compute missing values, prefer sockets over cores over threads */ > if (cpus == 0 || sockets == 0) { > sockets = sockets > 0 ? sockets : 1; > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > if (cpus == 0) { > cpus = cores * threads * sockets; > } > } else if (cores == 0) { > [...] > } else if (threads == 0) { > [...] > } else if (sockets * cores * threads < cpus) { > [...] > } > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ > > /* [2] */ > > if (max_cpus < cpus) { > error_report("maxcpus must be equal to or greater than smp"); > exit(1); > } > > Why not move the sockets == 0 check to [2]? it won't work, since the rest 'else if' depends on sockets being non 0 which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2 equivalent conditions. It's more code but it might be easier to read diff --git a/vl.c b/vl.c index 7fd700e..6320bdc 100644 --- a/vl.c +++ b/vl.c @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts) unsigned cores = qemu_opt_get_number(opts, "cores", 0); unsigned threads = qemu_opt_get_number(opts, "threads", 0); + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* compute missing values, prefer sockets over cores over threads */ - if (cpus == 0 || sockets == 0) { + if (cpus == 0) { sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; - if (cpus == 0) { - cpus = cores * threads * sockets; - } + cpus = cores * threads * sockets; + max_cpus = max_cpus > 0 ? max_cpus : cpus; + } else if (sockets == 0) { + cores = cores > 0 ? cores : 1; + threads = threads > 0 ? threads : 1; + sockets = max_cpus / (cores * threads); } else if (cores == 0) { threads = threads > 0 ? threads : 1; cores = cpus / (sockets * threads); @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts) exit(1); } - max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); - if (max_cpus < cpus) { error_report("maxcpus must be equal to or greater than smp"); exit(1); > > > > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > The two patches in this thread make QEMU print a warning on a > case that was never documented as invalid/deprecated: maxcpus now > needs to be a multiple of cores*threads. > > However, the error message doesn't make it obvious: > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) to me it look pretty obvious, fix one side of equation to make sure that VM starts in the future. > I think this would make more sense: > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) I think it will add not necessary warning/code, but if you really think we need it, I can add it to 'vl.c deprecate incorrect CPUs topology' on respin. > > > > } > > } else if (cores == 0) { > > threads = threads > 0 ? threads : 1; > > -- > > 2.7.4 > > >
On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote: > On Wed, 29 Aug 2018 14:33:01 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > > > commit > > > (5cdc9b76e3 vl.c: Remove dead assignment) > > > removed sockets calculation when 'sockets' weren't provided on CLI > > > since there wasn't any users for it back then. Exiting checks > > > are neither reachable > > > } else if (sockets * cores * threads < cpus) { > > > or nor triggable > > > if (sockets * cores * threads > max_cpus) > > > so we weren't noticing wrong topology since then, since users > > > recalculate sockets adhoc on their own. > > > > > > However with deprecation check it becomes noticable, for example > > > -smp 2 > > > will start printing warning: > > > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > > > calculating sockets if they weren't specified. > > > > > > Fix it by returning back sockets calculation if it's omited on CLI. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > vl.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/vl.c b/vl.c > > > index 7fd700e..333d638 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > if (cpus == 0 || sockets == 0) { > > > - sockets = sockets > 0 ? sockets : 1; > > > cores = cores > 0 ? cores : 1; > > > threads = threads > 0 ? threads : 1; > > > if (cpus == 0) { > > > + sockets = sockets > 0 ? sockets : 1; > > > cpus = cores * threads * sockets; > > > + } else { > > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > > > We already have a "max_cpus = qemu_opt_get_number(...)" line in > > this function[1]: > > > > /* compute missing values, prefer sockets over cores over threads */ > > if (cpus == 0 || sockets == 0) { > > sockets = sockets > 0 ? sockets : 1; > > cores = cores > 0 ? cores : 1; > > threads = threads > 0 ? threads : 1; > > if (cpus == 0) { > > cpus = cores * threads * sockets; > > } > > } else if (cores == 0) { > > [...] > > } else if (threads == 0) { > > [...] > > } else if (sockets * cores * threads < cpus) { > > [...] > > } > > > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ > > > > /* [2] */ > > > > if (max_cpus < cpus) { > > error_report("maxcpus must be equal to or greater than smp"); > > exit(1); > > } > > > > Why not move the sockets == 0 check to [2]? > it won't work, since the rest 'else if' depends on sockets being non 0 > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2 > equivalent conditions. It's more code but it might be easier to read > > diff --git a/vl.c b/vl.c > index 7fd700e..6320bdc 100644 > --- a/vl.c > +++ b/vl.c > @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts) > unsigned cores = qemu_opt_get_number(opts, "cores", 0); > unsigned threads = qemu_opt_get_number(opts, "threads", 0); > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > /* compute missing values, prefer sockets over cores over threads */ > - if (cpus == 0 || sockets == 0) { > + if (cpus == 0) { > sockets = sockets > 0 ? sockets : 1; > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > - if (cpus == 0) { > - cpus = cores * threads * sockets; > - } > + cpus = cores * threads * sockets; > + max_cpus = max_cpus > 0 ? max_cpus : cpus; > + } else if (sockets == 0) { > + cores = cores > 0 ? cores : 1; > + threads = threads > 0 ? threads : 1; > + sockets = max_cpus / (cores * threads); > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > cores = cpus / (sockets * threads); > @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts) > exit(1); > } > > - max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > - > if (max_cpus < cpus) { > error_report("maxcpus must be equal to or greater than smp"); > exit(1); > I once made a cleanup similar to the above, but with more more line removal. It doesn't look like I ever posted it, and I don't recall how much I tested it, but here it is diff --git a/vl.c b/vl.c index 5ba06adf787b..ad4e389fde48 100644 --- a/vl.c +++ b/vl.c @@ -1208,30 +1208,22 @@ static void smp_parse(QemuOpts *opts) unsigned cores = qemu_opt_get_number(opts, "cores", 0); unsigned threads = qemu_opt_get_number(opts, "threads", 0); + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); + /* compute missing values, prefer sockets over cores over threads */ - if (cpus == 0 || sockets == 0) { - sockets = sockets > 0 ? sockets : 1; + if (sockets == 0) { cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; - if (cpus == 0) { - cpus = cores * threads * sockets; - } + sockets = max_cpus > 0 ? max_cpus / (cores * threads) : 1; } else if (cores == 0) { threads = threads > 0 ? threads : 1; - cores = cpus / (sockets * threads); - cores = cores > 0 ? cores : 1; + cores = max_cpus > 0 ? max_cpus / (sockets * threads) : 1; } else if (threads == 0) { - threads = cpus / (cores * sockets); - threads = threads > 0 ? threads : 1; - } else if (sockets * cores * threads < cpus) { - error_report("cpu topology: " - "sockets (%u) * cores (%u) * threads (%u) < " - "smp_cpus (%u)", - sockets, cores, threads, cpus); - exit(1); + threads = max_cpus > 0 ? max_cpus / (sockets * cores) : 1; } - max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); + max_cpus = max_cpus ?: sockets * cores * threads; + cpus = cpus ?: max_cpus; if (max_cpus < cpus) { error_report("maxcpus must be equal to or greater than smp"); Thanks, drew > > > > > > > > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > > > The two patches in this thread make QEMU print a warning on a > > case that was never documented as invalid/deprecated: maxcpus now > > needs to be a multiple of cores*threads. > > > > However, the error message doesn't make it obvious: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) > to me it look pretty obvious, fix one side of equation to make sure that VM > starts in the future. > > > I think this would make more sense: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) > I think it will add not necessary warning/code, but if you really > think we need it, I can add it to 'vl.c deprecate incorrect CPUs > topology' on respin. > > > > > > > > } > > > } else if (cores == 0) { > > > threads = threads > 0 ? threads : 1; > > > -- > > > 2.7.4 > > > > > > >
On Thu, 30 Aug 2018 11:08:53 +0200 Andrew Jones <drjones@redhat.com> wrote: > On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote: > > On Wed, 29 Aug 2018 14:33:01 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > > > > commit > > > > (5cdc9b76e3 vl.c: Remove dead assignment) > > > > removed sockets calculation when 'sockets' weren't provided on CLI > > > > since there wasn't any users for it back then. Exiting checks > > > > are neither reachable > > > > } else if (sockets * cores * threads < cpus) { > > > > or nor triggable > > > > if (sockets * cores * threads > max_cpus) > > > > so we weren't noticing wrong topology since then, since users > > > > recalculate sockets adhoc on their own. > > > > > > > > However with deprecation check it becomes noticable, for example > > > > -smp 2 > > > > will start printing warning: > > > > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > > > > calculating sockets if they weren't specified. > > > > > > > > Fix it by returning back sockets calculation if it's omited on CLI. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > vl.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/vl.c b/vl.c > > > > index 7fd700e..333d638 100644 > > > > --- a/vl.c > > > > +++ b/vl.c > > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > > if (cpus == 0 || sockets == 0) { > > > > - sockets = sockets > 0 ? sockets : 1; > > > > cores = cores > 0 ? cores : 1; > > > > threads = threads > 0 ? threads : 1; > > > > if (cpus == 0) { > > > > + sockets = sockets > 0 ? sockets : 1; > > > > cpus = cores * threads * sockets; > > > > + } else { > > > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > > > > > We already have a "max_cpus = qemu_opt_get_number(...)" line in > > > this function[1]: > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > if (cpus == 0 || sockets == 0) { > > > sockets = sockets > 0 ? sockets : 1; > > > cores = cores > 0 ? cores : 1; > > > threads = threads > 0 ? threads : 1; > > > if (cpus == 0) { > > > cpus = cores * threads * sockets; > > > } > > > } else if (cores == 0) { > > > [...] > > > } else if (threads == 0) { > > > [...] > > > } else if (sockets * cores * threads < cpus) { > > > [...] > > > } > > > > > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ > > > > > > /* [2] */ > > > > > > if (max_cpus < cpus) { > > > error_report("maxcpus must be equal to or greater than smp"); > > > exit(1); > > > } > > > > > > Why not move the sockets == 0 check to [2]? > > it won't work, since the rest 'else if' depends on sockets being non 0 > > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2 > > equivalent conditions. It's more code but it might be easier to read > > > > diff --git a/vl.c b/vl.c > > index 7fd700e..6320bdc 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts) > > unsigned cores = qemu_opt_get_number(opts, "cores", 0); > > unsigned threads = qemu_opt_get_number(opts, "threads", 0); > > > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > /* compute missing values, prefer sockets over cores over threads */ > > - if (cpus == 0 || sockets == 0) { > > + if (cpus == 0) { > > sockets = sockets > 0 ? sockets : 1; > > cores = cores > 0 ? cores : 1; > > threads = threads > 0 ? threads : 1; > > - if (cpus == 0) { > > - cpus = cores * threads * sockets; > > - } > > + cpus = cores * threads * sockets; > > + max_cpus = max_cpus > 0 ? max_cpus : cpus; > > + } else if (sockets == 0) { > > + cores = cores > 0 ? cores : 1; > > + threads = threads > 0 ? threads : 1; > > + sockets = max_cpus / (cores * threads); > > } else if (cores == 0) { > > threads = threads > 0 ? threads : 1; > > cores = cpus / (sockets * threads); > > @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts) > > exit(1); > > } > > > > - max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > - > > if (max_cpus < cpus) { > > error_report("maxcpus must be equal to or greater than smp"); > > exit(1); > > > > I once made a cleanup similar to the above, but with more more line > removal. It doesn't look like I ever posted it, and I don't recall > how much I tested it, but here it is my main motivation touching this code is to get in deprecation notice with as little disruption as possible, and when it expires it should be easier to cleanup as we won't have to worry about invalid then variants. > diff --git a/vl.c b/vl.c > index 5ba06adf787b..ad4e389fde48 100644 > --- a/vl.c > +++ b/vl.c > @@ -1208,30 +1208,22 @@ static void smp_parse(QemuOpts *opts) > unsigned cores = qemu_opt_get_number(opts, "cores", 0); > unsigned threads = qemu_opt_get_number(opts, "threads", 0); > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > + > /* compute missing values, prefer sockets over cores over threads */ > - if (cpus == 0 || sockets == 0) { > - sockets = sockets > 0 ? sockets : 1; > + if (sockets == 0) { > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > - if (cpus == 0) { > - cpus = cores * threads * sockets; > - } > + sockets = max_cpus > 0 ? max_cpus / (cores * threads) : 1; > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > - cores = cpus / (sockets * threads); > - cores = cores > 0 ? cores : 1; > + cores = max_cpus > 0 ? max_cpus / (sockets * threads) : 1; Is it subject to the same issue why 'cores = cores > 0 ? cores : 1;' where added? Also s/cpus/maxcpus/ is not equivalent, considering cores are not discarded like sockets > } else if (threads == 0) { > - threads = cpus / (cores * sockets); > - threads = threads > 0 ? threads : 1; > - } else if (sockets * cores * threads < cpus) { > - error_report("cpu topology: " > - "sockets (%u) * cores (%u) * threads (%u) < " > - "smp_cpus (%u)", > - sockets, cores, threads, cpus); > - exit(1); > + threads = max_cpus > 0 ? max_cpus / (sockets * cores) : 1; ditto > } > > - max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > + max_cpus = max_cpus ?: sockets * cores * threads; > + cpus = cpus ?: max_cpus; > > if (max_cpus < cpus) { > error_report("maxcpus must be equal to or greater than smp"); > > > Thanks, > drew > > > > > > > > > > > > > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > > > > > The two patches in this thread make QEMU print a warning on a > > > case that was never documented as invalid/deprecated: maxcpus now > > > needs to be a multiple of cores*threads. > > > > > > However, the error message doesn't make it obvious: > > > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) > > to me it look pretty obvious, fix one side of equation to make sure that VM > > starts in the future. > > > > > I think this would make more sense: > > > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) > > I think it will add not necessary warning/code, but if you really > > think we need it, I can add it to 'vl.c deprecate incorrect CPUs > > topology' on respin. > > > > > > > > > > > > } > > > > } else if (cores == 0) { > > > > threads = threads > 0 ? threads : 1; > > > > -- > > > > 2.7.4 > > > > > > > > > > >
On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote: > On Wed, 29 Aug 2018 14:33:01 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > > > commit > > > (5cdc9b76e3 vl.c: Remove dead assignment) > > > removed sockets calculation when 'sockets' weren't provided on CLI > > > since there wasn't any users for it back then. Exiting checks > > > are neither reachable > > > } else if (sockets * cores * threads < cpus) { > > > or nor triggable > > > if (sockets * cores * threads > max_cpus) > > > so we weren't noticing wrong topology since then, since users > > > recalculate sockets adhoc on their own. > > > > > > However with deprecation check it becomes noticable, for example > > > -smp 2 > > > will start printing warning: > > > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > > > calculating sockets if they weren't specified. > > > > > > Fix it by returning back sockets calculation if it's omited on CLI. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > vl.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/vl.c b/vl.c > > > index 7fd700e..333d638 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > if (cpus == 0 || sockets == 0) { > > > - sockets = sockets > 0 ? sockets : 1; > > > cores = cores > 0 ? cores : 1; > > > threads = threads > 0 ? threads : 1; > > > if (cpus == 0) { > > > + sockets = sockets > 0 ? sockets : 1; > > > cpus = cores * threads * sockets; > > > + } else { > > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > > > We already have a "max_cpus = qemu_opt_get_number(...)" line in > > this function[1]: > > > > /* compute missing values, prefer sockets over cores over threads */ > > if (cpus == 0 || sockets == 0) { > > sockets = sockets > 0 ? sockets : 1; > > cores = cores > 0 ? cores : 1; > > threads = threads > 0 ? threads : 1; > > if (cpus == 0) { > > cpus = cores * threads * sockets; > > } > > } else if (cores == 0) { > > [...] > > } else if (threads == 0) { > > [...] > > } else if (sockets * cores * threads < cpus) { > > [...] > > } > > > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ > > > > /* [2] */ > > > > if (max_cpus < cpus) { > > error_report("maxcpus must be equal to or greater than smp"); > > exit(1); > > } > > > > Why not move the sockets == 0 check to [2]? > it won't work, since the rest 'else if' depends on sockets being non 0 > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2 > equivalent conditions. It's more code but it might be easier to read I like your original patch better. I wasn't thinking about the "cpus == 0 || sockets == 0" check, but about this line: sockets = !sockets ? max_cpus / (cores * threads) : sockets; Just moving this line to [2] would be enough, wouldn't it? > [...] > > > > > > > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > > > The two patches in this thread make QEMU print a warning on a > > case that was never documented as invalid/deprecated: maxcpus now > > needs to be a multiple of cores*threads. > > > > However, the error message doesn't make it obvious: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) > to me it look pretty obvious, fix one side of equation to make sure that VM > starts in the future. > > > I think this would make more sense: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) > I think it will add not necessary warning/code, but if you really > think we need it, I can add it to 'vl.c deprecate incorrect CPUs > topology' on respin. My main worry is about insufficient documentation: we need to clarify that the s*c*t == maxcpus rule applies even if "sockets" is omitted in the command-line, and that this indirectly requires that maxcpus be a multiple of c*6. That said, I won't mind if you keep the existing message in this patch for code simplicity, as long as the documentation is updated. > > > > > > > > } > > > } else if (cores == 0) { > > > threads = threads > 0 ? threads : 1; > > > -- > > > 2.7.4 > > > > > >
On Thu, 30 Aug 2018 11:08:00 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote: > > On Wed, 29 Aug 2018 14:33:01 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote: > > > > commit > > > > (5cdc9b76e3 vl.c: Remove dead assignment) > > > > removed sockets calculation when 'sockets' weren't provided on CLI > > > > since there wasn't any users for it back then. Exiting checks > > > > are neither reachable > > > > } else if (sockets * cores * threads < cpus) { > > > > or nor triggable > > > > if (sockets * cores * threads > max_cpus) > > > > so we weren't noticing wrong topology since then, since users > > > > recalculate sockets adhoc on their own. > > > > > > > > However with deprecation check it becomes noticable, for example > > > > -smp 2 > > > > will start printing warning: > > > > "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" > > > > calculating sockets if they weren't specified. > > > > > > > > Fix it by returning back sockets calculation if it's omited on CLI. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > vl.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/vl.c b/vl.c > > > > index 7fd700e..333d638 100644 > > > > --- a/vl.c > > > > +++ b/vl.c > > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) > > > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > > if (cpus == 0 || sockets == 0) { > > > > - sockets = sockets > 0 ? sockets : 1; > > > > cores = cores > 0 ? cores : 1; > > > > threads = threads > 0 ? threads : 1; > > > > if (cpus == 0) { > > > > + sockets = sockets > 0 ? sockets : 1; > > > > cpus = cores * threads * sockets; > > > > + } else { > > > > + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); > > > > > > We already have a "max_cpus = qemu_opt_get_number(...)" line in > > > this function[1]: > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > if (cpus == 0 || sockets == 0) { > > > sockets = sockets > 0 ? sockets : 1; > > > cores = cores > 0 ? cores : 1; > > > threads = threads > 0 ? threads : 1; > > > if (cpus == 0) { > > > cpus = cores * threads * sockets; > > > } > > > } else if (cores == 0) { > > > [...] > > > } else if (threads == 0) { > > > [...] > > > } else if (sockets * cores * threads < cpus) { > > > [...] > > > } > > > > > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */ > > > > > > /* [2] */ > > > > > > if (max_cpus < cpus) { > > > error_report("maxcpus must be equal to or greater than smp"); > > > exit(1); > > > } > > > > > > Why not move the sockets == 0 check to [2]? > > it won't work, since the rest 'else if' depends on sockets being non 0 > > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2 > > equivalent conditions. It's more code but it might be easier to read > > I like your original patch better. I wasn't thinking about the > "cpus == 0 || sockets == 0" check, but about this line: > > sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > Just moving this line to [2] would be enough, wouldn't it? it would be, but to me it looks more confusing, since one goes into a branch on condition sockets == 0 but then does nothing with socket. I'd rather have a duplicate qemu_opt_get_number(opts, "maxcpus", cpus) at the time when we have all date to set it instead of deferring it to the later time. Once deprecation expires we will be able to clean it all up as Drew suggested. > [...] > > > > > > > > > > + sockets = !sockets ? max_cpus / (cores * threads) : sockets; > > > > > > The two patches in this thread make QEMU print a warning on a > > > case that was never documented as invalid/deprecated: maxcpus now > > > needs to be a multiple of cores*threads. > > > > > > However, the error message doesn't make it obvious: > > > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7) > > to me it look pretty obvious, fix one side of equation to make sure that VM > > starts in the future. > > > > > I think this would make more sense: > > > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3 > > > qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1) > > I think it will add not necessary warning/code, but if you really > > think we need it, I can add it to 'vl.c deprecate incorrect CPUs > > topology' on respin. > > My main worry is about insufficient documentation: we need to > clarify that the s*c*t == maxcpus rule applies even if "sockets" > is omitted in the command-line, and that this indirectly requires > that maxcpus be a multiple of c*6. It's not limited to c*t but to any combination, how about adding following: " Note: it's assumed that maxcpus value must be multiple of the topology options present on command line to avoid creating an invalid topology. If maxcpus isn't be multiple of present topology options then the condition (sockets * cores * threads == maxcpus) can't be satisfied and it will trigger deprecation warning which later will be converted to a error. If you get deprecation warning it's recommended to explicitly specify a correct topology to make warning go away and ensure that it will continue working in the future. " > > That said, I won't mind if you keep the existing message in this > patch for code simplicity, as long as the documentation is > updated. > > > > > > > > > > > > > } > > > > } else if (cores == 0) { > > > > threads = threads > 0 ? threads : 1; > > > > -- > > > > 2.7.4 > > > > > > > > > >
diff --git a/vl.c b/vl.c index 7fd700e..333d638 100644 --- a/vl.c +++ b/vl.c @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) /* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { - sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; if (cpus == 0) { + sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * sockets; + } else { + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); + sockets = !sockets ? max_cpus / (cores * threads) : sockets; } } else if (cores == 0) { threads = threads > 0 ? threads : 1;
commit (5cdc9b76e3 vl.c: Remove dead assignment) removed sockets calculation when 'sockets' weren't provided on CLI since there wasn't any users for it back then. Exiting checks are neither reachable } else if (sockets * cores * threads < cpus) { or nor triggable if (sockets * cores * threads > max_cpus) so we weren't noticing wrong topology since then, since users recalculate sockets adhoc on their own. However with deprecation check it becomes noticable, for example -smp 2 will start printing warning: "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" calculating sockets if they weren't specified. Fix it by returning back sockets calculation if it's omited on CLI. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- vl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)