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

[Daniel P. Berrange]

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

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 451ccad..2d35d68 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -346,6 +346,10 @@ struct _virDomainNetDef {
>              virVirtualPortProfileParams virtPortProfile;
>          } direct;
>      } data;
> +    struct {
> +        int sndbuf_specified : 1;
> +        int sndbuf;
> +    } tune;

And this is storing an int.  They should really all be ints,
or all be longs.

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.

Regards,
Daniel