[libvirt] [PATCHv2 1/2] conf: add options for disabling segment offloading
John Ferlan
jferlan at redhat.com
Wed Sep 24 14:31:35 UTC 2014
On 09/24/2014 09:44 AM, Ján Tomko wrote:
> 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.
>
Yeah - good point. I guess I associated the rest/most with TCP... Of
course you could use "TCP/UDP" instead of just TCP... I'm OK without
the TCP reference though - it was a "extra" suggestion.
>>
>>> + <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.
>
Yes - I do agree that by not spelling them out it may cause someone to
"pause" before adding them.
However, of course, we're engineers so we have this "desire" to try
anyway and/or know what these new knobs/things do.
I guess it's one of those things that I don't like - that is seeing an
acronym on a website or in documentation that's not spelled out.
>>
>>
>>> + 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'?
>
That's fine - it was the "offloads off" that didn't read well.
>>
>>> + 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.
>
>
That's fine - like I said - I didn't see anything wrong with the code -
maybe something for the todo list to reduce the "need" for all the
repetition and silly errors some of us make...
John
More information about the libvir-list
mailing list