[pve-devel] [PATCH qemu-server] Fix #1384: add missing decrement to calculation of socket-id

[Fabian Grünbichler]

On Wed, May 17, 2017 at 11:47:10AM +0200, Tobias Böhm wrote:
> Am 17.05.2017 um 10:55 schrieb Fabian Grünbichler:
> > On Mon, May 15, 2017 at 04:16:12PM +0200, Tobias Böhm wrote:
> > > For calculation of "current_core" the input variable id is decremented.
> > > For calculation of "current_socket" this decrement was missing resulting
> > > in a wrong value when "cores" is set to 1.
> > > 
> > > Signed-off-by: Tobias Böhm <tb at robhost.de>
> > > ---
> > >  PVE/QemuServer.pm | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > > index c6bcb50..4919928 100644
> > > --- a/PVE/QemuServer.pm
> > > +++ b/PVE/QemuServer.pm
> > > @@ -1749,7 +1749,7 @@ sub print_cpu_device {
> > >      my $cores = $conf->{cores} || 1;
> > > 
> > >      my $current_core = ($id - 1) % $cores;
> > > -    my $current_socket = int(($id - $current_core)/$cores);
> > > +    my $current_socket = int(($id - 1 - $current_core)/$cores);
> > > 
> > >      return "$cpu-x86_64-cpu,id=cpu$id,socket-id=$current_socket,core-id=$current_core,thread-id=0";
> > >  }
> > > --
> > > 2.11.0
> > 
> > Seems like I missed this mail - I already sent an alternative patch
> > earlier today ;) your version fixes your use case, but breaks VMs with
> > more than 1 core per socket.
> > 
> > Sorry for the confusion, and thanks for the bug report and proposed fix!
> > 
> 
> Hi,
> 
> thank you very much for handling the report so fast.
> 
> How did you test my patch or run the calculations that it shows a
> difference for multicore setups? I tested it and it worked as before for
> multicore setups.

You are absolutely right - mea culpa! The index is (was) off by one in
all cases, but for multi core it was hidden by the division and
subsequent rounding.

I'll probably apply your patch and add another cleanup commit dropping
the unused sockets variable, unless there are further objections by
Thomas. Revamping the whole thing should be done separately IMHO,
especially because it needs careful testing to prevent breaking
migration.