Re: [PATCH v5 4/5] qemu: Add support to pin IOThreads to specific CPU

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

 




On 03/10/2015 12:37 PM, Ján Tomko wrote:
> On Tue, Mar 10, 2015 at 11:20:43AM -0400, John Ferlan wrote:
>>
>>
>> On 03/10/2015 09:51 AM, Ján Tomko wrote:
>>> On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
>>
>> <...snip...>
>>
>>>> +
>>>> +    /* pinning to all physical cpus means resetting,
>>>
>>> It doesn't.
>>> By default the iothreads inherit the pinning from the domain's cpumask.
>>>
>>> A completely clear bitmap would be a better value to mean resetting,
>>> since it makes no sense otherwise. But getting the cpumask in that case
>>> won't be that easy.
>>>
>>
>> So again - this is taken from qemuDomainPinVcpuFlags() - if it's invalid
>> here, then it's invalid there as well.
> 
> Yes, that function might also not behave properly on the corner case of
> pinning a vcpu to all CPUs.
> 
> But that one has been released for ages. This is new code that doesn't
> have to repeat its mistakes.
> 

No issues with doing things right and obviously I wasn't aware
the current API's are broken...

>>
>> Is your objection the comment?  Should it be striken or restated?
>>
> 
> It's not the comment I have a problem with, it's the behavior that follows it.
> 
> Calling this API on a domain with a per-domain cpuset to pin an iothread
> to all pCPUs will result in:
> 
> a) for AFFECT_LIVE
>   The iothread will get pinned to all pCPUs
>   The pininfo will get deleted from the definition. But missing pininfo
>   does not mean all pCPUs if there is a per-domain cpuset.
> 
> b) for AFFECT_CONFIG
>   The pininfo will be deleted, even though it does not match the
>   cpumask.
> 
> I think the easiest 'fix' is to just drop all the 'doReset' functionality
> and do not allow deleting pininfo with this API.
> 
> Alternatively, (since you already rejected
> VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO above) you can pin the iothread to
> the domain's cpuset.
> 
> Also, the full bitmap is ambiguous - does it means 'reset
> it to default' or 'pin it to all pCPUs'?
> 

Well let's just say I don't know all the nuances and known
issues of cpu pinning... 

Knowing that the other APIs (vcpu and emulator) are broken - are
you planning to send patches to fix them as well? Or is that
something I should do mimicing whatever is settled upon here?

So, from how I read your comments I have the following diffs which 
removes the doReset conditions, and removes the PinDel calls as a
result of them. Theoretically now nothing calls the PinDel API and
while I suppose I could remove it, once the hot remove an IOThread
code is added, I'd need it back.

With respect to the pin to the domain cpuset - this API takes a new
cpumap so I'm not sure I understand under what condition would thus
use the domain's cpuset.  Maybe I read your comment too literally

John


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e3d0acf..74440f1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5773,7 +5773,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
     virCapsPtr caps = NULL;
     virDomainDefPtr persistentDef = NULL;
     virBitmapPtr pcpumap = NULL;
-    bool doReset = false;
     qemuDomainObjPrivatePtr priv;
     virDomainVcpuPinDefPtr *newIOThreadsPin = NULL;
     size_t newIOThreadsPinNum = 0;
@@ -5823,12 +5822,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
         goto endjob;
     }
 
-    /* pinning to all physical cpus means resetting,
-     * so check if we can reset setting.
-     */
-    if (virBitmapIsAllSet(pcpumap))
-        doReset = true;
-
     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
 
         if (priv->iothreadpids == NULL) {
@@ -5891,17 +5884,13 @@ qemuDomainPinIOThread(virDomainPtr dom,
             }
         }
 
-        if (doReset) {
-            virDomainIOThreadsPinDel(vm->def, iothread_id);
-        } else {
-            if (vm->def->cputune.iothreadspin)
-                virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin,
-                                             vm->def->cputune.niothreadspin);
+        if (vm->def->cputune.iothreadspin)
+            virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin,
+                                         vm->def->cputune.niothreadspin);
 
-            vm->def->cputune.iothreadspin = newIOThreadsPin;
-            vm->def->cputune.niothreadspin = newIOThreadsPinNum;
-            newIOThreadsPin = NULL;
-        }
+        vm->def->cputune.iothreadspin = newIOThreadsPin;
+        vm->def->cputune.niothreadspin = newIOThreadsPinNum;
+        newIOThreadsPin = NULL;
          if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
             goto endjob;
@@ -5930,24 +5919,20 @@ qemuDomainPinIOThread(virDomainPtr dom,
             goto endjob;
         }
 
-        if (doReset) {
-            virDomainIOThreadsPinDel(persistentDef, iothread_id);
-        } else {
-            if (!persistentDef->cputune.iothreadspin) {
-                if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0)
-                    goto endjob;
-                persistentDef->cputune.niothreadspin = 0;
-            }
-            if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin,
-                                         &persistentDef->cputune.niothreadspin,
-                                         cpumap,
-                                         maplen,
-                                         iothread_id) < 0) {
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("failed to update or add iothreadspin xml "
-                                 "of a persistent domain"));
+        if (!persistentDef->cputune.iothreadspin) {
+            if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0)
                 goto endjob;
-            }
+            persistentDef->cputune.niothreadspin = 0;
+        }
+        if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin,
+                                     &persistentDef->cputune.niothreadspin,
+                                     cpumap,
+                                     maplen,
+                                     iothread_id) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("failed to update or add iothreadspin xml "
+                             "of a persistent domain"));
+            goto endjob;
         }
 
         ret = virDomainSaveConfig(cfg->configDir, persistentDef);

--
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]