[pve-devel] [PATCH qemu-server 2/2] fix #1384: fix cpu socket id calculation for hotplug

[Fabian Grünbichler]

On Wed, May 17, 2017 at 12:50:08PM +0200, Tobias Böhm wrote:
> Am 17.05.2017 um 11:44 schrieb Thomas Lamprecht:
> > On 05/17/2017 09:47 AM, Fabian Grünbichler wrote:
> > > the case of multiple single-core vcpus was not handled
> > > properly
> > > 
> > > Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> > > ---
> > >   PVE/QemuServer.pm | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > > index ed6c598..d480d55 100644
> > > --- a/PVE/QemuServer.pm
> > > +++ b/PVE/QemuServer.pm
> > > @@ -1723,9 +1723,14 @@ sub print_cpu_device {
> > >       my $sockets = $conf->{sockets} // 1;
> > >       my $cores = $conf->{cores} // 1;
> > > -    my $current_core = ($id - 1) % $cores;
> > > -    my $current_socket = int(($id - $current_core)/$cores);
> > > +    # single core default
> > > +    my $current_core = 0;
> > > +    my $current_socket = $id - 1;
> > > +    if ($cores > 1) {
> > > +	$current_core = ($id - 1) % $cores;
> > > +	$current_socket = int(($id - $current_core)/$cores);
> > > +    }
> > 
> > I still find the logic strange and hard to understand for that what it does.
> > Also the cpu IDs are not added in order, so I'm not sure if hotunplug always
> > removed the "correct" CPU

what do you mean with "are not added in order?"

> > 
> > I've looked at Tobias patch and at wasn't that happy with the existing code,
> > my current approach is to move the id wrap logic away from print_cpu_device
> > I'm happier with the print_cpu_device as it is below but not with the code
> > using it,
> > still need to get the bigger picture, maybe we can do this nicer altogether.
> > Or Wolfgang posts his superior QemuServer machine patches and solves this
> > all :-)
> > 
> > [ snip ]
> 
> Hi,
> 
> if you decrement the id before passing it to print_cpu_device, be aware
> that you need to increment the variable in the subroutine in the last
> line, where it is used for the cpu-id, which seems to be 1-based, in
> order to not change the behaviour there.
> 
> Regards,
> Tobias

Indeed - shifting all CPU ids by one would most likely break migration.