[libvirt] [PATCHv2 1/2] conf: add options for disabling segment offloading

[Ján Tomko]

On 09/24/2014 01:24 AM, John Ferlan wrote:
> 
> 
> On 09/18/2014 10:15 AM, Ján Tomko wrote:
>> Add options for tuning segment offloading:
>> <driver>
>>   <host csum='off' gso='off' tso4='off' tso6='off'
>>         ecn='off' ufo='off'/>
>>   <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/>
>> </driver>
>> which control the respective host_ and guest_ properties
>> of the virtio-net device.
>> ---
>>  docs/formatdomain.html.in                          |  24 ++-
>>  docs/schemas/domaincommon.rng                      |  44 ++++-
>>  src/conf/domain_conf.c                             | 218 ++++++++++++++++++++-
>>  src/conf/domain_conf.h                             |  15 ++
>>  .../qemuxml2argv-net-virtio-disable-offloads.xml   |  35 ++++
>>  tests/qemuxml2xmltest.c                            |   1 +
>>  6 files changed, 329 insertions(+), 8 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 8c03ebb..5dd70a4 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null
>>        <source network='default'/>
>>        <target dev='vnet1'/>
>>        <model type='virtio'/>
>> -      <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b>
>> +      <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'>
>> +        <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/>
>> +        <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/>
>> +      </driver>
>> +      </b>
>>      </interface>
>>    </devices>
>>    ...</pre>
>> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null
>>          processor, resulting in much higher throughput.
>>          <span class="since">Since 1.0.6 (QEMU and KVM only)</span>
>>        </dd>
>> +      <dt><code>host offloading options</code></dt>
> 
> Should this be <code>host</host> offloading options?  
> or host TCP options (in some way to indicate that these
> are TCP level options).  Probably also want your disclaimer
> use of these should be for only those that know what they
> are doing...
> 

Well ufo is an UDP option.

> 
>> +      <dd>
>> +        The <code>csum</code>, <code>gso</code>, <code>tso4</code>,
>> +        <code>tso6</code>, <code>ecn</code>, <code>ufo</code>
> 
> Should read: ecn, and ufo  
> 
> Should you "spell out" what each translates to?
> 
> csum (checksums), gso (generic segmentation offload),
> tso (tcp segmentation offload v4 and v6), ecn (congestion
>  notification), and ufo (UDP fragment offloads)
> 

I thought not spelling them out was the equivalent of the "only use this if
you know what you're doing" disclaimer.

> 
> 
>> +        attributes with possible values <code>on</code>
>> +        and <code>off</code> can be used to turn host offloads off.
> 
> s/turn host offloads off/turn off host TCP options/
> 
> Saying "offloads off" aloud just doesn't seem right.
> 
>> +        By default, the supported offloads are enabled by QEMU.
> 
> s/the supported offloads/the TCP options/
> 
>> +        <span class="since">Since 1.2.9 (QEMU only)</span>
>> +      </dd>
>> +      <dt>guest offloading options</dt>
> 
> Similar in here...   Does the host setting override the guest value
> or can the host say "tso4=off" while the guest has "tso4=on"?  Curious
> mainly.

It can say that, but I doubt it'll work.

> 
>> +      <dd>
>> +        The <code>csum</code>, <code>tso4</code>,
>> +        <code>tso6</code>, <code>ecn</code>, <code>ufo</code>
> 
> same here with the "and" - although I suppose you could just
> reference the <host> bits "above"... 
> 
> Another way to say it is guests can use the same options except gso
> 
>> +        attributes with possible values <code>on</code>
>> +        and <code>off</code> can be used to turn guest offloads off.
> 
> s/turn guest offloads off/turn off guest TCP options/

How about 'turn off guest offload options'?

> 
>> +        By default, the supported offloads are enabled by QEMU.
> 
> s/the supported offloads/the TCP options/

> 
> And of course the usage disclaimer!
> 
> 
>> +        <span class="since">Since 1.2.9 (QEMU only)</span>
>> +      </dd>
>>      </dl>
>>  
>>      <h5><a name="elementsBackendOptions">Setting network backend-specific options</a></h5>

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3ccec1c..cdaafa6 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>      char *ioeventfd = NULL;
>>      char *event_idx = NULL;
>>      char *queues = NULL;
>> +    char *str = NULL;
>>      char *filter = NULL;
>>      char *internal = NULL;
>>      char *devaddr = NULL;
>> @@ -7385,6 +7386,115 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>              }
>>              def->driver.virtio.queues = q;
>>          }
> 
> How about something like this? Not that you have anything wrong, but
> it avoids the chance that some "change" in the future requires 11 similar
> changes...

I was worried it wouldn't be clear enough and since we use similar repetition
all over domain_conf, it would be better handled separately.

Jan

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