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

[Laine Stump]

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.