Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

On Thu, 30 Aug 2018 11:08:00 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > ---
> > > >  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
> > > >     
> > >   
> >   
>