Re: [libvirt] [PATCHv3 10/11] qemu: Adapt qemuBuildInterfaceCommandLine to to multiqueue net

20 May 2013


      On 20.05.2013 19:15, Laine Stump wrote:
...
On 05/16/2013 08:49 AM, Michal Privoznik wrote:
...
---
 src/qemu/qemu_command.c | 82 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 24 deletions(-)
...
...
goto cleanup;
     } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
-        tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop);
-        if (tapfd < 0)
+        if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) {
+            virReportOOMError();
+            goto cleanup;
+        }
+        tapfdSize = 1;
+        tapfd[0] = qemuPhysIfaceConnect(def, driver, net,
+                                        qemuCaps, vmop);
macvtap doesn't support multiple queues? But macvtap is supposed to be
the panacea of high performance...
Unfortunately not yet. Macvtap is still under development in question of MQ.
...
...
+        if (tapfd[0] < 0)
             goto cleanup;
     }
@@ -6510,28 +6526,34 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
         actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
         /* Attempt to use vhost-net mode for these types of
            network device */
-        int vhostfd;
-        int vhostfdSize = 1;
+        if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) {
+            virReportOOMError();
+            goto cleanup;
+        }
+        vhostfdSize = 1;
I am *really* losing track of which layer of which set of fds is getting
the "multi" treatment in each patch. I think there are just too many
steps in this patch series. It would be easier to follow if there was a
patch to add the config parser/formatter/rng/docs, then just another
single patch to wire that data all the way up and down the stack. I find
myself spending time at each stage ttrying to decide if something is a
bug, or just an interim thing that will be fixed in a later step.
Well, I keep modifying patches as you review them. I think I gonna post
one more version - just in the form as you requested.
...
...
+
+        if (qemuOpenVhostNet(def, net, qemuCaps, vhostfd, &vhostfdSize) < 0)
+            goto cleanup;
+    }
-        if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, &vhostfdSize) < 0)
+    for (i = 0; i < tapfdSize; i++) {
+        virCommandTransferFD(cmd, tapfd[i]);
+        if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0) {
+            virReportOOMError();
             goto cleanup;
-        if (vhostfdSize > 0) {
-            virCommandTransferFD(cmd, vhostfd);
-            if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) {
+        }
+    }
+
+    for (i = 0; i < vhostfdSize; i++) {
+        if (vhostfd[i] >= 0) {
+            virCommandTransferFD(cmd, vhostfd[i]);
+            if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) {
                 virReportOOMError();
                 goto cleanup;
             }
         }
     }
-    if (tapfd >= 0) {
-        virCommandTransferFD(cmd, tapfd);
-        if (virAsprintf(&tapfdName, "%d", tapfd) < 0) {
-            virReportOOMError();
-            goto cleanup;
-        }
-    }
-
     /* Possible combinations:
      *
      *  1. Old way:   -net nic,model=e1000,vlan=1 -net tap,vlan=1
@@ -6544,8 +6566,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
         virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
         if (!(host = qemuBuildHostNetStr(net, driver,
                                          ',', vlan,
-                                         &tapfdName, tapfdName ? 1 : 0,
-                                         &vhostfdName, vhostfdName ?  1 : 0)))
+                                         tapfdName, tapfdSize,
+                                         vhostfdName, vhostfdSize)))
             goto cleanup;
         virCommandAddArgList(cmd, "-netdev", host, NULL);
     }
@@ -6562,8 +6584,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
           virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
         if (!(host = qemuBuildHostNetStr(net, driver,
                                          ',', vlan,
-                                         &tapfdName, tapfdName ? 1 : 0,
-                                         &vhostfdName, vhostfdName ? 1 : 0)))
+                                         tapfdName, tapfdSize,
+                                         vhostfdName, vhostfdSize)))
             goto cleanup;
         virCommandAddArgList(cmd, "-net", host, NULL);
     }
@@ -6572,6 +6594,18 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 cleanup:
     if (ret < 0)
         virDomainConfNWFilterTeardown(net);
+    for (i = 0; i < tapfdSize; i++) {
+        if (ret < 0)
+            VIR_FORCE_CLOSE(tapfd[i]);
+        VIR_FREE(tapfdName[i]);
Okay, tapfdSize isn't set until/unless tapfd and tapfdName are both
successfully allocated...
...
+    }
+    for (i = 0; i < vhostfdSize; i++) {
+        if (ret < 0)
+            VIR_FORCE_CLOSE(vhostfd[i]);
+        VIR_FREE(vhostfdName[i]);
Same here. (I mention this because I was worried about it initially :-)
...
+    }
+    VIR_FREE(tapfd);
+    VIR_FREE(vhostfd);
     VIR_FREE(nic);
     VIR_FREE(host);
     VIR_FREE(tapfdName);
ACK, aside from making tapfdSize an int* (so it can be modified on
failure/non-support in qemuIfaceNetworkConnect), and an answer about
support for multiple queues when macvtap is used.
Oh, actually another problem - in cases where multiple queues are
requested and can't be honored due to interface type (or any other
reason), I think we should log an error and fail, but it looks like you
just ignore it. So I guess that also needs fixing (or an explanation).
Yeah, I was thinking about it too.

Michal

    