| Message ID | 20220902075531.188916-2-pmorel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: CPU Topology | expand |
Quoting Pierre Morel (2022-09-02 09:55:22) > S390x do not support multithreading in the guest. > Do not let admin falsely specify multithreading on QEMU > smp commandline. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 70229b102b..b5ca154e2f 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) > MachineClass *mc = MACHINE_GET_CLASS(machine); > int i; > > + /* Explicitely do not support threads */ ^ Explicitly > + assert(machine->smp.threads == 1); It might be nicer to give a better error message to the user. What do you think about something like (broken whitespace ahead): if (machine->smp.threads != 1) { error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); return; }
On 9/5/22 13:32, Nico Boehr wrote: > Quoting Pierre Morel (2022-09-02 09:55:22) >> S390x do not support multithreading in the guest. >> Do not let admin falsely specify multithreading on QEMU >> smp commandline. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 70229b102b..b5ca154e2f 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >> MachineClass *mc = MACHINE_GET_CLASS(machine); >> int i; >> >> + /* Explicitely do not support threads */ > ^ > Explicitly > >> + assert(machine->smp.threads == 1); > > It might be nicer to give a better error message to the user. > What do you think about something like (broken whitespace ahead): > > if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { > error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); > return; > } > OK, I think I wanted to do this and I changed my mind, obviously, I do not recall why. I will do almost the same but after a look at error.h I will use error_report()/exit() instead of error_setg()/return as in: + /* Explicitly do not support threads */ + if (machine->smp.threads != 1) { + error_report("More than one thread specified, but multithreading unsupported"); + exit(1); + } Thanks, Regards, Pierre
On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote: > > On 9/5/22 13:32, Nico Boehr wrote: > > Quoting Pierre Morel (2022-09-02 09:55:22) > > > S390x do not support multithreading in the guest. > > > Do not let admin falsely specify multithreading on QEMU > > > smp commandline. > > > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > > --- > > > hw/s390x/s390-virtio-ccw.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > > > index 70229b102b..b5ca154e2f 100644 > > > --- a/hw/s390x/s390-virtio-ccw.c > > > +++ b/hw/s390x/s390-virtio-ccw.c > > > @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) > > > MachineClass *mc = MACHINE_GET_CLASS(machine); > > > int i; > > > > > > + /* Explicitely do not support threads */ > > ^ > > Explicitly > > > > > + assert(machine->smp.threads == 1); > > > > It might be nicer to give a better error message to the user. > > What do you think about something like (broken whitespace ahead): > > > > if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { > > error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); > > return; > > } > > > > > OK, I think I wanted to do this and I changed my mind, obviously, I do > not recall why. > I will do almost the same but after a look at error.h I will use > error_report()/exit() instead of error_setg()/return as in: > > > + /* Explicitly do not support threads */ > + if (machine->smp.threads != 1) { > + error_report("More than one thread specified, but > multithreading unsupported"); > + exit(1); > + } I agree that an assert is not a good solution, and I'm not sure aborting is a good idea either. I'm assuming that currently if you specify threads > 0 qemu will run with the number of CPUs multiplied by threads (compared to threads=1). If that is true, then a new qemu version will break existing invocations. An alternative would be to print a warning and do: cores *= threads threads = 1 The questions would be what the best place to do that is. I guess we'd need a new compat variable if that's done in machine-smp.c > > > Thanks, > > Regards, > Pierre >
On 9/5/22 17:23, Janis Schoetterl-Glausch wrote: > On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote: >> >> On 9/5/22 13:32, Nico Boehr wrote: >>> Quoting Pierre Morel (2022-09-02 09:55:22) >>>> S390x do not support multithreading in the guest. >>>> Do not let admin falsely specify multithreading on QEMU >>>> smp commandline. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index 70229b102b..b5ca154e2f 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >>>> MachineClass *mc = MACHINE_GET_CLASS(machine); >>>> int i; >>>> >>>> + /* Explicitely do not support threads */ >>> ^ >>> Explicitly >>> >>>> + assert(machine->smp.threads == 1); >>> >>> It might be nicer to give a better error message to the user. >>> What do you think about something like (broken whitespace ahead): >>> >>> if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { >>> error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); >>> return; >>> } >>> >> >> >> OK, I think I wanted to do this and I changed my mind, obviously, I do >> not recall why. >> I will do almost the same but after a look at error.h I will use >> error_report()/exit() instead of error_setg()/return as in: >> >> >> + /* Explicitly do not support threads */ >> + if (machine->smp.threads != 1) { >> + error_report("More than one thread specified, but >> multithreading unsupported"); >> + exit(1); >> + } > > I agree that an assert is not a good solution, and I'm not sure > aborting is a good idea either. > I'm assuming that currently if you specify threads > 0 qemu will run > with the number of CPUs multiplied by threads (compared to threads=1). > If that is true, then a new qemu version will break existing > invocations. > > An alternative would be to print a warning and do: > cores *= threads > threads = 1 > > The questions would be what the best place to do that is. > I guess we'd need a new compat variable if that's done in machine-smp.c Right, I think we can use the new "topology_disable" machine property. >> >> >> Thanks, >> >> Regards, >> Pierre >> >
On 9/5/22 17:10, Pierre Morel wrote: > > > On 9/5/22 13:32, Nico Boehr wrote: >> Quoting Pierre Morel (2022-09-02 09:55:22) >>> S390x do not support multithreading in the guest. >>> Do not let admin falsely specify multithreading on QEMU >>> smp commandline. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index 70229b102b..b5ca154e2f 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >>> MachineClass *mc = MACHINE_GET_CLASS(machine); >>> int i; >>> + /* Explicitely do not support threads */ >> ^ >> Explicitly >> >>> + assert(machine->smp.threads == 1); >> >> It might be nicer to give a better error message to the user. >> What do you think about something like (broken whitespace ahead): >> >> if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { >> error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); >> return; >> } >> > > > OK, I think I wanted to do this and I changed my mind, obviously, I do not recall why. > I will do almost the same but after a look at error.h I will use error_report()/exit() instead of error_setg()/return as in: > > > + /* Explicitly do not support threads */ > + if (machine->smp.threads != 1) { > + error_report("More than one thread specified, but multithreading unsupported"); > + exit(1); > + } or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg() as initially proposed. s390x_new_cpu() would benefit from it also. Thanks, C. > > > Thanks, > > Regards, > Pierre >
On 9/27/22 11:44, Cédric Le Goater wrote: > On 9/5/22 17:10, Pierre Morel wrote: >> >> >> On 9/5/22 13:32, Nico Boehr wrote: >>> Quoting Pierre Morel (2022-09-02 09:55:22) >>>> S390x do not support multithreading in the guest. >>>> Do not let admin falsely specify multithreading on QEMU >>>> smp commandline. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index 70229b102b..b5ca154e2f 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >>>> MachineClass *mc = MACHINE_GET_CLASS(machine); >>>> int i; >>>> + /* Explicitely do not support threads */ >>> ^ >>> Explicitly >>> >>>> + assert(machine->smp.threads == 1); >>> >>> It might be nicer to give a better error message to the user. >>> What do you think about something like (broken whitespace ahead): >>> >>> if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { >>> error_setg(&error_fatal, "More than one thread specified, >>> but multithreading unsupported"); >>> return; >>> } >>> >> >> >> OK, I think I wanted to do this and I changed my mind, obviously, I do >> not recall why. >> I will do almost the same but after a look at error.h I will use >> error_report()/exit() instead of error_setg()/return as in: >> >> >> + /* Explicitly do not support threads */ >> + if (machine->smp.threads != 1) { >> + error_report("More than one thread specified, but >> multithreading unsupported"); >> + exit(1); >> + } > > > or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg() > as initially proposed. s390x_new_cpu() would benefit from it also. > OK, Thanks, Pierre
More thinking about this I will drop this patch for backward compatibility and in topology masks treat CPUs as being cores*threads On 9/28/22 15:21, Pierre Morel wrote: > > > On 9/27/22 11:44, Cédric Le Goater wrote: >> On 9/5/22 17:10, Pierre Morel wrote: >>> >>> >>> On 9/5/22 13:32, Nico Boehr wrote: >>>> Quoting Pierre Morel (2022-09-02 09:55:22) >>>>> S390x do not support multithreading in the guest. >>>>> Do not let admin falsely specify multithreading on QEMU >>>>> smp commandline. >>>>> >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> --- >>>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>>> index 70229b102b..b5ca154e2f 100644 >>>>> --- a/hw/s390x/s390-virtio-ccw.c >>>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >>>>> MachineClass *mc = MACHINE_GET_CLASS(machine); >>>>> int i; >>>>> + /* Explicitely do not support threads */ >>>> ^ >>>> Explicitly >>>> >>>>> + assert(machine->smp.threads == 1); >>>> >>>> It might be nicer to give a better error message to the user. >>>> What do you think about something like (broken whitespace ahead): >>>> >>>> if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { >>>> error_setg(&error_fatal, "More than one thread specified, >>>> but multithreading unsupported"); >>>> return; >>>> } >>>> >>> >>> >>> OK, I think I wanted to do this and I changed my mind, obviously, I >>> do not recall why. >>> I will do almost the same but after a look at error.h I will use >>> error_report()/exit() instead of error_setg()/return as in: >>> >>> >>> + /* Explicitly do not support threads */ >>> + if (machine->smp.threads != 1) { >>> + error_report("More than one thread specified, but >>> multithreading unsupported"); >>> + exit(1); >>> + } >> >> >> or add an 'Error **errp' parameter to s390_init_cpus() and use >> error_setg() >> as initially proposed. s390x_new_cpu() would benefit from it also. >> > OK, Thanks, > > Pierre >
On 9/28/22 18:16, Pierre Morel wrote: > More thinking about this I will drop this patch for backward compatibility and in topology masks treat CPUs as being cores*threads yes. You never know, people might have set threads=2 in their domain file (like me). You could give the user a warning though, with warn_report(). Thanks, C. > > > > On 9/28/22 15:21, Pierre Morel wrote: >> >> >> On 9/27/22 11:44, Cédric Le Goater wrote: >>> On 9/5/22 17:10, Pierre Morel wrote: >>>> >>>> >>>> On 9/5/22 13:32, Nico Boehr wrote: >>>>> Quoting Pierre Morel (2022-09-02 09:55:22) >>>>>> S390x do not support multithreading in the guest. >>>>>> Do not let admin falsely specify multithreading on QEMU >>>>>> smp commandline. >>>>>> >>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>>> --- >>>>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>>>> index 70229b102b..b5ca154e2f 100644 >>>>>> --- a/hw/s390x/s390-virtio-ccw.c >>>>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >>>>>> MachineClass *mc = MACHINE_GET_CLASS(machine); >>>>>> int i; >>>>>> + /* Explicitely do not support threads */ >>>>> ^ >>>>> Explicitly >>>>> >>>>>> + assert(machine->smp.threads == 1); >>>>> >>>>> It might be nicer to give a better error message to the user. >>>>> What do you think about something like (broken whitespace ahead): >>>>> >>>>> if (machine->smp.threads != 1) {if (machine->smp.threads != 1) { >>>>> error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported"); >>>>> return; >>>>> } >>>>> >>>> >>>> >>>> OK, I think I wanted to do this and I changed my mind, obviously, I do not recall why. >>>> I will do almost the same but after a look at error.h I will use error_report()/exit() instead of error_setg()/return as in: >>>> >>>> >>>> + /* Explicitly do not support threads */ >>>> + if (machine->smp.threads != 1) { >>>> + error_report("More than one thread specified, but multithreading unsupported"); >>>> + exit(1); >>>> + } >>> >>> >>> or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg() >>> as initially proposed. s390x_new_cpu() would benefit from it also. >>> >> OK, Thanks, >> >> Pierre >> >
On Fri, Sep 02, 2022 at 09:55:22AM +0200, Pierre Morel wrote: > S390x do not support multithreading in the guest. > Do not let admin falsely specify multithreading on QEMU > smp commandline. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 70229b102b..b5ca154e2f 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) > MachineClass *mc = MACHINE_GET_CLASS(machine); > int i; > > + /* Explicitely do not support threads */ > + assert(machine->smp.threads == 1); What is the functional effect for currently released QEMU versions if a user has set threads == 2 for an s390 machine ? Is the threads setting simply ignored ? If we want to eliminate this mistake, then there's two possible options * If it had no effect, treat this like a deprecation process where we print a warning for 2 releases, and then turn the warning into an error. Gives a little grace to fix the config mistakes some users might have made, at a time convenient to them. Or * If it had effect and we need migration compatibility then forbid threads > 1 only for new machine type versions, so existing deployed guests are not changed. With regards, Daniel
On 9/28/22 20:11, Daniel P. Berrangé wrote: > On Fri, Sep 02, 2022 at 09:55:22AM +0200, Pierre Morel wrote: >> S390x do not support multithreading in the guest. >> Do not let admin falsely specify multithreading on QEMU >> smp commandline. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 70229b102b..b5ca154e2f 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) >> MachineClass *mc = MACHINE_GET_CLASS(machine); >> int i; >> >> + /* Explicitely do not support threads */ >> + assert(machine->smp.threads == 1); > > What is the functional effect for currently released QEMU versions > if a user has set threads == 2 for an s390 machine ? Is the > threads setting simply ignored ? It is not ignored, the number of CPUs per sockets seen by the guest is cores*threads > > If we want to eliminate this mistake, then there's two possible > options > > * If it had no effect, treat this like a deprecation process > where we print a warning for 2 releases, and then turn the > warning into an error. Gives a little grace to fix the config > mistakes some users might have made, at a time convenient to > them. > > Or > > * If it had effect and we need migration compatibility then forbid > threads > 1 only for new machine type versions, so existing > deployed guests are not changed. > > With regards, > Daniel Thanks for your comments Daniel. I will need to forbid threads > 1 for new machine. regards, Pierre
On 9/28/22 18:28, Cédric Le Goater wrote: > On 9/28/22 18:16, Pierre Morel wrote: >> More thinking about this I will drop this patch for backward >> compatibility and in topology masks treat CPUs as being cores*threads > > yes. You never know, people might have set threads=2 in their > domain file (like me). You could give the user a warning though, > with warn_report(). More thinking, I come back to the first idea after Daniel comment and protect the change with a new machine type version. > > Thanks, > > C. > > >>
On 10/11/22 09:21, Pierre Morel wrote: > > > On 9/28/22 18:28, Cédric Le Goater wrote: >> On 9/28/22 18:16, Pierre Morel wrote: >>> More thinking about this I will drop this patch for backward compatibility and in topology masks treat CPUs as being cores*threads >> >> yes. You never know, people might have set threads=2 in their >> domain file (like me). You could give the user a warning though, >> with warn_report(). > > More thinking, I come back to the first idea after Daniel comment and protect the change with a new machine type version. yes. That would be another machine class attribute to set in the new machine, may be 'max_threads' to compare with the user provided value. C. > > >> >> Thanks, >> >> C. >> >> >>> >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 70229b102b..b5ca154e2f 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine) MachineClass *mc = MACHINE_GET_CLASS(machine); int i; + /* Explicitely do not support threads */ + assert(machine->smp.threads == 1); + /* initialize possible_cpus */ mc->possible_cpu_arch_ids(machine);
S390x do not support multithreading in the guest. Do not let admin falsely specify multithreading on QEMU smp commandline. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 3 +++ 1 file changed, 3 insertions(+)