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

[Eric Blake]

On 01/12/2011 11:45 PM, 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.

s/elemnent/element/

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

Added to my list of things to remind you about, if you forget :)

> +    unsigned long sndbuf;
>  
> +    if (virXPathULong("string(./tune/sndbuf)", ctxt, &sndbuf) >= 0) {
> +        def->tune.sndbuf = sndbuf;

Hmm, we don't have virXPathInt or virXPathUInt.

Type mismatch.  If sndbuf > INT_MAX, we've got issues.  Should
def->tune.sndbuf be unsigned?  If so...

> @@ -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);

this changes to %lu

> +    struct {
> +        int sndbuf_specified : 1;
> +        int sndbuf;

and this line changes

> @@ -1662,8 +1665,11 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>                            type_sep, net->info.alias);
>      }
>  
> -    if (vhostfd && *vhostfd) {
> -        virBufferVSprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd);
> +    if (is_tap) {
> +        if (vhostfd && *vhostfd)
> +            virBufferVSprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd);
> +        if (net->tune.sndbuf_specified)
> +            virBufferVSprintf(&buf, ",sndbuf=%d", net->tune.sndbuf);

%lu

>                  def->backend = VIR_DOMAIN_NET_BACKEND_TYPE_QEMU;
>              }
>              def->backend_specified = 1;
> +        } else if (STREQ(keywords[i], "sndbuf") && values[i]) {
> +            if (virStrToLong_i(values[i], NULL, 10, &def->tune.sndbuf) < 0) {

Oh, we don't have virStrToLong_ul.

How odd.  Maybe we should do a prerequisite patch that rounds out the
type system so that both xml.h and util.h have a matching set of
functions (for every type we can parse out of xml, we also have a way to
parse it out of a string), so that you can pick the best type to use.

At any rate, 'unsigned long' may be overkill, but I do think this should
use 'unsigned int' rather than int in the in-memory representation.

I'll go ahead and propose a quickie patch to fix the missing utility
functions, then we can do a v2 of this on top of that with a sane type
throughout.  But other than the type mismatch and one spelling nit, I
like the approach of this patch.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110113/4bcbdb5d/attachment-0001.sig>