Re: [PATCH 0/6] Follow-up patches for IOThread API/display

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 09, 2015 at 08:27:10AM -0400, John Ferlan wrote:
> 
> 
> On 03/08/2015 07:03 AM, Ján Tomko wrote:
> > ACK series, thanks for touching up VcpuInfo and EmulatorInfo as well!
> > 
> > There's one bug that I noticed:
> > If the CPUs are pinned domain-wide, that is:
> >   <vcpu placement='static' cpuset='0-1'>4</vcpu>
> >   <iothreads>2</iothreads>
> > 
> > Both vcpu threads and iothreads will inherit this pinning.
> > 
> > For a shutoff domain, vcpupininfo will display 0-1 for all vcpus, but
> > iothreadsinfo shows 0-4, even though they will get pinned to 0-1 after
> > domain startup.
> > 
> > Turns out the vpcupin info is filled for all the vcpus when the XML is
> > parsed since commit 10f8a45deb0f057a70a0d49794d3a3d19d17dceb
> > 
> > Falling back to targetDef->cpumask in qemuDomainGetIOThreadsConfig
> > (as qemuDomainGetEmulatorPinInfo does) would solve that too.
> > 
> 
> Squashed the following into patch 4 to address this...

I don't think this functional change belongs to the patch refactoring
the function to use the virBitmap* APIs. But since the functionality has
not yet been released, nobody should feel the need to backport any of
these commits, so mixing them up is not that big deal.

> 
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2a9db1b..ceceafa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5653,6 +5653,8 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>                               virDomainIOThreadInfoPtr **info)
>  {
>      virDomainIOThreadInfoPtr *info_ret = NULL;
> +    virBitmapPtr bitmap = NULL;
> +    virBitmapPtr cpumask = NULL;
>      int hostcpus;
>      size_t i;
>      int ret = -1;
> @@ -5668,7 +5670,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>  
>      for (i = 0; i < targetDef->iothreads; i++) {
>          virDomainVcpuPinDefPtr pininfo;
> -        virBitmapPtr bitmap = NULL;
>  
>          if (VIR_ALLOC(info_ret[i]) < 0)
>              goto cleanup;
> @@ -5681,20 +5682,22 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>                                               targetDef->cputune.niothreadspin,
>                                               i + 1);
>          if (!pininfo) {
> -            if (!(bitmap = virBitmapNew(hostcpus)))
> -                goto cleanup;
> -            virBitmapSetAll(bitmap);
> +            if (targetDef->cpumask) {
> +                cpumask = targetDef->cpumask;
> +            } else {
> +                if (!(bitmap = virBitmapNew(hostcpus)))
> +                    goto cleanup;
> +                virBitmapSetAll(bitmap);
> +                cpumask = bitmap;
> +            }
>          } else {
> -            bitmap = pininfo->cpumask;
> +            cpumask = pininfo->cpumask;
>          }
> -        if (virBitmapToData(bitmap, &info_ret[i]->cpumap,
> -                            &info_ret[i]->cpumaplen) < 0) {
> -            if (!pininfo)
> -                virBitmapFree(bitmap);
> +        if (virBitmapToData(cpumask, &info_ret[i]->cpumap,
> +                            &info_ret[i]->cpumaplen) < 0)
>              goto cleanup;
> -        }
> -        if (!pininfo)
> -            virBitmapFree(bitmap);
> +        virBitmapFree(bitmap);
> +        bitmap = NULL;
>      }
>  
>      *info = info_ret;
> @@ -5707,6 +5710,7 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>              virDomainIOThreadsInfoFree(info_ret[i]);
>          VIR_FREE(info_ret);
>      }
> +    virBitmapFree(bitmap);
>  
>      return ret;
>  }
> 
> 
> Replaced the overly aggressively removed lines from patch 5...
> 
> I will send a separate patch for the domain_conf.c change that
> addresses any needs for iothreadspin setup to use the def->cpumask.

However the squashed-in patch could've benefited from going through a
standard review process. If you plan to copy the vcpupin code in domain_conf.c
to do the same with iothreadspin, the squashed-in change is redundant.

Jan

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]