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

Re: [libvirt] [PATCH] Enable tuning of qemu network tap device "sndbuf" size



On 01/14/2011 08:27 AM, Daniel P. Berrange wrote:
On Thu, Jan 13, 2011 at 01:45:29AM -0500, Laine Stump wrote:
This is in response to a request in:

    https://bugzilla.redhat.com/show_bug.cgi?id=665293

In short, under heavy load, it's possible for qemu's networking to
lock up due to the tap device's default 1MB sndbuf being
inadequate. adding "sndbuf=0" to the qemu commandline -netdevice
option will alleviate this problem (sndbuf=0 actually sets it to
0xffffffff).

Because we must be able to explicitly specify "0" as a value, the
standard practice of "0 means not specified" won't work here. Instead,
virDomainNetDef also has a sndbuf_specified, which defaults to 0, but
is set to 1 if some value was given.

The sndbuf value is put inside a<tune>  element of each<interface>  in
the domain. The intent is that further tunable settings will also be
placed inside this elemnent.

      <interface type='network'>
        ...
        <tune>
          <sndbuf>0</sndbuf>
        ...
        </tune>
      </interface>
---

Note that in qemuBuildHostNetStr() I have moved

     if (vhostfd&&  *vhostfd) {
         virBufferVSprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd);

into a newly created "if (is_tap) { ... }" block. This always should
have been inside such a conditional, but none existed until now. (I
can make this a separate patch if anyone wants, but it seemed so
simple and obvious that I took the slothenly way out :-)

Also, as with the vhost patch, I didn't get the html docs updated for
this addition either. I will add both in a single followup patch next
week.

  docs/schemas/domain.rng |   10 ++++++++++
  src/conf/domain_conf.c  |   31 +++++++++++++++++++++++++++++--
  src/conf/domain_conf.h  |    4 ++++
  src/qemu/qemu_command.c |   19 +++++++++++++++++--
  4 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 04ed502..5d1b8cf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2280,6 +2280,7 @@ err_exit:
  static virDomainNetDefPtr
  virDomainNetDefParseXML(virCapsPtr caps,
                          xmlNodePtr node,
+                        xmlXPathContextPtr ctxt,
                          int flags ATTRIBUTE_UNUSED) {
      virDomainNetDefPtr def;
      xmlNodePtr cur;
@@ -2298,15 +2299,19 @@ virDomainNetDefParseXML(virCapsPtr caps,
      char *internal = NULL;
      char *devaddr = NULL;
      char *mode = NULL;
+    unsigned long sndbuf;
      virNWFilterHashTablePtr filterparams = NULL;
      virVirtualPortProfileParams virtPort;
      bool virtPortParsed = false;
+    xmlNodePtr oldnode = ctxt->node;

      if (VIR_ALLOC(def)<  0) {
          virReportOOMError();
          return NULL;
      }

+    ctxt->node = node;
+
      type = virXMLPropString(node, "type");
      if (type != NULL) {
          if ((int)(def->type = virDomainNetTypeFromString(type))<  0) {
@@ -2593,7 +2598,13 @@ virDomainNetDefParseXML(virCapsPtr caps,
          }
      }

+    if (virXPathULong("string(./tune/sndbuf)", ctxt,&sndbuf)>= 0) {
+        def->tune.sndbuf = sndbuf;
+        def->tune.sndbuf_specified = 1;
+    }
This is parsing a 'long'

@@ -6315,6 +6336,12 @@ virDomainNetDefFormat(virBufferPtr buf,
          VIR_FREE(attrs);
      }

+    if (def->tune.sndbuf_specified) {
+        virBufferAddLit(buf,   "<tune>\n");
+        virBufferVSprintf(buf, "<sndbuf>%d</sndbuf>\n", def->tune.sndbuf);
+        virBufferAddLit(buf,   "</tune>\n");
+    }
But this is printing an int


Yeah, the problem was that I needed to generate the value both with a virXPath*() function (which only had something to return a long) as well as with a conversion from string (and virStrToLong_*() could only give me an int. So either way I was out of luck. eblake came to the rescue with a patch that rounds out both function sets to do both types, so I'm going to make it an unsigned long.



Also bitfields should be 'unsigned' rather than int, otherwise
you're actually storing  0 and -1  as possible values, rather
than 0&  1. Or alternatively could just use a 'bool' here.
Since there's only one bitfield, padding means we're not saving
any space.

My first instinct was to make it a bool, but I saw cases of others using int bitfields for the same purpose, and nobody using bool; I decided to conform to existing practice.

If it's okay with you to make it a bool, that's what I'll do in v2.


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